Skip to content

Preserve UNIQUE NULLS NOT DISTINCT#413

Merged
tianzhou merged 4 commits intopgplex:mainfrom
ivgiuliani:fix/unique-nulls-not-distinct-dropped
Apr 29, 2026
Merged

Preserve UNIQUE NULLS NOT DISTINCT#413
tianzhou merged 4 commits intopgplex:mainfrom
ivgiuliani:fix/unique-nulls-not-distinct-dropped

Conversation

@ivgiuliani
Copy link
Copy Markdown
Contributor

@ivgiuliani ivgiuliani commented Apr 27, 2026

pgschema dump was silently dropping UNIQUE table constraints with NULLS NOT DISTINCT (something added by PG15+) by just emitting a plain UNIQUE (...) without preserving the NULLS NOT DISTINCT modifier.

E.g. given the following input

CREATE TABLE t (a int, b int, CONSTRAINT t_uniq UNIQUE NULLS NOT DISTINCT (a, b));

It'd produce the following (incorrect) output:

CREATE TABLE IF NOT EXISTS t (a int, b int, CONSTRAINT t_uniq UNIQUE (a, b));

This PR adds support for NULLS NOT DISTINCT so that the above produces the expected correct output inclusive of NULLS NOT DISTINCT.

As this is only supported by >=PG15, GetConstraintsForSchema has to use the to_jsonb trick like for is_period to work around the fact that pg_index.indnullsnotdistinct doesn't exist on older versions of postgres, so the read returns NULL in that case and COALESCE collapses it to false.

Fixes #412

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-apps
Copy link
Copy Markdown

greptile-apps Bot commented Apr 27, 2026

Greptile Summary

This PR fixes a silent data-loss bug where pgschema dump and plan dropped the NULLS NOT DISTINCT modifier from UNIQUE constraints (PG15+), causing the tool to emit semantically incorrect DDL. The fix threads a new NullsNotDistinct field through the IR, all three SQL-generation sites in table.go, and constraintsEqual, using the same to_jsonb PG14-compatibility trick already in place for conperiod.

Confidence Score: 4/5

Safe 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

Filename Overview
ir/queries/queries.sql Adds LEFT JOIN to pg_index and uses the to_jsonb trick to read indnullsnotdistinct safely on PG14; pattern is consistent with the existing is_period column.
ir/queries/queries.sql.go Generated file kept in sync with queries.sql; adds NullsNotDistinct sql.NullBool field and scan target correctly.
ir/inspector.go Correctly reads NullsNotDistinct.Bool from the first row of each grouped constraint; consistent with IsTemporal handling.
ir/ir.go Adds NullsNotDistinct bool field with correct JSON tag (omitempty) to the Constraint struct.
internal/diff/constraint.go generateConstraintSQL now emits NULLS NOT DISTINCT modifier; constraintsEqual updated to detect modifier changes and trigger re-creation.
internal/diff/table.go All three UNIQUE SQL generation paths (inline ADD COLUMN constraint, ADD CONSTRAINT for new table, ADD CONSTRAINT for existing table) updated consistently.
testutil/skip_list.go New test correctly placed in skipListPG14 (NULLS NOT DISTINCT is PG15+), not in skipListPG14_17.

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]
Loading

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
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 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).
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

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.indnullsnotdistinct in a PG14-safe way (to_jsonb + COALESCE).
  • Add nulls_not_distinct to the IR constraint model and propagate it through the inspector.
  • Update SQL generation and diff equality so UNIQUE NULLS NOT DISTINCT is 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.

Copy link
Copy Markdown
Contributor

@tianzhou tianzhou left a comment

Choose a reason for hiding this comment

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

LGTM.

Thanks for the contribution

@tianzhou tianzhou merged commit cdac18a into pgplex:main Apr 29, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

pgschema dump drops NULLS NOT DISTINCT from UNIQUE table constraints

3 participants