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 support for no_inherit on column level constraints and in create_constraint operation #670

Merged
merged 4 commits into from
Feb 14, 2025

Conversation

kvch
Copy link
Contributor

@kvch kvch commented Feb 10, 2025

This PR adds a new option no_inherit to inline and table level check constraints in create_constraint. It also makes sure that the check is duplicated correctly during migration. Also, I added the new attribute noInherit to the output of analyze/read_schema.

@kvch kvch force-pushed the feature-unify-check-constraint-options branch from 14e8f7f to c7eacb4 Compare February 10, 2025 12:02
@kvch kvch force-pushed the feature-unify-check-constraint-options branch 6 times, most recently from 18faa94 to 845f661 Compare February 10, 2025 13:46
@kvch kvch force-pushed the feature-unify-check-constraint-options branch 3 times, most recently from 4d25638 to 9c405b7 Compare February 10, 2025 14:22
@kvch kvch marked this pull request as ready for review February 10, 2025 14:26
@kvch kvch requested a review from andrew-farries February 10, 2025 14:26
Copy link
Collaborator

@andrew-farries andrew-farries left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look OK, but I think the PR needs to be rebased to pick up relevant changes from main, eg #665.

Table inheritance feels like a very rarely used Postgres feature; did this work arise from seeing it used somwhere like an ORM migration, or for completeness to cover all constraint options? In general, I think we need to be guided more by the former and add features only when we see them in use.

docs/operations/create_constraint.mdx Show resolved Hide resolved
pkg/migrations/duplicate_test.go Show resolved Hide resolved
pkg/migrations/op_create_table.go Outdated Show resolved Hide resolved
pkg/migrations/op_create_table.go Outdated Show resolved Hide resolved
@kvch
Copy link
Contributor Author

kvch commented Feb 11, 2025

I haven't seen this option in any ORM output yet. I added it for the sake of completeness.
If we are guided by ORM support, then it does not make sense to put more effort into this PR. Do you still want me to finish it and merge it?

@kvch
Copy link
Contributor Author

kvch commented Feb 11, 2025

I won't close this PR, it comes with some nice cleanups.

@kvch kvch force-pushed the feature-unify-check-constraint-options branch from 9c405b7 to 6f258f6 Compare February 11, 2025 11:28
@kvch kvch requested a review from andrew-farries February 11, 2025 11:48
Copy link
Collaborator

@andrew-farries andrew-farries left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍

I think there are changes to sql2pgroll needed too? For example this is supported now right?

"ALTER TABLE foo ADD CONSTRAINT bar CHECK (age > 0) NO INHERIT",

@kvch kvch merged commit ab567b6 into xataio:main Feb 14, 2025
28 checks passed
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