planner: name an unnamed CHECK or FOREIGN KEY so its online sequence can be built

[aparajon]

Why

The online form of ADD CHECK and ADD FOREIGN KEY is ADD CONSTRAINT ... NOT VALID followed by VALIDATE CONSTRAINT <name>, and the second step has to name the constraint. When the statement named none, pg-sprite refused it as rewrite-required and told the author to name it themselves. That turned the most common way to write a check constraint into a manual rewrite, even though PostgreSQL's choice of name is deterministic.

What

statement.AddNotValid now gives an unnamed CHECK or FOREIGN KEY the name PostgreSQL would choose, and the rewrite states it:

• <table>_<columns>_fkey for a foreign key.
• <table>_<column>_check for a check that references exactly one distinct column.
• <table>_check for any other check.

Names are truncated the way the server's makeObjectName truncates, reusing the helper ImplicitRelationNames already mirrors. The server moves past its first choice when a constraint anywhere in the schema already holds that name, numbering the label (check1, fkey1, ...). A dry-run and a run read the schema's constraint names and the table's columns, and number the name the same way. A bare reference to the table's own name reads as its whole row unless the table has a column by that name. A dry-run reads an unqualified table where the session search_path resolves it, as a run does. Suggest and lint read no live table, so they state the first choice, numbered past the names the script's own earlier statements claim. A field selected from the whole row, (t).qty, counts as that column, and ROW(t.*) as every column, as the server's analysis resolves them.

Example: ALTER TABLE orders ADD CHECK (qty > 0) on a live orders table.

Before                                  After

┌──────────────────────────────┐        ┌──────────────────────────────┐
│ ADD CHECK (qty > 0)          │        │ ADD CHECK (qty > 0)          │
│ no constraint name           │        │ no constraint name           │
└──────────────┬───────────────┘        └──────────────┬───────────────┘
               │ plan                                  │ plan
               ▼                                       ▼
┌──────────────────────────────┐        ┌──────────────────────────────┐
│ rewrite-required, exit 2     │ ✗      │ 1. ADD CONSTRAINT            │
│ help: name the constraint    │        │    orders_qty_check ...      │
│ yourself, then rerun         │        │    NOT VALID                 │
└──────────────────────────────┘        │ 2. VALIDATE CONSTRAINT       │
                                        │    orders_qty_check          │
                                        └──────────────┬───────────────┘
                                                       │ run
                                                       ▼
                                        ┌──────────────────────────────┐
                                        │ executed, exit 0             │
                                        │ orders_qty_check validated   │
                                        └──────────────────────────────┘

A server-oracle test adds each unnamed constraint with plain DDL and asserts that AddNotValid picks the name PostgreSQL chose. The cases are:

• checks on one column, a repeated column, two columns, no column, a column inside a function call, and a table-qualified column;
• quoted mixed-case names;
• truncation, including truncation inside a multibyte character;
• foreign keys on one column, two columns, and a column list long enough to truncate;
• schema-qualified columns, fields of a composite column, a whole-row reference by the table's bare name, and a column named like its table;
• first choices already held by another table, by the table and a domain, a truncated first choice, and a foreign key's first choice. name-constraint-then-validate stays in the guidance vocabulary for a CHECK or FOREIGN KEY whose rewrite cannot be built, so the vocabulary and format_version are unchanged. The refuse demo tape now uses an inline UNIQUE on ADD COLUMN, which still refuses with typed help.

Before: migrate --alter 'ALTER TABLE orders ADD CHECK (qty > 0)' --dry-run

statement 1:
  ALTER TABLE orders ADD CHECK (qty > 0);

error[rewrite-required]:
  refused — blocks as written and no online replacement could be
  constructed; rewrite the change as separate online steps and dry-run
  each one

note[safer-idiom]:
  ADD CONSTRAINT — holds a blocking lock on the table for the whole
  operation — writes (and for some forms reads) wait until it finishes

help[name-constraint-then-validate]:
  name the constraint, add it NOT VALID, then VALIDATE CONSTRAINT online

docs:
  https://github.com/block/pg-sprite/blob/main/docs/postgres-online-ddl-reference.md#rewrite-required
  https://github.com/block/pg-sprite/blob/main/docs/postgres-online-ddl-reference.md#safer-idiom
  https://github.com/block/pg-sprite/blob/main/docs/suggest-report.md#name-constraint-then-validate

plan:
  public.orders (PostgreSQL 16.15) — 1 statement, 0 steps to run, 1 refused

dry-run:
  nothing was executed

After: migrate --alter 'ALTER TABLE orders ADD CHECK (qty > 0)' --dry-run

statement 1:
  ALTER TABLE orders ADD CHECK (qty > 0);

