-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add cockroachdb to EFCore CI #1
Comments
@rafiss first off, it's great that you guys are focusing on improving EF support for CockroachDB. I guess the main thing here is to identify what are the actual gaps for full CockroachDB support; ideally a list would be made, and based on that list we'd discuss together what the best way forward. The best outcome here IMHO would be that the gaps aren't that big, and then we'd integrate them into EFCore.PG. BTW we've already discussed adding a UseCockroachDB config option, if that turns out to be necessary. I'm also definitely OK with running CI on CockroachDB, though that would probably something that would need to be added by CockroachDB people. If the gaps are such that the provider would need to be substantially changed, then a separate CockroachDB provider may be the way forward - though I do hope that we don't end up going down this route. One option here would be the fork, while another would be for the CockroachDB provider to extend and override EFCore.PG; this may require work on the EFCore.PG for the proper extensibility hooks (nobody has extended EFCore.PG AFAIK), but that could still be an option. You could also start out by forking and doing changes on your side, but then gradually integrate the changes back upstream into EFCore.PG. To summarize, it all depends on what kind of changes you'd need from EFCore.PG; once we have that identified we can see what's best. How does that sound? |
@roji Thanks for your response! I hear you that a list of specific gaps is most helpful. Actually, we are in progress of creating that list too. We have been working with a team specializing in C# expertise who has has a project of identifying the gaps when running the EF Core test suite against CockroachDB. You can see the changes they've made on this branch: https://github.com/cockroachdb/efcore.pg/tree/cockroach-implementation It's not yet in a state where we'd merge these changes or release them as a standalone provider. This was more of a fact-finding project. Currently, most of the missing functionality is commented with a link to a tracking issue. Those links are private right now, but we're working on using public links for those instead. Also, the list of test failures is available in this spreadsheet: https://docs.google.com/spreadsheets/d/1TPAGVHx9XKZoMzzf58-8SFDGgIyS3w0M--W8hf1n_a4/edit#gid=778464921 (I've shared it with the email address listed in your github profile, but let me know if you'd prefer a different email) To summarize at a high level, the major implementation differences are JSON/JSONB differences, date functions and regular expression support (multi-line regexes vs single line). Some of the gaps are features that CockroachDB doesn't support and will not support in the near-term (e.g. user-defined functions, full-text search, or multi-dimensional arrays). Many of the other gaps seem like simple changes. |
@rafiss sounds like you have a pretty good idea already, that's great. Stuff like unsupported types or even features (user-defined functions, full-text search, or multi-dimensional arrays) shouldn't be very problematic and may not even require any changes - as long as users don't attempt to use them. I think it's generally OK for CockroachDB to simply error if an unsupported operation is attempted by the user (this is the strategy in many other parts. If we find that most/all of the gaps are translation differences, then it's also possible to address this via a plugin; EF Core has an extensibility model which works very well for that (see how spatial support is supported via the NetTopologySuite plugin, also the NodaTime plugin). If that's all it is, then a simple CockroachDB plugin could be used by the user to override any internal mappings and/or translations. But if it goes beyond that (e.g. scaffolding), then extension points do not yet exist for plugins to override other behavior. A hybrid approach may also work, where we made certain minor changes in the core, and then override mappings/translations via a plugin. |
@roji I think it may end up being something like the middle ground you are saying. On that branch I linked to (actually everything has been squashes into one commit 1a3c7bd), some of the workarounds were minor changes to EF Core (e.g., not using However, the great majority of the changes are to skip tests that are not supported on CockroachDB, so hopefully there would be a way to add a conditional "skip test if CRDB" annotation to the EF Core test suite. Actually ideally there would also be a "skip if CRDB version is less than or equal to X" annotation since as we do more releases we plan to support more features. Using the following CRDB settings were important to get a vast number of the tests passing:
|
Yeah, skipping tests should be very easy, and we could definitely integrate that into the core test suite (rather than creating another provider). As an example, see how we skip certain tests when the PostgreSQL version is below a certain threshold; this is done with a simple attribute, which checks the database being tested against. Something similar could be done for CockroachDB, i.e. Re stuff like |
@roji This discussion has been helpful. I think the first step would be for us to help get CockroachDB working in the https://github.com/npgsql/efcore.pg/ CI. What can we do to help you do that? Should we track this in an issue on https://github.com/npgsql/efcore.pg/ ? Ideally, we could get it running as a non-blocking status check before updating all the tests. That way we can make iterative progress on skipping the tests that won't work with CRDB until we get to a state where the CRDB CI is passing. |
Sure thing. The easiest way would be to submit a PR adding this to the build workflow. You could add a special build matrix entry (just like the one running with debug) to test against CockroachDB, that would make it run in parallel to the rest. |
I've rephrased the issue title; and we hope to work on this soon. |
Hi @roji, we will be starting the work for integrating CRDB with EFCore and we will pick up from where we left off previously to integrate CRDB with EFCore CI. Richard (@RichardAZ) will be the primary contributor to work on this over the next couple of months. The main action items are to:
We are hoping to complete this work by end of October and will likely need some support in terms of code reviews etc. Please let me know if you have any questions for us. Thanks! Cc @rafiss, @ZhouXing19 |
Hi @roji, in efcore.pg code, what's the simplest way to allow recognising if the database is CockroachDB. Some parameters from PostgresParameters in NpgsqlConnection may help but they are only available when the connection is Open. |
This is now implemented as a provider here: https://github.com/cockroachdb/efcore.pg.cockroach |
In order for EFCore to fully support CockroachDB there needs to be CockroachDB-aware EFCore provider. After some testing and investigation, we've concluded that there are enough subtle differences between PostgreSQL and CockroachDB that using the PostgreSQL provider as-is does not work 100% with CockroachDB.
I've forked the PostgreSQL provider so that we can decide how to proceed. To resolve this issue, let's decide on one of the following options:
Make changes to npgsql/efcore.pg so that it is CockroachDB-aware, but still supports PostgreSQL by default. The outcome would be for all changes needed to fully support CockroachDB to be merged into the upstream npgsql/efcore.pg repo. Ideally, the CI tests would also run against CockroachDB (perhaps as a non-blocking check). This of course would require the owners of the repo to agree, and we'd build off the basic scaffolding that was added in Basic scaffolding for CockroachDB npgsql/efcore.pg#1379.
Fork npgsql/efcore.pg and make changes so it works with CockroachDB. Then Cockroach Labs would be responsible for maintaining this forked EF Core provider.
cc @roji -- I wonder if you could take a look at this thread and offer any thoughts you have. Option 1 would be our preference, but I'd like to know what is possible for you and your team.
The text was updated successfully, but these errors were encountered: