Skip to content

Conversation

@johnmastro
Copy link
Contributor

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.

@netlify
Copy link

netlify bot commented Apr 1, 2025

👷 Deploy request for squawkhq pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 7d840eb

let mut errs = vec![];
for raw_stmt in tree {
match &raw_stmt.stmt {
Stmt::AlterDomainStmt(stmt) if stmt.subtype == "C" => {
Copy link
Contributor Author

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

Copy link
Owner

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):

https://tea.squawkhq.com

@johnmastro
Copy link
Contributor Author

i think the failures in the python jobs are unrelated to these changes and instead related to sccache usage.

the final error reported is

💥 maturin failed
  Caused by: Cargo metadata failed. Does your crate compile with `cargo build`?
  Caused by: `cargo metadata` exited with an error: 

but it does compile with both cargo build and cargo build --release, and above that we have

error: process didn't exit successfully: `sccache /home/runner/.rustup/toolchains/1.84.0-x86_64-unknown-linux-gnu/bin/rustc -vV` (exit status: 2)
--- stderr
sccache: error: Server startup failed: cache storage failed to read: Unexpected (permanent) at read => {"$id":"1","innerException":null,"message":"This legacy service is shutting down, effective April 15, 2025. Migrate to the new service ASAP. For more information: https://gh.io/gha-cache-sunset","typeName":"Microsoft.Azure.DevOps.ArtifactCache.WebApi.ArtifactCacheServiceDecommissionedException, Microsoft.Azure.DevOps.ArtifactCache.WebApi","typeKey":"ArtifactCacheServiceDecommissionedException","errorCode":0,"eventId":3000}


## problem

Postgres [domains][], which associate a data type with an optional check constraint, have poor support for online migrations
Copy link
Owner

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 sbdchd added the automerge automerge with kodiak label Apr 2, 2025
Copy link
Owner

@sbdchd sbdchd left a 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!

@kodiakhq kodiakhq bot merged commit ca0b55b into sbdchd:master Apr 2, 2025
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge automerge with kodiak

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants