-
Notifications
You must be signed in to change notification settings - Fork 56
add rules to ban domains with constraints #418
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
Conversation
👷 Deploy request for squawkhq pending review.Visit the deploys page to approve it
|
| let mut errs = vec![]; | ||
| for raw_stmt in tree { | ||
| match &raw_stmt.stmt { | ||
| Stmt::AlterDomainStmt(stmt) if stmt.subtype == "C" => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i couldn't find direct documentation, or even a clear place to cite in the libpg_query code, to demonstrate that subtype == "C" means it's the add constraint variation of alter domain. but, i have every other form in the "shouldn't report errors" test (ban_alter_domain_without_add_constraint_is_ok) to validate it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the api is pretty wonky at the moment, I'm working on a new parser that should make this easier when it's done!
preview of it at (using wasm):
|
i think the failures in the python jobs are unrelated to these changes and instead related to sccache usage. the final error reported is but it does compile with both |
|
|
||
| ## problem | ||
|
|
||
| Postgres [domains][], which associate a data type with an optional check constraint, have poor support for online migrations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both of these docs are very thorough, thank you!
sbdchd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL domains are a thing! These rules are both great, thanks!
Ban creating domains with constraints (
CREATE DOMAIN ... CHECK) or adding constraints to existing domains (ALTER DOMAIN ... ADD CONSTRAINT).Domains with constraints are hard to use/operationalize well with online migrations, have performance problems, and are soft-discourage by Postgres developers as having unfavorable tradeoffs.