Skip to content
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

Closed
rafiss opened this issue Oct 25, 2021 · 11 comments · May be fixed by npgsql/efcore.pg#2280
Closed

Add cockroachdb to EFCore CI #1

rafiss opened this issue Oct 25, 2021 · 11 comments · May be fixed by npgsql/efcore.pg#2280

Comments

@rafiss
Copy link
Collaborator

rafiss commented Oct 25, 2021

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:

  1. 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.

  2. 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.

@roji
Copy link

roji commented Oct 26, 2021

@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?

@rafiss
Copy link
Collaborator Author

rafiss commented Oct 26, 2021

@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.

@roji
Copy link

roji commented Oct 26, 2021

@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.

@rafiss
Copy link
Collaborator Author

rafiss commented Oct 29, 2021

@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 xmin, or slightly differing syntaxes with privileges, or not using pg_terminate_backend).

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:

  • default_int_size = 4
  • serial_normalization = sql_sequence_cached
  • experimental_enable_temp_tables = true

@roji
Copy link

roji commented Oct 29, 2021

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.

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. [SkipOnCockroachDB] and/or [MinimumCockroachDBVersion].

Re stuff like xmin, IIRC the provider only uses it if the user uses a specific API (i.e. via UseXminAsConcurrencyToken); so I'd say you don't necessarily have to do anything here - the feature is simply unsupported on CockroachDB, and a user trying to use it would simply get an error.

@rafiss
Copy link
Collaborator Author

rafiss commented Nov 3, 2021

@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.

@roji
Copy link

roji commented Nov 3, 2021

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.

@rafiss rafiss added this to Triage in SQL Sessions - Deprecated via automation Dec 8, 2021
@rafiss rafiss moved this from Triage to December 2021 in SQL Sessions - Deprecated Dec 8, 2021
@rafiss rafiss changed the title Determine how to distribute a provider that supports CockroachDB Add cockroachdb to EFCore CI Dec 8, 2021
@rafiss
Copy link
Collaborator Author

rafiss commented Dec 8, 2021

I've rephrased the issue title; and we hope to work on this soon.

@rafiss rafiss moved this from February 2021 to Potentially for 22.2 in SQL Sessions - Deprecated Apr 21, 2022
@rimadeodhar
Copy link

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:

  • Add CRDB to EFCore CI as non blocking step
  • Update/skip tests as applicable to get CRDB CI tests to pass.
  • Develop CockroachDB plugin if necessary to bridge any translation gaps.

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

@rimadeodhar rimadeodhar moved this from 22.2 Now to 22.2 Later in SQL Sessions - Deprecated Sep 12, 2022
@giangpham712
Copy link
Collaborator

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.

@rafiss
Copy link
Collaborator Author

rafiss commented Feb 5, 2024

This is now implemented as a provider here: https://github.com/cockroachdb/efcore.pg.cockroach

@rafiss rafiss closed this as completed Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

5 participants