Preserve UNIQUE NULLS NOT DISTINCT#413
Conversation
Adds a regression fixture under create_table/add_unique_constraint_nulls_not_distinct that asserts a table-level UNIQUE NULLS NOT DISTINCT constraint round-trips with the modifier intact. Today the inspector drops it (ir.Constraint has no NullsNotDistinct field), so the migration emits a plain UNIQUE (a, b) — silently changing semantics. Marked PG15+ via skipListPG14, mirroring create_index/add_index.
Adds a LEFT JOIN to pg_index in GetConstraintsForSchema and exposes indnullsnotdistinct as a new nulls_not_distinct column. The column is read defensively via to_jsonb so the query still plans on PG14, where pg_index.indnullsnotdistinct does not exist (mirrors the existing is_period pattern for PG18-only conperiod).
UNIQUE constraints carrying NULLS NOT DISTINCT (PG15+) round-tripped without the modifier because pgschema's IR.Constraint did not capture it — only IR.Index did. The dump and diff paths therefore emitted a plain UNIQUE (...), silently changing the semantics: with the modifier, NULL-bearing tuples collide, and INSERT ... ON CONFLICT (cols) flows that rely on that collision break after a round-trip. Adds NullsNotDistinct to ir.Constraint, populates it from the freshly exposed query column, and emits the modifier across the four UNIQUE DDL sites: - generateConstraintSQL: inline UNIQUE constraints inside CREATE TABLE. - ALTER TABLE ... ADD CONSTRAINT ... UNIQUE: the addition path. - ALTER TABLE ... ADD CONSTRAINT ... UNIQUE: the modify path (drop + re-add). - inline single-column UNIQUE on ADD COLUMN. constraintsEqual now compares NullsNotDistinct so a flipped modifier on an existing constraint correctly triggers a drop + re-add rather than silently no-op'ing. PG14 stays a no-op: pg_index.indnullsnotdistinct does not exist there, the to_jsonb read returns NULL, and COALESCE collapses it to false. Closes the regression pinned by the create_table/ add_unique_constraint_nulls_not_distinct fixture.
Greptile SummaryThis PR fixes a silent data-loss bug where Confidence Score: 4/5Safe to merge; fix is correct and complete across all code paths, with only a missing dump-level regression test. All changed paths are consistent and the PG14 compatibility approach (to_jsonb trick) is proven in this codebase. Only P2 finding: no testdata/dump test covering the original bug report scenario. No files require special attention; the only gap is a missing dump test. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[pg_constraint c] -->|LEFT JOIN on conindid| B[pg_index i]
B -->|to_jsonb trick| C{indnullsnotdistinct exists?}
C -->|PG15+ yes| D[true/false]
C -->|PG14 no / non-index constraint| E[COALESCE → false]
D --> F[NullsNotDistinct sql.NullBool]
E --> F
F --> G[inspector.go Constraint.NullsNotDistinct]
G --> H{SQL generation path}
H -->|CREATE TABLE inline| I[generateConstraintSQL CONSTRAINT x UNIQUE NULLS NOT DISTINCT]
H -->|ALTER TABLE ADD COLUMN inline| J[table.go L777 CONSTRAINT x UNIQUE NULLS NOT DISTINCT]
H -->|ALTER TABLE ADD CONSTRAINT| K[table.go L887 and L1019 ADD CONSTRAINT x UNIQUE NULLS NOT DISTINCT]
G --> L[constraintsEqual detects modifier change → re-create]
Reviews (1): Last reviewed commit: "fix: preserve UNIQUE NULLS NOT DISTINCT ..." | Re-trigger Greptile |
| @@ -0,0 +1,11 @@ | |||
| -- Regression for: UNIQUE NULLS NOT DISTINCT modifier dropped from constraints | |||
There was a problem hiding this comment.
Missing dump test for NULLS NOT DISTINCT
The PR description says pgschema dump was the primary regression surface, yet no dump test (testdata/dump/) is added to verify that the dump command now emits NULLS NOT DISTINCT correctly when inspecting a live database. The diff test covers the planning path, but a dump test (raw.sql + pgschema.sql pair) would directly confirm the inspector → formatter round-trip and prevent regressions on the original reported bug.
The diff fixture exercises inspector → IR → DDL emission, which is the same code path the dump command takes; this fixture closes the loop on the original bug report (pgplex#412) by asserting a CREATE TABLE round-trip through the dump command itself. Gated to PG15+ via skipListPG14 (NULLS NOT DISTINCT is PG15+). Verified on PG14 (skip), PG15, PG16, PG17, PG18 (pass).
There was a problem hiding this comment.
Pull request overview
This PR fixes pgschema dump and migration diffs so they correctly preserve PostgreSQL 15+ UNIQUE ... NULLS NOT DISTINCT table constraints instead of silently emitting a plain UNIQUE (...) and changing semantics.
Changes:
- Extend the constraints inspector query to read
pg_index.indnullsnotdistinctin a PG14-safe way (to_jsonb+COALESCE). - Add
nulls_not_distinctto the IR constraint model and propagate it through the inspector. - Update SQL generation and diff equality so
UNIQUE NULLS NOT DISTINCTis emitted and detected as a meaningful change; add regression fixtures + integration test.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
ir/queries/queries.sql |
Adds nulls_not_distinct extraction via pg_index (PG14-safe) to the constraints query. |
ir/queries/queries.sql.go |
Regenerates query bindings and row struct to include nulls_not_distinct. |
ir/ir.go |
Adds NullsNotDistinct to the IR Constraint model. |
ir/inspector.go |
Populates Constraint.NullsNotDistinct from the query result. |
internal/diff/constraint.go |
Emits UNIQUE NULLS NOT DISTINCT for inline constraints and compares the flag in constraintsEqual. |
internal/diff/table.go |
Emits NULLS NOT DISTINCT in ALTER/inline constraint SQL generation paths. |
cmd/dump/dump_integration_test.go |
Adds integration coverage for issue #412. |
testutil/skip_list.go |
Skips the new PG15+ testcases on PG14. |
testdata/dump/issue_412_unique_nulls_not_distinct/* |
Adds dump regression test fixtures for issue #412. |
testdata/diff/create_table/add_unique_constraint_nulls_not_distinct/* |
Adds diff regression fixtures ensuring migrations preserve the modifier. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tianzhou
left a comment
There was a problem hiding this comment.
LGTM.
Thanks for the contribution
pgschema dumpwas silently droppingUNIQUEtable constraints withNULLS NOT DISTINCT(something added by PG15+) by just emitting a plainUNIQUE (...)without preserving theNULLS NOT DISTINCTmodifier.E.g. given the following input
It'd produce the following (incorrect) output:
This PR adds support for
NULLS NOT DISTINCTso that the above produces the expected correct output inclusive ofNULLS NOT DISTINCT.As this is only supported by >=PG15,
GetConstraintsForSchemahas to use theto_jsonbtrick like foris_periodto work around the fact thatpg_index.indnullsnotdistinctdoesn't exist on older versions of postgres, so the read returnsNULLin that case andCOALESCEcollapses it to false.Fixes #412