warning[safer-idiom]:
  ADD CONSTRAINT — holds a blocking lock on the table for the whole
  operation — writes (and for some forms reads) wait until it finishes

note:
  pg-sprite will run a safer online sequence instead:
  1. ALTER TABLE orders ADD CONSTRAINT orders_qty_check CHECK (qty > 0) NOT VALID;
  2. ALTER TABLE "orders" VALIDATE CONSTRAINT "orders_qty_check";

note:
  each step commits on its own — not transactionally equivalent, and the
  sequence must not run inside a transaction block

docs:
  https://github.com/block/pg-sprite/blob/main/docs/postgres-online-ddl-reference.md#safer-idiom

plan:
  public.orders (PostgreSQL 16.15) — 1 statement, 2 steps to run, 0 refused

dry-run:
  nothing was executed

apply:
  re-run without --dry-run

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

A quoted "*" column receives an incorrect constraint name, potentially causing an avoidable collision.

1 open issue

• Avoid conflating quoted "*" column with whole-row marker · New

What changed in this PR

Enables pg-sprite to construct online CHECK and FOREIGN KEY sequences without requiring users to name constraints.

Changes:

• Generates default constraint names for the NOT VALID → VALIDATE sequence.
• Adds naming and execution tests.
• Updates capability documentation, guidance, and the refusal demo.

File	Description
pkg/​suggest/​suggest.go	Updates guidance comments for unnamed constraints.
pkg/​suggest/​suggest_test.go	Expects runnable unnamed-constraint rewrites.
pkg/​statement/​rewrite.go	Generates names for unnamed constraints.
pkg/​statement/​ops_test.go	Removes the unnamed-constraint refusal case.
pkg/​statement/​constraint_name_integration_test.go	Compares generated names with PostgreSQL.
pkg/​planner/​planner_test.go	Expects two-step unnamed-constraint plans.
pkg/​plan/​plan_test.go	Removes obsolete naming-guidance expectations.
pkg/​migrate/​run_integration_test.go	Tests unnamed CHECK execution and validation.
pkg/​capabilities/​capabilities.yaml	Records naming and collision behavior.
docs/​suggest-report.md	Updates constraint guidance documentation.
docs/​demos/​refuse.tape	Uses inline UNIQUE to demonstrate refusal.
docs/​capabilities.md	Documents unnamed-constraint support.

_{Review effort: Balanced}

[aparajon]

🤖 The one open issue in the Copilot review summary (quoted "*" column conflated with the whole-row marker) is fixed in b6f584c. That thread has a reply and is resolved.

Addressed by Claude Code

[Kiran01bm]

🤖 Review findings - created by Kiran's code review agent - for pg-sprite/pull/219, 6cd72fe.

