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

WIP on improving compatibility of efcore.pg with CockroachDB #2892

Draft
wants to merge 77 commits into
base: main
Choose a base branch
from

Conversation

giangpham712
Copy link

Hi @roji. I have been working with Cockroach team to update efcore.pg to make it more compatible with CockroachDB. The goal is to:

  • Add cockroachDB to EFcore CI. Ensure that this test suite passes against CockroachDB 22.3.x or 22.2.x depending on what is available. Each failing test should be fixed or skipped.
  • Modify the EF Core PostgreSQL provider to support CockroachDB e.g., not using xmin, as outlined in this issue.

So far, we have managed to make the majority of the tests pass and the number of failing tests remains less than 20. Here's the draft PR with all the changes so far. While we will continue to add more changes and update the details of this PR. Could you take a look and share with us your feedback about our current approach.

@rafiss @fqazi

Update to exclude tables from crdb_internal and pg_extension which exist for CockroachDB
Skip NodaTimeQueryNpgsqlTest for CockroachDB due to the lack of support for range types
Skip tests that fail due to known issues and unsupported features
Update tests to be compatible with CockroachDB behavior
- Suppress transaction for operations that change schema
- Use CREATE SCHEMA IF NOT EXISTS because CockroachDB doesn't support IF
- Cast computed value to target column type as CockroachDB doesn't do it automatically
- Drop user-defined types at last because CockroachDB doesn't support DROP TYPE CASCADE
- Use DROP FUNCTION because CockroachDB doesn't support DROP ROUTINE
- Remove hidden column generated by CockroachDB when primary key is not defined
- Other CockroachDB differences
- Skip tests in NpgsqlDatabaseModelFactoryTest that are not compatible
Workaround for CockroachDB issue that doesn't return error when database name is invalid cockroachdb/cockroach#109992
- Add missing tests
- Skip tests that use unsupported features
@roji
Copy link
Member

roji commented Oct 6, 2023

To set expectations, I am very busy at the moment and will unfortunately likely remain busy until the .NET/EF 8.0 release mid-November; so I likely won't have time for a deep review or lots of back-and-forth conversation on this.

However, from a cursory look I can see a lot of tweaking across many of the provider services to account for various Cockroach incompatibilties and problems. It isn't ideal to integrate all this Cockroach-specific logic into the provider, both because it isn't a very scalable solution (i.e. another PG-like database may want to do the same, etc.), and because any future changes would have to be accepted into the driver as well.

So before going too far down this road, have you considered releasing an EF extension which would replace the relevant Npgsql services (e.g. NpgsqlMigrationSqlGenerator) with Cockroach-specific versions (e.g. CockroachMigrationSqlGenerator)? Of course, the Cockroach-specific versions would extend the Npgsql ones, and just override where there's a Cockroach-specific behavior quirk. It wouldn't be very different from the approach in this PR (almost all of the work would be preserved), but it would keep all the Cockroach-specific logic out of the provider, and crucially, allow Cockroach themselves to be the owner of the extension, make changes to it, etc. (which I think is a better approach than making e.g. me the owner).

I'd recommend at least giving this approach a try - if you need help with creating an extension that replaces services, let me know!

@giangpham712
Copy link
Author

Thanks @roji for your feedback. I will update the code in that direction

@giangpham712
Copy link
Author

Thanks @roji , if you could point or share with me some example, it'll be great.

@giangpham712
Copy link
Author

giangpham712 commented Oct 15, 2023

Hi @roji . Perhaps I misunderstood your suggestion at first. By EF extension, did you mean that we make a different code base that references efcore.pg and implement CockroachDB specific subclass services to modify the behaviour? With that approach, I'm wondering how to run all the efcore.pg test suite against CockroachDB

@roji
Copy link
Member

roji commented Oct 15, 2023

did you mean that we make a different code base that references efcore.pg and implement CockroachDB specific subclass services to modify the behaviour?

Yes, exactly - this would ideally be a separate nuget package which users users add in addition to Npgsql.EntityFrameworkCore.PostgreSQL, and which is developed in a separate, Cockroach-owned repository. This way Cockroach can fully maintain it, release versions, etc. without needing anything from Npgsql itself.

With that approach, I'm wondering how to run all the efcore.pg test suite against CockroachDB

That's a good question... I'd be happy to start publishing the EFCore.PG test suite as a nuget package, similar to how EF itself publishes the Microsoft.EntityFrameworkCore.Relational.Specification.Tests as a nuget for providers. You would be able to reference that nuget and run the tests in your separate build against CockroachDB - how does that sound?

@giangpham712
Copy link
Author

@roji That sounds good. Just want to make sure that I will be able to override or skip specific tests that are not compatible.

@giangpham712
Copy link
Author

Hi @roji , I know that you're very busy at the moment but could you give an ETA for that test suite package. Thanks

@roji
Copy link
Member

roji commented Oct 16, 2023

@giangpham712 it will probably take 2-3 weeks as I'm busy with 8.0 stabilization (please ping me here again if it doesn't happen by then). But I suggest that for now you just reference the Npgsql function test project directly - via a <ProjectReference>, so that you can continue working. When the nupkg comes out, you'll be able to just replace that.

@giangpham712
Copy link
Author

Hi @roji , sorry for bothering you again. If I just reference the test project (e.g. EFCore.PG.FunctionalTests), is there a way to run all the tests from that package from my test project without having to create subclasses for all the test classes?

@giangpham712
Copy link
Author

giangpham712 commented Oct 17, 2023

I guess that's not possible and my test project will have to override all the expected test classes. If that's the case, will it be simpler for me to just copy all the tests from efcore.pg into my extension library code base @roji

@roji
Copy link
Member

roji commented Oct 17, 2023

You can include the test source code via e.g. a git submodule or some similar mechanism. However, you'll likely also need to tweak NpgsqlTestStore - as well as skip/tweak some specific tests - for CockroachDB-specific things. So I'm not sure there's an alternative to overriding the test classes from the Npgsql functional test suite. On the other hand that shouldn't be too much of a big deal, I think...

If that's the case, will it be simpler for me to just copy all the tests from efcore.pg into my extension library code base

Of course you can do that, but then you'll have to also keep them up to date as changes occur in Npgsql's test code. Simply extending Npgsql's tests allows you to keep in sync more easily.

@giangpham712
Copy link
Author

Hi @roji , I hope you're doing well. Just curious when you will have some time to publish the efcore pg test suite as a package. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants