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

fix(crdb): ignore crdb_internal schema when diffing the database #5108

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kshinn
Copy link

@kshinn kshinn commented Jan 8, 2025

There has been a change in a Nov release of Cockroach DB. The crdb_internal_region enum is coming up in the diff engine regardless of having a multi-region deployment. Prisma should not scan or alter the crdb_internal schema as it should never be directly under user manipulation.

Since the cockroach connector is really just the postgres engine under the hood, perhaps the namespace scanning should be changed. This PR is rather naive in that it just force ignores the crdb_internal schema and should cause the engine to ignore the fields.

I'm happy to hear a better way to approach this fix from the team. This issue described in #25696 I believe is coming from the engine.

@kshinn kshinn requested a review from a team as a code owner January 8, 2025 19:48
@kshinn kshinn requested review from aqrln and removed request for a team January 8, 2025 19:48
@CLAassistant
Copy link

CLAassistant commented Jan 8, 2025

CLA assistant check
All committers have signed the CLA.

@aqrln aqrln added this to the 6.3.0 milestone Jan 13, 2025
@aqrln
Copy link
Member

aqrln commented Jan 13, 2025

Thanks so much for the PR @kshinn!

Could we scope this change to CockroachDB only so it doesn't apply to regular Postgres? An easy way to do this would be to create a separate namespaces_query_cockroach.sql file next to namespaces_query.sql and then change the get_namespaces method in schema-engine/sql-schema-describer/src/postgres.rs to do this:

let sql = if self.is_cockroach() {
    include_str!("postgres/namespaces_query_cockroach.sql")
} else {
    include_str!("postgres/namespaces_query.sql")
};

Could you also confirm that you have tested this change and it fixes the problem for you? Would it be feasible to add tests for this in code?

@rafiss
Copy link

rafiss commented Jan 17, 2025

Hi! CockroachDB developer here. Skipping the crdb_internal schema does make sense, but the crdb_internal schema is not the same thing as the crdb_internal_region enum. That enum is a special internal type that is used when a table is setup for multiregion access (mentioned in docs here and this example. This PR may not fix the issue described in prisma/prisma#25696.

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.

4 participants