Verdict: 3 findings — 0 blocking, 1 non-blocking (an unnamed constraint can collide with an existing name), 2 suggestions (the predicted name differs from the server's in two cases).

Non-blocking

1. An unnamed CHECK or FOREIGN KEY that PostgreSQL would accept is turned into a rewrite that fails when run. pkg/statement/rewrite.go:86 always assigns PostgreSQL's first-choice name: con.Conname = defaultConstraintName(alter.GetRelation().GetRelname(), con). pkg/planner/planner.go:498 then marks it native/safer-idiom, and Facts carries no constraint names, so the planner cannot see a collision. If t already has t_qty_check, raw ADD CHECK (qty < 1000) would succeed as t_qty_check1, but dry-run and advise recommend ADD CONSTRAINT t_qty_check ... NOT VALID, which fails with 42710. Before this PR the same statement was refused at plan time with typed guidance. The failure leaves the table unchanged and is documented (rewrite.go:64, capabilities doc), but the planner already reads live table facts, so it could check the predicted name and either refuse or pick the next suffix.

General suggestions

2. For composite-field access, the name comes from the field instead of the column. pkg/statement/rewrite.go:138 takes the last field of every ColumnRef: name = last.GetString_().GetSval(). So ADD CHECK (addr.zip <> '') becomes t_zip_check, while the server resolves addr.zip to the Var addr and names it t_addr_check. NOT VALID and VALIDATE still agree because the name is explicit, but the doc comment's list of exceptions leaves this case out, and the oracle integration test has no composite case.
3. A bare whole-row reference by the table name is counted as a column. ADD CHECK (t IS NOT NULL) reaches the same line, rewrite.go:138, and gets t_t_check; the server gets a varattno-0 Var whose get_attname returns NULL, so it picks t_check. The function comment admits this, but docs/capabilities.md:221 and docs/suggest-report.md:132 still promise "the name PostgreSQL would choose first", and no test covers it. The effect is cosmetic only: the constraint gets a different name than direct DDL would give it.

The one thing that could have broken, verified

The riskiest mechanism is the predicted constraint name drifting from the one in the VALIDATE step, which would leave a NOT VALID constraint that never gets validated. That cannot happen. The rewrite sets the name explicitly before both statements are built. The planner quotes it in VALIDATE with pgx.Identifier.Sanitize, and the deparser quotes the mixed-case form in ADD CONSTRAINT. The integration oracle covers the "Orders"/"Qty" case. Every mismatch found above only changes which name gets chosen. None splits the two steps.

Verified correct

• makeObjectName (implicit.go:250) matches PostgreSQL's overhead/availchars balancing and multibyte clipping. Joining all FK columns before truncating gives the same prefix as the server's early break.
• defaultConstraintName produces <table>_check for zero columns, several distinct columns, t.* alone, and whole-row plus a column, matching pull_var_clause + list_union.
• Repeated references to one column (qty > 0 AND qty < 100, t.qty with qty) collapse to a single name, as the server's list_union does.
• A top-level ADD CHECK/FOREIGN KEY with an explicit name that already exists fails before anything is added, matching the AddNotValid comment and the capabilities doc.
• Relation and column names come from the parsed tree, which is already case-folded and truncated to NAMEDATALEN, the same identifiers the server sees.
• The only caller of AddNotValid is planner.classifyAddConstraint, gated on single. Nothing else relied on ErrNotValidNotApplicable for the unnamed form, and schemadiff/diffplan never emit unnamed ADD CHECK/FOREIGN KEY.
• CI: PostgreSQL 14, 15, 17 and 18 pass, new integration tests included. On PostgreSQL 16 every package this PR touches passes. The only failure there is TestCheckAbortsWhenTheLockIsLostMidRepair in pkg/checksum (expected 0, got 1500), a package the PR does not touch, and all-green fails only because of it. The package imports pkg/statement, so it is not fully isolated from this change, but the failing test exercises lock loss during a checksum repair rather than constraint naming. Main's CI at 48f317a0 failed the same way in the same package (TestCheckCountsNoRepairWhenTheRepairRollsBack on PostgreSQL 14: a rolled-back repair counted as done), which points to an intermittent pkg/checksum rollback issue that predates this PR. Its root cause was not investigated, so a re-run is worth doing before merge. No local builds or tests were run; this review is source inspection only.

This review was generated by Claude Code (claude-opus-5-5).

[Kiran01bm]

🤖 Approved on Kiran's (@kmuddukrishna) behalf by the scheduled review agent — no blocking findings at 6cd72fe. See the review comment above; non-blocking findings and suggestions, if any, are not merge gates.

[aparajon]

🤖 Thanks for the review. All three findings are addressed in 17061df and add994f:

1. Name collision (non-blocking): fixed. A dry-run and Run now read every constraint name in the table's schema (preflight.SchemaConstraintNames; tables and domains alike, since ChooseConstraintName checks the whole namespace). The rewrite then moves past a taken name the way the server does: check1, check2, ... and fkey1, recomputing the truncation for each label. The oracle integration test now covers four cases where the server's first choice is taken: by another table in the schema, by the table and a domain (giving t_qty_check2), a truncated long name, and a foreign key. A migrate Run case executes t_qty_check1 next to an existing t_qty_check. Suggest and lint read no live table, so they still state the first choice. docs/suggest-report.md now says so and points to a dry-run.
2. Composite field: I checked this against the server and the premise does not hold. ADD CHECK (addr.zip <> '') fails with missing FROM-clause entry for table "addr" (42P01): a two-part name is always table.column. A field of a composite column has to be written (addr).zip, which wraps a reference to addr, so taking the last name part was already right. I added oracle cases for (addr).zip and (t.addr).zip (both t_addr_check) and for schema.t.qty, and the doc comment now explains this.
3. Bare whole-row reference: fixed. When the live columns are known, a bare name equal to the table's own name, with no column by that name, reads as the whole row (t IS NOT NULL gives t_check). A column named like the table still wins (t_t_check), as the server resolves it. Both cases are in the oracle test.

On the PG16 failure: TestCheckAbortsWhenTheLockIsLostMidRepair is in pkg/checksum, which this PR does not change. The branch is now merged with main, and CI is running again.

Addressed by Claude Code

[Copilot]

🟡 Changes recommended

Expression-based naming, dry-run target resolution, and multi-statement suggestion collisions remain unresolved.

3 open issues

• Raw-tree walk misnames constraints for field selection and star expansion · New
• Dry-run resolves unqualified targets differently from execution · New
• Offline suggestions reuse constraint names within one script · New

1 resolved since last review

• Avoid conflating quoted "*" column with whole-row marker

_{Review effort: Balanced}