diff --git a/docs/capabilities.md b/docs/capabilities.md
index de26aa5a..0bf64db3 100644
--- a/docs/capabilities.md
+++ b/docs/capabilities.md
@@ -224,7 +224,7 @@ or the only online form is a direct statement the operator runs).
| Operation | Status | Engine path | Online-safety problem? | Behavior and why |
| --- | --- | --- | --- | --- |
| `ADD PRIMARY KEY` / `ADD UNIQUE` (plain key columns) | ✅ | native, safer sequence | Yes | Rewritten to the online sequence: `CREATE UNIQUE INDEX CONCURRENTLY` → `ADD CONSTRAINT ... USING INDEX`. The sequence carries the key's `INCLUDE` columns, `NULLS NOT DISTINCT`, `WITH (...)` storage parameters, and index tablespace onto the index, and `DEFERRABLE` / `INITIALLY DEFERRED` onto the attach. A storage parameter the sequence cannot render faithfully (a value that is not a plain word, number, or string, a value containing a backslash, or a namespaced parameter name) gets no rewrite and is refused as `rewrite-required` with the `unique-index-then-constraint` guidance: build the index with it, then attach it. In a desired file, a new key whose name and index match a valid live unique index takes that index over with `ADD CONSTRAINT ... USING INDEX` at the key's declared deferrability, instead of a destructive drop and rebuild. Execution reads the index again under the table's lock and refuses with the permanent `adopted-index-drift` code, attaching nothing, if it was dropped, replaced, or left invalid since planning |
-| `ADD CHECK` / `ADD FOREIGN KEY` (imperative) | ✅ | native, safer sequence | Yes | Rewritten to the online sequence: `ADD CONSTRAINT ... NOT VALID` (brief metadata lock) → `VALIDATE CONSTRAINT` (writes keep flowing during the scan) |
+| `ADD CHECK` / `ADD FOREIGN KEY` (imperative) | ✅ | native, safer sequence | Yes | Rewritten to the online sequence: `ADD CONSTRAINT ... NOT VALID` (brief metadata lock) → `VALIDATE CONSTRAINT` (writes keep flowing during the scan). An unnamed constraint is given the name PostgreSQL would choose (`
__check`, `_check`, `__fkey`, numbered `check1`, `fkey1`, ... past a name another constraint in the schema already holds), because the `VALIDATE` step has to name it |
| `ADD CONSTRAINT ... NOT VALID` / `... USING INDEX` / `VALIDATE CONSTRAINT` | ✅ | native, as-is | Yes | Already the online idiom; executed as-is |
| `ADD FOREIGN KEY ... NOT VALID` on a **partitioned parent** | ✅ | native, safer sequence | Yes — server version 18 or later | Supported on version 18 and later; refused on 14–17 with an environmental class |
| `EXCLUDE` constraints (and unrecognized constraint forms) | ❌ | — | Yes — unsolvable today | No online pattern exists in PostgreSQL — the build scans under `ACCESS EXCLUSIVE` with no `NOT VALID`/`USING INDEX` equivalent. Refused; revisit only if PostgreSQL grows one |
diff --git a/docs/demos/diff-greenfield.tape b/docs/demos/diff-greenfield.tape
index 2a7555a0..f1ddc893 100644
--- a/docs/demos/diff-greenfield.tape
+++ b/docs/demos/diff-greenfield.tape
@@ -13,6 +13,9 @@ Set Padding 16
Set TypingSpeed 30ms
Set Theme "Dracula"
+# The seed names public explicitly: the compose role is named pgsprite, like
+# the engine's own schema, so an unqualified name would land there once a
+# copy-swap run has created it.
Hide
Type `export PATH="$(git rev-parse --show-toplevel)/bin:$PATH"` Enter
# The rendering machine's environment leaks into the recorded shell: strip
@@ -20,7 +23,7 @@ Type `export PATH="$(git rev-parse --show-toplevel)/bin:$PATH"` Enter
# terminal and the GIF shows the diagnostic colors deterministically.
Type "unset NO_COLOR && export TERM=xterm-256color" Enter
Type "export PGSPRITE_URL='postgres://pgsprite:pgsprite@localhost:5432/pgsprite?sslmode=disable'" Enter
-Type "docker exec compose-postgres-1 psql -U pgsprite -d pgsprite -c 'DROP TABLE IF EXISTS widgets' >/dev/null" Enter
+Type "docker exec compose-postgres-1 psql -U pgsprite -d pgsprite -c 'DROP TABLE IF EXISTS public.widgets' >/dev/null" Enter
Type `demo_dir="$(mktemp -d)" && cd "$demo_dir"` Enter
Type "echo 'CREATE TABLE widgets (id bigint PRIMARY KEY, name text NOT NULL, price numeric);' > widgets.sql" Enter
# Prompt: directory cyan, branch magenta; typed input bright white. The DEBUG
diff --git a/docs/demos/improve.tape b/docs/demos/improve.tape
index 2543847c..90163c59 100644
--- a/docs/demos/improve.tape
+++ b/docs/demos/improve.tape
@@ -15,6 +15,9 @@ Set Padding 16
Set TypingSpeed 30ms
Set Theme "Dracula"
+# The seed names public explicitly: the compose role is named pgsprite, like
+# the engine's own schema, so an unqualified name would land there once a
+# copy-swap run has created it.
Hide
Type `export PATH="$(git rev-parse --show-toplevel)/bin:$PATH"` Enter
# The rendering machine's environment leaks into the recorded shell: strip
@@ -22,7 +25,7 @@ Type `export PATH="$(git rev-parse --show-toplevel)/bin:$PATH"` Enter
# terminal and the GIF shows the diagnostic colors deterministically.
Type "unset NO_COLOR && export TERM=xterm-256color" Enter
Type "export PGSPRITE_URL='postgres://pgsprite:pgsprite@localhost:5432/pgsprite?sslmode=disable'" Enter
-Type `docker exec compose-postgres-1 psql -U pgsprite -d pgsprite -c 'DROP TABLE IF EXISTS users' -c 'CREATE TABLE users (id bigint PRIMARY KEY, email text)' -c "INSERT INTO users SELECT g, 'u'||g||'@x.com' FROM generate_series(1,1000) g" >/dev/null` Enter
+Type `docker exec compose-postgres-1 psql -U pgsprite -d pgsprite -c 'DROP TABLE IF EXISTS public.users' -c 'CREATE TABLE public.users (id bigint PRIMARY KEY, email text)' -c "INSERT INTO public.users SELECT g, 'u'||g||'@x.com' FROM generate_series(1,1000) g" >/dev/null` Enter
Type `demo_dir="$(mktemp -d)" && cd "$demo_dir"` Enter
Type "echo 'CREATE TABLE users (' > users.sql" Enter
Type "echo ' id bigint PRIMARY KEY,' >> users.sql" Enter
diff --git a/docs/demos/refuse.gif b/docs/demos/refuse.gif
index 8f941396..dba907fc 100644
Binary files a/docs/demos/refuse.gif and b/docs/demos/refuse.gif differ
diff --git a/docs/demos/refuse.tape b/docs/demos/refuse.tape
index ba696eea..72dcbb05 100644
--- a/docs/demos/refuse.tape
+++ b/docs/demos/refuse.tape
@@ -1,5 +1,5 @@
# Refuse with typed help and the CI exit-code contract:
-# error[rewrite-required] + note[safer-idiom] + help[name-constraint-then-validate],
+# error[rewrite-required] + note[safer-idiom] + help[add-column-then-constraint],
# doc anchors, and echo $? showing 2.
# Render from this directory: vhs refuse.tape (or `make demos` at the repo root).
# Needs: make build && make db-up first. Hidden setup reseeds the users table.
@@ -13,6 +13,9 @@ Set Padding 16
Set TypingSpeed 30ms
Set Theme "Dracula"
+# The seed names public explicitly: the compose role is named pgsprite, like
+# the engine's own schema, so an unqualified name would land there once a
+# copy-swap run has created it.
Hide
Type `export PATH="$(git rev-parse --show-toplevel)/bin:$PATH"` Enter
# The rendering machine's environment leaks into the recorded shell: strip
@@ -20,7 +23,7 @@ Type `export PATH="$(git rev-parse --show-toplevel)/bin:$PATH"` Enter
# terminal and the GIF shows the diagnostic colors deterministically.
Type "unset NO_COLOR && export TERM=xterm-256color" Enter
Type "export PGSPRITE_URL='postgres://pgsprite:pgsprite@localhost:5432/pgsprite?sslmode=disable'" Enter
-Type `docker exec compose-postgres-1 psql -U pgsprite -d pgsprite -c 'DROP TABLE IF EXISTS users' -c 'CREATE TABLE users (id bigint PRIMARY KEY, email text)' -c "INSERT INTO users SELECT g, 'u'||g||'@x.com' FROM generate_series(1,1000) g" >/dev/null` Enter
+Type `docker exec compose-postgres-1 psql -U pgsprite -d pgsprite -c 'DROP TABLE IF EXISTS public.users' -c 'CREATE TABLE public.users (id bigint PRIMARY KEY, email text)' -c "INSERT INTO public.users SELECT g, 'u'||g||'@x.com' FROM generate_series(1,1000) g" >/dev/null` Enter
# Prompt: directory cyan, branch magenta; typed input bright white. The DEBUG
# trap resets the color before each command runs, so output renders in the
# tool's own colors (the diagnostic labels) rather than the input style.
@@ -30,7 +33,7 @@ Type "clear" Enter
Sleep 1.5s
Show
-Type "pg-sprite migrate --alter 'ALTER TABLE users ADD CHECK (id > 0)' --dry-run"
+Type "pg-sprite migrate --alter 'ALTER TABLE users ADD COLUMN code text UNIQUE' --dry-run"
Sleep 500ms
Enter
Sleep 8s
diff --git a/docs/plan-report.md b/docs/plan-report.md
index 21fca9fe..c335ec8e 100644
--- a/docs/plan-report.md
+++ b/docs/plan-report.md
@@ -48,7 +48,7 @@ consumer rendering either into a shared surface must clamp and escape them.
|---|---|---|---|
| `format_version` | int | always | Contract version; reject unknown versions. |
| `source` | string | always | Front door that derived the plan (see Sources). |
-| `schema` | string | when resolved | Target schema. The **resolved** name the engine planned against — an unqualified alter reports the schema the engine introspected (`public`), never an empty echo of the submitted text. Absent only when the statement has no single table target. |
+| `schema` | string | when resolved | Target schema. The **resolved** name the engine planned against — an unqualified alter reports the schema the session `search_path` resolves its table to, the one execution acts on (`public` when the `search_path` shows no such table), never an empty echo of the submitted text. Absent only when the statement has no single table target. |
| `table` | string | when targeted | Target table; absent for statements with no single table target (index maintenance). |
| `server_version` | string | when connected | The PostgreSQL `server_version` the plan was derived against. Classification is version-sensitive; a stored or forwarded report names the server whose rules produced it. |
| `table_exists` | bool | when introspected | Whether the live table was found. Set by both sources that introspect the target (diff, and the alter dry run); absent means the plan has no single table target to introspect. For diff, `false` means the plan is the full desired schema; for an alter dry run, `false` means the plan was classified from zero facts, executing it would fail, and the dry run exits with the refusal code. |
diff --git a/docs/suggest-report.md b/docs/suggest-report.md
index 48a26a56..099920fd 100644
--- a/docs/suggest-report.md
+++ b/docs/suggest-report.md
@@ -102,10 +102,8 @@ and advise again.
**Emitted for:** `ADD COLUMN` with an inline UNIQUE / PRIMARY KEY / FOREIGN KEY / CHECK.
The inline constraint builds or validates under the ADD COLUMN's ACCESS EXCLUSIVE lock.
-Add the plain column first, then build the constraint as a separate, **named**
-`ADD CONSTRAINT` with its online pattern — named, because the unnamed `ADD CHECK` /
-`ADD FOREIGN KEY` forms are themselves refused
-([`name-constraint-then-validate`](#name-constraint-then-validate)).
+Add the plain column first, then build the constraint as a separate `ADD CONSTRAINT`
+with its online pattern.
### `pre-add-validated-check`
@@ -126,12 +124,18 @@ then the NOT NULL constraint is a catalog flip — the same scaffold sequence th
### `name-constraint-then-validate`
-**Emitted for:** unnamed `ADD CHECK` / `ADD FOREIGN KEY`.
-
-The online sequence's `VALIDATE CONSTRAINT` step needs the constraint's name, and the
-server assigns one only at creation — so no rewrite can be constructed from the statement
-alone. Name the constraint, add it `NOT VALID`, then `VALIDATE` it online (the same
-sequence the named form gets constructed).
+**Emitted for:** `ADD CHECK` / `ADD FOREIGN KEY` whose `NOT VALID` rewrite could not be
+constructed from the statement.
+
+Name the constraint, add it `NOT VALID`, then `VALIDATE` it online: the same sequence the
+constructed rewrite emits. A statement that names no constraint gets that rewrite too, under
+the name PostgreSQL would choose first (`__check`, `_check`,
+`__fkey`), because the `VALIDATE` step has to name it. A name an earlier
+statement of the same script adds is numbered past (`__check1`), as the
+server would number it. Suggest reads no live table, so it cannot see a name the schema
+already uses, nor tell a bare reference to the table's own name, which
+reads as its whole row unless the table has a column by that name. A dry-run against the
+target reads both and names the constraint as the server would.
### `unique-index-then-constraint`
diff --git a/internal/cli/dryrun.go b/internal/cli/dryrun.go
index 7140e101..d4b6d258 100644
--- a/internal/cli/dryrun.go
+++ b/internal/cli/dryrun.go
@@ -60,7 +60,7 @@ func (c *MigrateCmd) runDryRun(ctx context.Context, out io.Writer) error {
"route", string(classified.Route), "disposition", string(routed.Disposition))
report := plan.NewReport(plan.SourceAlter)
- report.Schema = migrate.ResolvedSchema(st)
+ report.Schema = facts.Schema
report.Table = st.Table()
report.TableExists = facts.TableExists
if report.ServerVersion, err = dbconn.ServerVersion(ctx, pool); err != nil {
diff --git a/internal/cli/dryrun_integration_test.go b/internal/cli/dryrun_integration_test.go
index e975e116..c9e6bdba 100644
--- a/internal/cli/dryrun_integration_test.go
+++ b/internal/cli/dryrun_integration_test.go
@@ -3,6 +3,7 @@ package cli
import (
"encoding/json"
"fmt"
+ neturl "net/url"
"strings"
"testing"
@@ -216,8 +217,9 @@ func TestDryRunAndDiffAgreeOnDestructive(t *testing.T) {
}
// An unqualified statement resolves to the schema the engine actually
-// introspected — public — and the report says so: a stored plan must not
-// depend on the reader's search_path to name its target. The report also
+// introspected (public, on the default search_path) and the report says
+// so: a stored plan must not depend on the reader's search_path to name
+// its target. The report also
// stamps the server version its classification was derived against.
func TestDryRunResolvesUnqualifiedSchemaAndStampsServerVersion(t *testing.T) {
url := testutil.StartPostgres(t)
@@ -241,6 +243,47 @@ func TestDryRunResolvesUnqualifiedSchemaAndStampsServerVersion(t *testing.T) {
"resolving to public must feed the live facts to the classifier")
}
+// A dry-run reads an unqualified table where the session search_path
+// resolves it, as execution does, even when public holds a table of the
+// same name. The schema it resolves to already holds the server's first
+// choice for an unnamed CHECK, so the plan names the constraint as the
+// server would there, and execution runs exactly the planned SQL.
+func TestMigrateDryRunResolvesTheTableThroughSearchPath(t *testing.T) {
+ url := testutil.StartPostgres(t)
+ pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: url})
+ require.NoError(t, err)
+ defer pool.Close()
+ table := testutil.NewPublicTable(t, pool, "(qty int)")
+ schema := testutil.NewSchema(t, pool)
+ _, err = pool.Exec(t.Context(), fmt.Sprintf(
+ "CREATE TABLE %s.%s (qty int, CONSTRAINT %s_qty_check CHECK (qty < 1000))", schema, table, table))
+ require.NoError(t, err)
+
+ u, err := neturl.Parse(url)
+ require.NoError(t, err)
+ q := u.Query()
+ q.Set("options", "-csearch_path="+schema+",public")
+ u.RawQuery = q.Encode()
+ alter := fmt.Sprintf("ALTER TABLE %s ADD CHECK (qty > 0)", table)
+
+ report := dryRunPlan(t, u.String(), alter)
+ assert.Equal(t, schema, report.Schema, "the plan names the schema the search_path resolves the table to")
+ require.Len(t, report.Statements, 1)
+ planned := report.Statements[0].ExecSQL
+ require.Len(t, planned, 2)
+ assert.Contains(t, planned[0], table+"_qty_check1", "the plan numbers past the name the resolved schema holds")
+
+ cmd := newMigrateCmd(u.String(), alter)
+ cmd.JSON = true
+ var out strings.Builder
+ require.NoError(t, cmd.run(t.Context(), &out))
+ var v verdict.Verdict
+ require.NoError(t, json.Unmarshal([]byte(out.String()), &v))
+ assert.Equal(t, verdict.OutcomeExecuted, v.Outcome)
+ require.Len(t, v.ExecutedSQL, 2)
+ assert.Contains(t, v.ExecutedSQL[0], table+"_qty_check1", "execution names the constraint as the plan did")
+}
+
// A dry-run against a table that does not exist classifies with zero facts:
// the unprovable type change routes conservatively instead of failing, and
// the report records the missing target.
diff --git a/pkg/capabilities/capabilities.yaml b/pkg/capabilities/capabilities.yaml
index f6185871..34bac6fd 100644
--- a/pkg/capabilities/capabilities.yaml
+++ b/pkg/capabilities/capabilities.yaml
@@ -170,7 +170,7 @@ rows:
front_doors:
migrate: supported
diff: supported
- reason_notes: "Rewritten to the online sequence: `ADD CONSTRAINT ... NOT VALID` (brief metadata lock) → `VALIDATE CONSTRAINT` (writes keep flowing during the scan)"
+ reason_notes: "Rewritten to the online sequence: `ADD CONSTRAINT ... NOT VALID` (brief metadata lock) → `VALIDATE CONSTRAINT` (writes keep flowing during the scan). An unnamed constraint is given the name PostgreSQL would choose (`__check`, `_check`, `__fkey`, numbered `check1`, `fkey1`, ... past a name another constraint in the schema already holds), because the `VALIDATE` step has to name it"
- id: add-constraint-not-valid-using-index-validate-constraint
area: "constraints"
operation: "`ADD CONSTRAINT ... NOT VALID` / `... USING INDEX` / `VALIDATE CONSTRAINT`"
diff --git a/pkg/lint/lint.go b/pkg/lint/lint.go
index 43fe2d65..b8e89af0 100644
--- a/pkg/lint/lint.go
+++ b/pkg/lint/lint.go
@@ -128,7 +128,8 @@ type Report struct {
}
// Check lints a DDL script: every statement is parsed with the PostgreSQL
-// grammar and classified with zero live facts. A parse failure is an
+// grammar and classified with zero live facts beyond the constraint names
+// the script's earlier statements claim (planner.ScriptClassifier). A parse failure is an
// error; an unsupported operation is not — it is an error-severity
// finding, so one bad statement never hides the rest of the report.
func Check(sql string) (Report, error) {
@@ -141,8 +142,9 @@ func Check(sql string) (Report, error) {
PostgresVersions: planner.RulesPostgresVersions,
Findings: []Finding{},
}
+ var classifier planner.ScriptClassifier
for i, stmt := range stmts {
- findings, err := checkStatement(i+1, stmt)
+ findings, err := checkStatement(&classifier, i+1, stmt)
if err != nil {
return Report{}, fmt.Errorf("statement %d: %w", i+1, err)
}
@@ -163,8 +165,8 @@ func Check(sql string) (Report, error) {
// route warrants one, and a destructive finding where the decision is
// marked destructive. The classifier is the single rulebook — the linter
// adds severity and presentation, never a second opinion.
-func checkStatement(index int, stmt statement.SourceStatement) ([]Finding, error) {
- classified, err := planner.Classify(stmt.SQL, planner.Facts{})
+func checkStatement(classifier *planner.ScriptClassifier, index int, stmt statement.SourceStatement) ([]Finding, error) {
+ classified, err := classifier.Classify(stmt.SQL)
if err != nil {
return nil, err
}
diff --git a/pkg/lint/lint_test.go b/pkg/lint/lint_test.go
index 24fc7cf5..14787136 100644
--- a/pkg/lint/lint_test.go
+++ b/pkg/lint/lint_test.go
@@ -230,3 +230,20 @@ func TestReportJSONCleanFindingsAreEmptyArray(t *testing.T) {
"warnings": 0
}`, string(raw))
}
+
+// Two unnamed CHECKs on one column in one script: the second finding's
+// suggestion names its constraint past the first's, as the server would.
+func TestCheckUnnamedChecksInOneScriptSuggestDistinctNames(t *testing.T) {
+ report, err := lint.Check("ALTER TABLE t ADD CHECK (qty > 0); ALTER TABLE t ADD CHECK (qty < 100);")
+ require.NoError(t, err)
+ var suggested []string
+ for _, f := range report.Findings {
+ if len(f.Suggestion) > 0 {
+ suggested = append(suggested, f.Suggestion[0])
+ }
+ }
+ assert.Equal(t, []string{
+ "ALTER TABLE t ADD CONSTRAINT t_qty_check CHECK (qty > 0) NOT VALID",
+ "ALTER TABLE t ADD CONSTRAINT t_qty_check1 CHECK (qty < 100) NOT VALID",
+ }, suggested)
+}
diff --git a/pkg/migrate/facts.go b/pkg/migrate/facts.go
index de5b8afe..3386bb89 100644
--- a/pkg/migrate/facts.go
+++ b/pkg/migrate/facts.go
@@ -6,6 +6,7 @@ import (
"fmt"
"slices"
+ "github.com/jackc/pgx/v5"
"github.com/jackc/pgx/v5/pgxpool"
"github.com/block/pg-sprite/pkg/planner"
@@ -27,6 +28,14 @@ type Facts struct {
// for plan-time partition checks; zero whenever Classifier is.
Target preflight.TargetFacts
+ // Schema is the schema the facts describe: the statement's
+ // qualification, or the schema an unqualified table resolves to on the
+ // session search_path, the one Run resolves it to. An unqualified table
+ // the search_path does not show is looked up in public, and reported
+ // missing; a statement without a single table target carries its own
+ // qualification.
+ Schema string
+
// TableExists mirrors the introspection outcome for a plan report:
// true when the table was found, false when it was looked up and
// missing, nil when the statement has no single table target to
@@ -36,21 +45,28 @@ type Facts struct {
// LiveFacts introspects the statement's target table for classifier facts,
// and for an ALTER TABLE that adds columns, has the server prove whether
-// each one rewrites the table (preflight.AddColumnRewrites).
+// each one rewrites the table (preflight.AddColumnRewrites). For one that
+// adds an unnamed CHECK or FOREIGN KEY it also reads the constraint names
+// the schema already uses, so the safer sequence names the constraint as
+// the server would.
func LiveFacts(ctx context.Context, pool *pgxpool.Pool,
st statement.Statement) (Facts, error) {
if st.Table() == "" {
- return Facts{}, nil
+ return Facts{Schema: st.Schema()}, nil
+ }
+ schema, err := factsSchema(ctx, pool, st)
+ if err != nil {
+ return Facts{}, err
}
- live, err := schemadiff.Introspect(ctx, pool, ResolvedSchema(st), st.Table())
+ live, err := schemadiff.Introspect(ctx, pool, schema, st.Table())
switch {
case errors.Is(err, schemadiff.ErrTableNotFound):
exists := false
- return Facts{TableExists: &exists}, nil
+ return Facts{Schema: schema, TableExists: &exists}, nil
case err != nil:
return Facts{}, err
}
- targetFacts, err := preflight.LookupTargetFacts(ctx, pool, ResolvedSchema(st), st.Table())
+ targetFacts, err := preflight.LookupTargetFacts(ctx, pool, schema, st.Table())
if err != nil {
return Facts{}, err
}
@@ -60,18 +76,58 @@ func LiveFacts(ctx context.Context, pool *pgxpool.Pool,
return Facts{}, err
}
if adds {
- if classifier.AddColumnProofs, err = preflight.AddColumnRewrites(ctx, pool, ResolvedSchema(st), st.Table(), st.SQL()); err != nil {
+ if classifier.AddColumnProofs, err = preflight.AddColumnRewrites(ctx, pool, schema, st.Table(), st.SQL()); err != nil {
+ return Facts{}, err
+ }
+ }
+ names, err := addsUnnamedValidatableConstraint(st)
+ if err != nil {
+ return Facts{}, err
+ }
+ if names {
+ if classifier.ConstraintNames, err = preflight.SchemaConstraintNames(ctx, pool, schema); err != nil {
return Facts{}, err
}
}
exists := true
return Facts{
+ Schema: schema,
Classifier: classifier,
Target: targetFacts,
TableExists: &exists,
}, nil
}
+// factsSchema is the schema LiveFacts reads st's table in: its
+// qualification, or where the session search_path resolves the unqualified
+// name, so a dry-run reads the table Run would act on. A name the
+// search_path does not show falls back to ResolvedSchema, where the table
+// is then reported missing.
+func factsSchema(ctx context.Context, pool *pgxpool.Pool, st statement.Statement) (string, error) {
+ if st.Schema() != "" {
+ return st.Schema(), nil
+ }
+ // INV: CO-9 — every function, operator, and relation is
+ // pg_catalog-qualified, so a planted overload on the session's
+ // search_path cannot resolve the name to another relation's schema.
+ // to_regclass itself resolves the bare name on that path, as Run's
+ // resolveTarget does.
+ const q = `
+ SELECT n.nspname
+ FROM pg_catalog.pg_class c
+ JOIN pg_catalog.pg_namespace n ON n.oid OPERATOR(pg_catalog.=) c.relnamespace
+ WHERE c.oid OPERATOR(pg_catalog.=) pg_catalog.to_regclass(pg_catalog.quote_ident($1))`
+ var schema string
+ err := pool.QueryRow(ctx, q, st.Table()).Scan(&schema)
+ if errors.Is(err, pgx.ErrNoRows) {
+ return ResolvedSchema(st), nil
+ }
+ if err != nil {
+ return "", fmt.Errorf("resolve %s against search_path: %w", st.Table(), err)
+ }
+ return schema, nil
+}
+
// ResolvedSchema is the schema the engine plans against: the statement's
// qualification, or public — the default the engine introspects — when a
// table-targeted statement leaves it unqualified. A report carries the
@@ -96,3 +152,19 @@ func addsColumn(st statement.Statement) (bool, error) {
}
return slices.ContainsFunc(ops, func(op statement.Op) bool { return op.Kind == statement.OpAddColumn }), nil
}
+
+// addsUnnamedValidatableConstraint reports whether st is an ALTER TABLE
+// adding a CHECK or FOREIGN KEY it does not name.
+func addsUnnamedValidatableConstraint(st statement.Statement) (bool, error) {
+ if st.Kind() != statement.KindAlterTable {
+ return false, nil
+ }
+ ops, err := statement.ParseOps(st.SQL())
+ if err != nil {
+ return false, fmt.Errorf("read the operations of %q: %w", st.SQL(), err)
+ }
+ return slices.ContainsFunc(ops, func(op statement.Op) bool {
+ validatable := op.Constraint == statement.ConstraintCheck || op.Constraint == statement.ConstraintForeignKey
+ return op.Kind == statement.OpAddConstraint && validatable && op.Name == ""
+ }), nil
+}
diff --git a/pkg/migrate/run_integration_test.go b/pkg/migrate/run_integration_test.go
index 9f8cc015..06b1cfe6 100644
--- a/pkg/migrate/run_integration_test.go
+++ b/pkg/migrate/run_integration_test.go
@@ -84,6 +84,56 @@ func TestRunDispatch(t *testing.T) {
assert.True(t, notNull)
})
+ // An unnamed CHECK runs as the same NOT VALID → VALIDATE sequence as a
+ // named one, under the name the server would have chosen, and ends
+ // validated.
+ t.Run("names an unnamed check and validates it online", func(t *testing.T) {
+ schema := testutil.NewSchema(t, pool)
+ _, err := pool.Exec(t.Context(), fmt.Sprintf("CREATE TABLE %s.t (id int PRIMARY KEY, qty int)", schema))
+ require.NoError(t, err)
+ _, err = pool.Exec(t.Context(), fmt.Sprintf(
+ "INSERT INTO %s.t SELECT g, g FROM generate_series(1, 100) g", schema))
+ require.NoError(t, err)
+
+ v, err := migrate.Run(t.Context(), pool,
+ parseOne(t, fmt.Sprintf("ALTER TABLE %s.t ADD CHECK (qty > 0)", schema)), runOptions())
+ require.NoError(t, err)
+ assert.Equal(t, verdict.OutcomeExecuted, v.Outcome)
+ assert.Equal(t, []string{
+ fmt.Sprintf("ALTER TABLE %s.t ADD CONSTRAINT t_qty_check CHECK (qty > 0) NOT VALID", schema),
+ fmt.Sprintf(`ALTER TABLE "%s"."t" VALIDATE CONSTRAINT "t_qty_check"`, schema),
+ }, v.ExecutedSQL)
+
+ var validated bool
+ require.NoError(t, pool.QueryRow(t.Context(),
+ `SELECT convalidated FROM pg_constraint
+ WHERE conrelid = ($1 || '.t')::regclass AND conname = 't_qty_check'`, schema).Scan(&validated))
+ assert.True(t, validated)
+ })
+
+ // When the table already holds the server's first choice, the unnamed
+ // CHECK runs under the numbered name the server would move on to, so
+ // the NOT VALID step does not collide with the existing constraint.
+ t.Run("names an unnamed check past a name the table holds", func(t *testing.T) {
+ schema := testutil.NewSchema(t, pool)
+ for _, ddl := range []string{
+ "CREATE TABLE %s.t (id int PRIMARY KEY, qty int)",
+ "ALTER TABLE %s.t ADD CONSTRAINT t_qty_check CHECK (qty < 1000)",
+ } {
+ _, err := pool.Exec(t.Context(), fmt.Sprintf(ddl, schema))
+ require.NoError(t, err, ddl)
+ }
+
+ v, err := migrate.Run(t.Context(), pool,
+ parseOne(t, fmt.Sprintf("ALTER TABLE %s.t ADD CHECK (qty > 0)", schema)), runOptions())
+ require.NoError(t, err)
+ assert.Equal(t, verdict.OutcomeExecuted, v.Outcome)
+ assert.Equal(t, []string{
+ fmt.Sprintf("ALTER TABLE %s.t ADD CONSTRAINT t_qty_check1 CHECK (qty > 0) NOT VALID", schema),
+ fmt.Sprintf(`ALTER TABLE "%s"."t" VALIDATE CONSTRAINT "t_qty_check1"`, schema),
+ }, v.ExecutedSQL)
+ })
+
t.Run("re-checks the gate for callers that skipped it", func(t *testing.T) {
schema := testutil.NewSchema(t, pool)
_, err := pool.Exec(t.Context(), fmt.Sprintf("CREATE TABLE %s.t (id int PRIMARY KEY)", schema))
diff --git a/pkg/plan/plan_test.go b/pkg/plan/plan_test.go
index 234f42c9..acd357db 100644
--- a/pkg/plan/plan_test.go
+++ b/pkg/plan/plan_test.go
@@ -74,25 +74,6 @@ func TestFromRoutedDerivesGuidanceForRewriteRequired(t *testing.T) {
assert.Equal(t, suggest.GuidanceAddColumnThenConstraint, st.Guidance)
}
-// An unnamed CHECK or FOREIGN KEY is rewrite-required — the VALIDATE step
-// needs the name the server assigns only at creation — and carries the
-// naming guidance rather than failing the whole plan.
-func TestFromRoutedUnnamedConstraintCarriesNamingGuidance(t *testing.T) {
- for _, sql := range []string{
- "ALTER TABLE users ADD CHECK (age > 0)",
- "ALTER TABLE users ADD FOREIGN KEY (org_id) REFERENCES orgs (id)",
- } {
- routed := router.Route([]planner.Plan{classifyCanonical(t, sql)})
- require.Len(t, routed.Statements, 1, "statement: %s", sql)
- require.Equal(t, router.DispositionRewriteRequired, routed.Statements[0].Disposition,
- "an unnamed constraint has no constructible online rewrite: %s", sql)
-
- st, err := plan.FromRouted(routed.Statements[0])
- require.NoError(t, err, "statement: %s", sql)
- assert.Equal(t, suggest.GuidanceNameConstraintThenValidate, st.Guidance, "statement: %s", sql)
- }
-}
-
// A compound rewrite-required statement advises splitting first: no safer
// sequence can be constructed for a multi-operation statement, so the
// manual path starts with one statement per operation.
diff --git a/pkg/planner/planner.go b/pkg/planner/planner.go
index 972101c0..41437e20 100644
--- a/pkg/planner/planner.go
+++ b/pkg/planner/planner.go
@@ -268,6 +268,12 @@ type Facts struct {
// constant; the proof leaves out only an ADD COLUMN IF NOT EXISTS for a
// column the table already has, which the server skips.
AddColumnProofs map[string]statement.AddColumnProof
+ // ConstraintNames are the names of the constraints already in the
+ // table's schema, which the server passes over when it names an
+ // unnamed CHECK or FOREIGN KEY. Without them the safer sequence states
+ // the server's first choice, and if the table already uses that name
+ // its NOT VALID step fails before anything is added.
+ ConstraintNames map[string]bool
}
// FactsFrom extracts the facts a live introspection model provides: the
@@ -413,6 +419,11 @@ func classifyOp(op statement.Op, st statement.Statement, facts Facts, sql string
case statement.OpAddConstraint:
d = classifyAddConstraint(op, st, sql, single)
+ if len(d.SaferSQL) > 0 && validatedOnAdd(op) {
+ // Restated from the live facts, which name an unnamed
+ // constraint as the server would on this table.
+ d.SaferSQL = notValidSequence(st, sql, facts)
+ }
case statement.OpValidateConstraint:
d.Route, d.Reason = RouteNative, ReasonOnlineIdiom
@@ -491,18 +502,15 @@ func classifyAddConstraint(op statement.Op, st statement.Statement, sql string,
d.SaferSQL = usingIndexSequence(st, op)
}
- case op.Constraint == statement.ConstraintCheck,
- op.Constraint == statement.ConstraintForeignKey:
+ case validatedOnAdd(op):
// Direct ADD CHECK/FK validates under ACCESS EXCLUSIVE; the safer
- // sequence is NOT VALID plus an online VALIDATE.
+ // sequence is NOT VALID plus an online VALIDATE. An unnamed
+ // constraint gets the server's first choice here, from the
+ // statement alone; classifyOp restates the sequence from the live
+ // facts.
d.Route, d.Reason = RouteNative, ReasonSaferIdiom
if single {
- if notValid, name, err := statement.AddNotValid(sql); err == nil {
- d.SaferSQL = []string{
- notValid,
- "ALTER TABLE " + tableIdent(st) + " VALIDATE CONSTRAINT " + pgx.Identifier{name}.Sanitize(),
- }
- }
+ d.SaferSQL = notValidSequence(st, sql, Facts{})
}
case op.Constraint == statement.ConstraintNotNull:
@@ -515,6 +523,39 @@ func classifyAddConstraint(op statement.Op, st statement.Statement, sql string,
return d
}
+// validatedOnAdd reports a CHECK or FOREIGN KEY, the constraints ADD
+// CONSTRAINT validates against every row unless told NOT VALID.
+func validatedOnAdd(op statement.Op) bool {
+ return op.Constraint == statement.ConstraintCheck || op.Constraint == statement.ConstraintForeignKey
+}
+
+// notValidSequence is the safer sequence for a single ADD CHECK or FOREIGN
+// KEY: the constraint added NOT VALID, then validated online. It is nil
+// when the statement cannot take NOT VALID.
+func notValidSequence(st statement.Statement, sql string, facts Facts) []string {
+ notValid, name, err := statement.AddNotValid(sql, constraintNaming(facts))
+ if err != nil {
+ return nil
+ }
+ return []string{
+ notValid,
+ "ALTER TABLE " + tableIdent(st) + " VALIDATE CONSTRAINT " + pgx.Identifier{name}.Sanitize(),
+ }
+}
+
+// constraintNaming is what facts tell the rewrite about naming an unnamed
+// constraint: the live columns, and the names the schema already uses.
+func constraintNaming(facts Facts) statement.ConstraintNaming {
+ naming := statement.ConstraintNaming{Taken: facts.ConstraintNames}
+ if facts.ColumnTypes != nil {
+ naming.Columns = make(map[string]bool, len(facts.ColumnTypes))
+ for column := range facts.ColumnTypes {
+ naming.Columns[column] = true
+ }
+ }
+ return naming
+}
+
// classifyTypeChange routes ALTER COLUMN TYPE: binary-coercible changes are
// a brief catalog relabel; everything else — or anything the planner cannot
// verify against live column facts — is a rewrite.
diff --git a/pkg/planner/planner_test.go b/pkg/planner/planner_test.go
index 0e8e875a..bb662891 100644
--- a/pkg/planner/planner_test.go
+++ b/pkg/planner/planner_test.go
@@ -295,7 +295,8 @@ func TestClassifyReferenceRows(t *testing.T) {
{"add foreign key", "ALTER TABLE t ADD CONSTRAINT fk FOREIGN KEY (pid) REFERENCES p (id)", planner.RouteNative, planner.ReasonSaferIdiom, 2},
{"add check not valid", "ALTER TABLE t ADD CONSTRAINT c CHECK (age > 0) NOT VALID", planner.RouteNative, planner.ReasonOnlineIdiom, 0},
{"add foreign key not valid", "ALTER TABLE t ADD CONSTRAINT fk FOREIGN KEY (pid) REFERENCES p (id) NOT VALID", planner.RouteNative, planner.ReasonOnlineIdiom, 0},
- {"add unnamed check", "ALTER TABLE t ADD CHECK (age > 0)", planner.RouteNative, planner.ReasonSaferIdiom, 0},
+ {"add unnamed check", "ALTER TABLE t ADD CHECK (age > 0)", planner.RouteNative, planner.ReasonSaferIdiom, 2},
+ {"add unnamed foreign key", "ALTER TABLE t ADD FOREIGN KEY (pid) REFERENCES p (id)", planner.RouteNative, planner.ReasonSaferIdiom, 2},
{"add exclusion", "ALTER TABLE t ADD CONSTRAINT ex EXCLUDE USING gist (room WITH =)", planner.RouteRefuse, planner.ReasonUnsupportedOperation, 0},
{"validate constraint", "ALTER TABLE t VALIDATE CONSTRAINT c", planner.RouteNative, planner.ReasonOnlineIdiom, 0},
{"drop constraint", "ALTER TABLE t DROP CONSTRAINT c", planner.RouteNative, planner.ReasonMetadataOnly, 0},
diff --git a/pkg/planner/script.go b/pkg/planner/script.go
new file mode 100644
index 00000000..d7ffa457
--- /dev/null
+++ b/pkg/planner/script.go
@@ -0,0 +1,88 @@
+package planner
+
+import (
+ "fmt"
+
+ "github.com/block/pg-sprite/pkg/statement"
+)
+
+// ScriptClassifier classifies the statements of one script in order, with
+// zero live facts except the constraint names the script's own earlier
+// statements leave behind. Running the script, the server numbers an
+// unnamed CHECK or FOREIGN KEY past a name an earlier statement claimed, so
+// a later safer sequence must state the numbered name too, or it would
+// collide with the constraint the script just added. Names are tracked per
+// schema as the statement writes it, an unqualified table under its own
+// key. The zero value is ready to use.
+type ScriptClassifier struct {
+ names map[string]map[string]bool
+}
+
+// Classify parses and routes sql, the script's next statement, as
+// [Classify] does, then records the constraint names it leaves: each
+// constraint an ALTER TABLE adds, by its stated name or the name its NOT
+// VALID sequence states, less each it drops. Any other constraint added
+// unnamed is not recorded, since the planner states no name for it.
+func (c *ScriptClassifier) Classify(sql string) (Plan, error) {
+ st, err := statement.ParseOne(sql)
+ if err != nil {
+ return Plan{}, err
+ }
+ plan, err := Classify(sql, Facts{ConstraintNames: c.names[st.Schema()]})
+ if err != nil {
+ return Plan{}, err
+ }
+ if st.Kind() != statement.KindAlterTable {
+ return plan, nil
+ }
+ ops, err := statement.ParseOps(sql)
+ if err != nil {
+ return Plan{}, err
+ }
+ if len(ops) != len(plan.Decisions) {
+ return Plan{}, fmt.Errorf("planner produced %d decisions for %d operations", len(plan.Decisions), len(ops))
+ }
+ for i, op := range ops {
+ switch op.Kind {
+ case statement.OpAddConstraint:
+ name, err := addedConstraintName(op, plan.Decisions[i])
+ if err != nil {
+ return Plan{}, err
+ }
+ if name != "" {
+ c.claim(st.Schema(), name)
+ }
+ case statement.OpDropConstraint:
+ delete(c.names[st.Schema()], op.Name)
+ }
+ }
+ return plan, nil
+}
+
+// addedConstraintName is the name op's constraint gets: the one it states,
+// else, for a CHECK or FOREIGN KEY, the one its NOT VALID sequence states,
+// else "" when neither does.
+func addedConstraintName(op statement.Op, d Decision) (string, error) {
+ if op.Name != "" || !validatedOnAdd(op) || len(d.SaferSQL) == 0 {
+ return op.Name, nil
+ }
+ ops, err := statement.ParseOps(d.SaferSQL[0])
+ if err != nil {
+ return "", fmt.Errorf("read the constraint the safer sequence adds: %w", err)
+ }
+ if len(ops) != 1 || ops[0].Kind != statement.OpAddConstraint {
+ return "", fmt.Errorf("safer sequence for %s does not start with one ADD CONSTRAINT: %s", op.Describe(), d.SaferSQL[0])
+ }
+ return ops[0].Name, nil
+}
+
+// claim records name as taken in schema.
+func (c *ScriptClassifier) claim(schema, name string) {
+ if c.names == nil {
+ c.names = map[string]map[string]bool{}
+ }
+ if c.names[schema] == nil {
+ c.names[schema] = map[string]bool{}
+ }
+ c.names[schema][name] = true
+}
diff --git a/pkg/planner/script_test.go b/pkg/planner/script_test.go
new file mode 100644
index 00000000..72013dc7
--- /dev/null
+++ b/pkg/planner/script_test.go
@@ -0,0 +1,67 @@
+package planner_test
+
+import (
+ "testing"
+
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
+
+ "github.com/block/pg-sprite/pkg/planner"
+)
+
+// A script classified in order numbers an unnamed constraint past the
+// names its earlier statements claim, whether stated or chosen by a safer
+// sequence, frees a name an earlier statement drops, and keeps schemas
+// apart.
+func TestScriptClassifierNumbersPastNamesTheScriptClaims(t *testing.T) {
+ cases := []struct {
+ name string
+ script []string
+ want string
+ }{
+ {
+ name: "first statement takes the first choice",
+ script: []string{"ALTER TABLE t ADD CHECK (qty > 0)"},
+ want: "t_qty_check",
+ },
+ {
+ name: "a name an earlier rewrite chose",
+ script: []string{"ALTER TABLE t ADD CHECK (qty > 0)", "ALTER TABLE t ADD CHECK (qty < 100)"},
+ want: "t_qty_check1",
+ },
+ {
+ name: "a name an earlier statement states",
+ script: []string{"ALTER TABLE t ADD CONSTRAINT t_qty_check CHECK (qty > 0) NOT VALID", "ALTER TABLE t ADD CHECK (qty < 100)"},
+ want: "t_qty_check1",
+ },
+ {
+ name: "a name an earlier statement dropped",
+ script: []string{
+ "ALTER TABLE t ADD CHECK (qty > 0)",
+ "ALTER TABLE t DROP CONSTRAINT t_qty_check",
+ "ALTER TABLE t ADD CHECK (qty < 100)",
+ },
+ want: "t_qty_check",
+ },
+ {
+ name: "a name claimed in another schema",
+ script: []string{"ALTER TABLE app.t ADD CHECK (qty > 0)", "ALTER TABLE t ADD CHECK (qty < 100)"},
+ want: "t_qty_check",
+ },
+ }
+ for _, tc := range cases {
+ t.Run(tc.name, func(t *testing.T) {
+ var classifier planner.ScriptClassifier
+ var last planner.Plan
+ for _, sql := range tc.script {
+ plan, err := classifier.Classify(sql)
+ require.NoError(t, err, sql)
+ last = plan
+ }
+ require.Len(t, last.Decisions, 1)
+ require.Len(t, last.Decisions[0].SaferSQL, 2)
+ assert.Contains(t, last.Decisions[0].SaferSQL[0], "ADD CONSTRAINT "+tc.want+" CHECK")
+ assert.Contains(t, last.Decisions[0].SaferSQL[1], `VALIDATE CONSTRAINT "`+tc.want+`"`)
+ })
+ }
+}
diff --git a/pkg/preflight/constraint_names.go b/pkg/preflight/constraint_names.go
new file mode 100644
index 00000000..91ccf603
--- /dev/null
+++ b/pkg/preflight/constraint_names.go
@@ -0,0 +1,32 @@
+package preflight
+
+import (
+ "context"
+ "fmt"
+
+ "github.com/jackc/pgx/v5"
+ "github.com/jackc/pgx/v5/pgxpool"
+)
+
+// SchemaConstraintNames reads the name of every constraint in schema, each
+// table's and each domain's: the names PostgreSQL passes over when it names
+// an unnamed constraint added to a table there, since it looks for a name
+// no constraint in the schema holds.
+func SchemaConstraintNames(ctx context.Context, pool *pgxpool.Pool, schema string) (map[string]bool, error) {
+ rows, err := pool.Query(ctx, `SELECT c.conname::text
+ FROM pg_catalog.pg_constraint c
+ JOIN pg_catalog.pg_namespace n ON n.oid = c.connamespace
+ WHERE n.nspname = $1`, schema)
+ if err != nil {
+ return nil, fmt.Errorf("read the constraint names in schema %s: %w", pgx.Identifier{schema}.Sanitize(), err)
+ }
+ names, err := pgx.CollectRows(rows, pgx.RowTo[string])
+ if err != nil {
+ return nil, fmt.Errorf("read the constraint names in schema %s: %w", pgx.Identifier{schema}.Sanitize(), err)
+ }
+ taken := make(map[string]bool, len(names))
+ for _, name := range names {
+ taken[name] = true
+ }
+ return taken, nil
+}
diff --git a/pkg/statement/constraint_name_integration_test.go b/pkg/statement/constraint_name_integration_test.go
new file mode 100644
index 00000000..b7776295
--- /dev/null
+++ b/pkg/statement/constraint_name_integration_test.go
@@ -0,0 +1,347 @@
+package statement_test
+
+import (
+ "fmt"
+ "slices"
+ "strings"
+ "testing"
+
+ "github.com/jackc/pgx/v5"
+ "github.com/jackc/pgx/v5/pgconn"
+ "github.com/jackc/pgx/v5/pgxpool"
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
+
+ "github.com/block/pg-sprite/internal/testutil"
+ "github.com/block/pg-sprite/pkg/dbconn"
+ "github.com/block/pg-sprite/pkg/preflight"
+ "github.com/block/pg-sprite/pkg/statement"
+)
+
+// Two-oracle check: the name AddNotValid gives an unnamed CHECK or FOREIGN
+// KEY, told the table's live columns and the constraint names its schema
+// already uses, is the name the server gives the same constraint added
+// without one, including when the first choice is taken and the server
+// moves on to a numbered label. The rewrite then runs and leaves the
+// constraint under that name, NOT VALID, ready for the VALIDATE step that
+// names it.
+func TestAddNotValidNamesAnUnnamedConstraintAsTheServerDoes(t *testing.T) {
+ pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: testutil.StartPostgres(t)})
+ require.NoError(t, err)
+ t.Cleanup(pool.Close)
+
+ longTable := strings.Repeat("a", 60)
+ longColumn := strings.Repeat("b", 60)
+ wideTable := strings.Repeat("ü", 31)
+ col := func(c byte) string { return strings.Repeat(string(c), 30) }
+ // Each case runs in its own schema, substituted as %[1]s; table is the
+ // unquoted name of the table the constraint is added to, and setup runs
+ // after create, before the constraint is added.
+ cases := []struct {
+ name string
+ table string
+ create string
+ setup []string
+ alter string
+ }{
+ {
+ name: "check on one column",
+ table: "t",
+ create: "CREATE TABLE %[1]s.t (qty int, code text)",
+ alter: "ALTER TABLE %[1]s.t ADD CHECK (qty > 0)",
+ },
+ {
+ name: "check on one column referenced twice",
+ table: "t",
+ create: "CREATE TABLE %[1]s.t (qty int, code text)",
+ alter: "ALTER TABLE %[1]s.t ADD CHECK (qty > 0 AND qty < 100)",
+ },
+ {
+ name: "check on two columns",
+ table: "t",
+ create: "CREATE TABLE %[1]s.t (qty int, code text)",
+ alter: "ALTER TABLE %[1]s.t ADD CHECK (qty > 0 OR code <> '')",
+ },
+ {
+ name: "check on no column",
+ table: "t",
+ create: "CREATE TABLE %[1]s.t (qty int)",
+ alter: "ALTER TABLE %[1]s.t ADD CHECK (1 > 0)",
+ },
+ {
+ name: "check on a column inside a function call",
+ table: "t",
+ create: "CREATE TABLE %[1]s.t (qty int, code text)",
+ alter: "ALTER TABLE %[1]s.t ADD CHECK (length(code) > 0)",
+ },
+ {
+ name: "check on a column qualified by its table",
+ table: "t",
+ create: "CREATE TABLE %[1]s.t (qty int)",
+ alter: "ALTER TABLE %[1]s.t ADD CHECK (t.qty > 0)",
+ },
+ {
+ name: "check on a column qualified by its schema and table",
+ table: "t",
+ create: "CREATE TABLE %[1]s.t (qty int)",
+ alter: "ALTER TABLE %[1]s.t ADD CHECK (%[1]s.t.qty > 0)",
+ },
+ {
+ name: "check on a field of a composite column",
+ table: "t",
+ create: "CREATE TABLE %[1]s.t (addr %[1]s.address)",
+ alter: "ALTER TABLE %[1]s.t ADD CHECK ((addr).zip <> '')",
+ },
+ {
+ name: "check on a field of a composite column qualified by its table",
+ table: "t",
+ create: "CREATE TABLE %[1]s.t (addr %[1]s.address)",
+ alter: "ALTER TABLE %[1]s.t ADD CHECK ((t.addr).zip <> '')",
+ },
+ {
+ name: "check on the whole row by the table's bare name",
+ table: "t",
+ create: "CREATE TABLE %[1]s.t (qty int)",
+ alter: "ALTER TABLE %[1]s.t ADD CHECK (t IS NOT NULL)",
+ },
+ {
+ name: "check on a field selected from the whole row",
+ table: "t",
+ create: "CREATE TABLE %[1]s.t (qty int, code text)",
+ alter: "ALTER TABLE %[1]s.t ADD CHECK ((t).qty > 0)",
+ },
+ {
+ name: "check on a field selected from the whole row's star",
+ table: "t",
+ create: "CREATE TABLE %[1]s.t (qty int, code text)",
+ alter: "ALTER TABLE %[1]s.t ADD CHECK ((t.*).qty > 0)",
+ },
+ {
+ name: "check on a row of the whole row's one column",
+ table: "t",
+ create: "CREATE TABLE %[1]s.t (qty int)",
+ alter: "ALTER TABLE %[1]s.t ADD CHECK (ROW(t.*) IS NOT NULL)",
+ },
+ {
+ name: "check on a row of the whole row's two columns",
+ table: "t",
+ create: "CREATE TABLE %[1]s.t (qty int, code text)",
+ alter: "ALTER TABLE %[1]s.t ADD CHECK (ROW(t.*) IS NOT NULL)",
+ },
+ {
+ name: "check on a row of a column and the whole row's one column",
+ table: "t",
+ create: "CREATE TABLE %[1]s.t (qty int)",
+ alter: "ALTER TABLE %[1]s.t ADD CHECK (ROW(qty, t.*) IS NOT NULL)",
+ },
+ {
+ name: "check on the whole row and a field selected from it",
+ table: "t",
+ create: "CREATE TABLE %[1]s.t (qty int, code text)",
+ alter: "ALTER TABLE %[1]s.t ADD CHECK (t IS NOT NULL AND (t).qty > 0)",
+ },
+ {
+ name: "check on a column named like its table",
+ table: "t",
+ create: "CREATE TABLE %[1]s.t (t int)",
+ alter: "ALTER TABLE %[1]s.t ADD CHECK (t > 0)",
+ },
+ {
+ name: "check whose first choice another table in the schema holds",
+ table: "t",
+ create: "CREATE TABLE %[1]s.t (qty int)",
+ setup: []string{"CREATE TABLE %[1]s.other (x int, CONSTRAINT t_qty_check CHECK (x > 0))"},
+ alter: "ALTER TABLE %[1]s.t ADD CHECK (qty > 0)",
+ },
+ {
+ name: "check whose first two choices the table and a domain hold",
+ table: "t",
+ create: "CREATE TABLE %[1]s.t (qty int)",
+ setup: []string{
+ "ALTER TABLE %[1]s.t ADD CONSTRAINT t_qty_check CHECK (qty < 1000)",
+ "CREATE DOMAIN %[1]s.positive AS int CONSTRAINT t_qty_check1 CHECK (VALUE > 0)",
+ },
+ alter: "ALTER TABLE %[1]s.t ADD CHECK (qty > 0)",
+ },
+ {
+ name: "check whose truncated first choice is taken",
+ table: longTable,
+ create: "CREATE TABLE %[1]s." + longTable + " (" + longColumn + " int)",
+ setup: []string{
+ "ALTER TABLE %[1]s." + longTable + " ADD CHECK (" + longColumn + " < 1000)",
+ },
+ alter: "ALTER TABLE %[1]s." + longTable + " ADD CHECK (" + longColumn + " > 0)",
+ },
+ {
+ name: "foreign key whose first choice another table in the schema holds",
+ table: "t",
+ create: "CREATE TABLE %[1]s.t (pid int)",
+ setup: []string{"CREATE TABLE %[1]s.other (x int, CONSTRAINT t_pid_fkey CHECK (x > 0))"},
+ alter: "ALTER TABLE %[1]s.t ADD FOREIGN KEY (pid) REFERENCES %[1]s.p (id)",
+ },
+ {
+ name: "check on a column quoted as a star",
+ table: "t",
+ create: `CREATE TABLE %[1]s.t ("*" int)`,
+ alter: `ALTER TABLE %[1]s.t ADD CHECK ("*" > 0)`,
+ },
+ {
+ name: "check on the whole row",
+ table: "t",
+ create: "CREATE TABLE %[1]s.t (qty int)",
+ alter: "ALTER TABLE %[1]s.t ADD CHECK (t.* IS NOT NULL)",
+ },
+ {
+ name: "check on a quoted mixed-case table and column",
+ table: "Orders",
+ create: `CREATE TABLE %[1]s."Orders" ("Qty" int)`,
+ alter: `ALTER TABLE %[1]s."Orders" ADD CHECK ("Qty" > 0)`,
+ },
+ {
+ name: "check whose name is truncated",
+ table: longTable,
+ create: "CREATE TABLE %[1]s." + longTable + " (" + longColumn + " int)",
+ alter: "ALTER TABLE %[1]s." + longTable + " ADD CHECK (" + longColumn + " > 0)",
+ },
+ {
+ name: "check whose truncation lands inside a multibyte character",
+ table: wideTable,
+ create: "CREATE TABLE %[1]s." + wideTable + " (qty int)",
+ alter: "ALTER TABLE %[1]s." + wideTable + " ADD CHECK (qty > 0)",
+ },
+ {
+ name: "foreign key on one column",
+ table: "t",
+ create: "CREATE TABLE %[1]s.t (pid int)",
+ alter: "ALTER TABLE %[1]s.t ADD FOREIGN KEY (pid) REFERENCES %[1]s.p (id)",
+ },
+ {
+ name: "foreign key on two columns",
+ table: "t",
+ create: "CREATE TABLE %[1]s.t (pid int, pk int)",
+ alter: "ALTER TABLE %[1]s.t ADD FOREIGN KEY (pid, pk) REFERENCES %[1]s.p (id, k)",
+ },
+ {
+ name: "foreign key whose column list is truncated",
+ table: "t",
+ create: "CREATE TABLE %[1]s.t (" + col('x') + " int, " + col('y') + " int, " + col('z') + " int)",
+ alter: "ALTER TABLE %[1]s.t ADD FOREIGN KEY (" + col('x') + ", " + col('y') + ", " + col('z') + ") " +
+ "REFERENCES %[1]s.p3 (a, b, c)",
+ },
+ }
+ for _, tc := range cases {
+ t.Run(tc.name, func(t *testing.T) {
+ serverSchema := testutil.NewSchema(t, pool)
+ rewriteSchema := testutil.NewSchema(t, pool)
+ for _, schema := range []string{serverSchema, rewriteSchema} {
+ ddls := []string{
+ "CREATE TABLE %[1]s.p (id int PRIMARY KEY, k int, UNIQUE (id, k))",
+ "CREATE TABLE %[1]s.p3 (a int, b int, c int, UNIQUE (a, b, c))",
+ "CREATE TYPE %[1]s.address AS (zip text)",
+ tc.create,
+ }
+ for _, ddl := range append(ddls, tc.setup...) {
+ _, err := pool.Exec(t.Context(), fmt.Sprintf(ddl, schema))
+ require.NoError(t, err, ddl)
+ }
+ }
+
+ before := tableConstraints(t, pool, serverSchema, tc.table)
+ _, err := pool.Exec(t.Context(), fmt.Sprintf(tc.alter, serverSchema))
+ require.NoError(t, err)
+ serverName, _ := addedConstraint(t, pool, serverSchema, tc.table, before)
+
+ taken, err := preflight.SchemaConstraintNames(t.Context(), pool, rewriteSchema)
+ require.NoError(t, err)
+ naming := statement.ConstraintNaming{Columns: tableColumns(t, pool, rewriteSchema, tc.table), Taken: taken}
+ rewritten, name, err := statement.AddNotValid(fmt.Sprintf(tc.alter, rewriteSchema), naming)
+ require.NoError(t, err)
+ assert.Equal(t, serverName, name, "the name the server chose for %s", tc.alter)
+
+ before = tableConstraints(t, pool, rewriteSchema, tc.table)
+ _, err = pool.Exec(t.Context(), rewritten)
+ require.NoError(t, err, rewritten)
+ rewrittenName, validated := addedConstraint(t, pool, rewriteSchema, tc.table, before)
+ assert.Equal(t, name, rewrittenName)
+ assert.False(t, validated, "the rewrite adds the constraint NOT VALID")
+ })
+ }
+}
+
+// Without the schema's constraint names the rewrite states the server's
+// first choice, and when the table already holds it the NOT VALID step
+// fails with a duplicate-object error before anything is added.
+func TestAddNotValidUnawareOfATakenNameFailsBeforeAdding(t *testing.T) {
+ pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: testutil.StartPostgres(t)})
+ require.NoError(t, err)
+ t.Cleanup(pool.Close)
+ schema := testutil.NewSchema(t, pool)
+ for _, ddl := range []string{
+ "CREATE TABLE %[1]s.t (qty int)",
+ "ALTER TABLE %[1]s.t ADD CONSTRAINT t_qty_check CHECK (qty < 1000)",
+ } {
+ _, err := pool.Exec(t.Context(), fmt.Sprintf(ddl, schema))
+ require.NoError(t, err, ddl)
+ }
+
+ rewritten, name, err := statement.AddNotValid(fmt.Sprintf("ALTER TABLE %s.t ADD CHECK (qty > 0)", schema), statement.ConstraintNaming{})
+ require.NoError(t, err)
+ assert.Equal(t, "t_qty_check", name)
+ _, err = pool.Exec(t.Context(), rewritten)
+ var pgErr *pgconn.PgError
+ require.ErrorAs(t, err, &pgErr)
+ assert.Equal(t, "42710", pgErr.Code, "duplicate_object")
+ assert.Equal(t, map[string]bool{"t_qty_check": true}, tableConstraints(t, pool, schema, "t"), "nothing was added")
+}
+
+// tableConstraints returns the names of the CHECK and FOREIGN KEY
+// constraints on schema.table.
+func tableConstraints(t *testing.T, pool *pgxpool.Pool, schema, table string) map[string]bool {
+ t.Helper()
+ rows, err := pool.Query(t.Context(), `SELECT conname::text FROM pg_catalog.pg_constraint
+ WHERE conrelid = $1::regclass AND contype IN ('c', 'f')`, pgx.Identifier{schema, table}.Sanitize())
+ require.NoError(t, err)
+ names, err := pgx.CollectRows(rows, pgx.RowTo[string])
+ require.NoError(t, err)
+ set := make(map[string]bool, len(names))
+ for _, name := range names {
+ set[name] = true
+ }
+ return set
+}
+
+// tableColumns returns the live columns of schema.table.
+func tableColumns(t *testing.T, pool *pgxpool.Pool, schema, table string) map[string]bool {
+ t.Helper()
+ rows, err := pool.Query(t.Context(), `SELECT attname::text FROM pg_catalog.pg_attribute
+ WHERE attrelid = $1::regclass AND attnum > 0 AND NOT attisdropped`, pgx.Identifier{schema, table}.Sanitize())
+ require.NoError(t, err)
+ names, err := pgx.CollectRows(rows, pgx.RowTo[string])
+ require.NoError(t, err)
+ columns := make(map[string]bool, len(names))
+ for _, name := range names {
+ columns[name] = true
+ }
+ return columns
+}
+
+// addedConstraint returns the name and validity of the one CHECK or
+// FOREIGN KEY constraint on schema.table that before does not name.
+func addedConstraint(t *testing.T, pool *pgxpool.Pool, schema, table string, before map[string]bool) (string, bool) {
+ t.Helper()
+ rows, err := pool.Query(t.Context(), `SELECT conname, convalidated FROM pg_catalog.pg_constraint
+ WHERE conrelid = $1::regclass AND contype IN ('c', 'f')`, pgx.Identifier{schema, table}.Sanitize())
+ require.NoError(t, err)
+ type constraint struct {
+ name string
+ validated bool
+ }
+ found, err := pgx.CollectRows(rows, func(row pgx.CollectableRow) (constraint, error) {
+ var c constraint
+ return c, row.Scan(&c.name, &c.validated)
+ })
+ require.NoError(t, err)
+ found = slices.DeleteFunc(found, func(c constraint) bool { return before[c.name] })
+ require.Len(t, found, 1)
+ return found[0].name, found[0].validated
+}
diff --git a/pkg/statement/ops_test.go b/pkg/statement/ops_test.go
index 2093ba79..6b9d9c22 100644
--- a/pkg/statement/ops_test.go
+++ b/pkg/statement/ops_test.go
@@ -421,7 +421,7 @@ func TestConcurrentlyRefusesOtherStatements(t *testing.T) {
}
func TestAddNotValidRewritesNamedCheck(t *testing.T) {
- safer, name, err := statement.AddNotValid("ALTER TABLE t ADD CONSTRAINT c CHECK (age > 0)")
+ safer, name, err := statement.AddNotValid("ALTER TABLE t ADD CONSTRAINT c CHECK (age > 0)", statement.ConstraintNaming{})
require.NoError(t, err)
assert.Equal(t, "c", name)
op := parseOneOp(t, safer)
@@ -433,14 +433,13 @@ func TestAddNotValidRefusals(t *testing.T) {
name string
sql string
}{
- {"unnamed constraint", "ALTER TABLE t ADD CHECK (age > 0)"},
{"primary key", "ALTER TABLE t ADD CONSTRAINT p PRIMARY KEY (id)"},
{"not an add constraint", "ALTER TABLE t DROP COLUMN a"},
{"multiple subcommands", "ALTER TABLE t ADD CONSTRAINT c CHECK (a > 0), DROP COLUMN b"},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
- _, _, err := statement.AddNotValid(tc.sql)
+ _, _, err := statement.AddNotValid(tc.sql, statement.ConstraintNaming{})
assert.ErrorIs(t, err, statement.ErrNotValidNotApplicable)
})
}
diff --git a/pkg/statement/rewrite.go b/pkg/statement/rewrite.go
index 2adaa5bd..81d3e1fb 100644
--- a/pkg/statement/rewrite.go
+++ b/pkg/statement/rewrite.go
@@ -3,9 +3,13 @@ package statement
import (
"errors"
"fmt"
+ "slices"
+ "strconv"
+ "strings"
pganalyze "github.com/pganalyze/pg_query_go/v6"
pgquery "github.com/wasilibs/go-pgquery"
+ "google.golang.org/protobuf/reflect/protoreflect"
)
// Typed refusals for advisory rewrites. The rewriters flip one syntactic
@@ -16,7 +20,7 @@ var (
// CONCURRENTLY form to rewrite to.
ErrNotRewritable = errors.New("statement has no concurrent form")
// ErrNotValidNotApplicable is returned when the statement is not a
- // single named ADD CHECK / ADD FOREIGN KEY that could take NOT VALID.
+ // single ADD CHECK / ADD FOREIGN KEY that could take NOT VALID.
ErrNotValidNotApplicable = errors.New("statement cannot take NOT VALID")
)
@@ -49,11 +53,35 @@ func Concurrently(sql string) (string, error) {
return deparseOne(node)
}
+// ConstraintNaming is what the server consults when it names an unnamed
+// constraint, read from the live table. The zero value knows nothing: a
+// bare name reads as a column and no name counts as taken, which yields the
+// server's first choice.
+type ConstraintNaming struct {
+ // Columns are the table's live columns. A bare name in a CHECK reads
+ // as a column when the table has one by that name and otherwise, when
+ // it is the table's own name, as the table's whole row. Nil reads every
+ // bare name as a column.
+ Columns map[string]bool
+ // Taken are the names of the constraints already in the table's
+ // schema, every table's and domain's alike: the server passes over each
+ // of them when it picks a name.
+ Taken map[string]bool
+}
+
// AddNotValid rewrites a single-command ALTER TABLE ... ADD CONSTRAINT
-// (named CHECK or FOREIGN KEY) to its NOT VALID form and returns the
-// rewritten statement plus the constraint name for the follow-up
-// VALIDATE CONSTRAINT step.
-func AddNotValid(sql string) (rewritten, constraint string, err error) {
+// (CHECK or FOREIGN KEY) to its NOT VALID form and returns the rewritten
+// statement plus the constraint name for the follow-up VALIDATE CONSTRAINT
+// step.
+//
+// The VALIDATE step has to name the constraint, so an unnamed constraint is
+// given the name PostgreSQL would choose (defaultConstraintName) and the
+// rewrite states it. The server takes its first choice unless a constraint
+// in the schema already holds it, and then tries the label with 1, 2, ...
+// appended; naming.Taken lets the rewrite make the same moves. A name the
+// rewrite states that the table already uses fails the NOT VALID step with
+// a duplicate-object error, before anything is added.
+func AddNotValid(sql string, naming ConstraintNaming) (rewritten, constraint string, err error) {
node, err := parseSingle(sql)
if err != nil {
return "", "", err
@@ -69,9 +97,12 @@ func AddNotValid(sql string) (rewritten, constraint string, err error) {
con := cmd.GetDef().GetConstraint()
validatable := con.GetContype() == pganalyze.ConstrType_CONSTR_CHECK ||
con.GetContype() == pganalyze.ConstrType_CONSTR_FOREIGN
- if !validatable || con.GetConname() == "" {
+ if !validatable {
return "", "", ErrNotValidNotApplicable
}
+ if con.GetConname() == "" {
+ con.Conname = defaultConstraintName(alter.GetRelation().GetRelname(), con, naming)
+ }
con.SkipValidation = true
con.InitiallyValid = false
if rewritten, err = deparseOne(node); err != nil {
@@ -80,6 +111,165 @@ func AddNotValid(sql string) (rewritten, constraint string, err error) {
return rewritten, con.GetConname(), nil
}
+// defaultConstraintName mirrors PostgreSQL's ChooseConstraintName for an
+// unnamed CHECK or FOREIGN KEY added to table: `__fkey`
+// over the referencing columns of a foreign key, and for a check
+// `__check` when its expression references exactly one
+// distinct column, else `_check`, truncated the way the server
+// truncates (makeObjectName). A name naming.Taken holds moves on to the
+// label with 1, 2, ... appended, as the server's does.
+//
+// The server counts the columns of the analyzed expression; this counts
+// the column references of the parsed one, resolved the way the server
+// resolves them against the one table in scope (checkColumn). The server
+// itself calls the rule an approximation of the column-versus-table
+// constraint distinction.
+func defaultConstraintName(table string, con *pganalyze.Constraint, naming ConstraintNaming) string {
+ var column, label string
+ if con.GetContype() == pganalyze.ConstrType_CONSTR_FOREIGN {
+ columns := make([]string, 0, len(con.GetFkAttrs()))
+ for _, attr := range con.GetFkAttrs() {
+ columns = append(columns, attr.GetString_().GetSval())
+ }
+ column, label = strings.Join(columns, "_"), "fkey"
+ } else {
+ refs := checkColumns(con.GetRawExpr().ProtoReflect(), table, naming.Columns, nil)
+ if len(refs) == 1 && refs[0] != wholeRowRef {
+ column = refs[0]
+ }
+ label = "check"
+ }
+ name := makeObjectName(table, column, label)
+ for pass := 1; naming.Taken[name]; pass++ {
+ name = makeObjectName(table, column, label+strconv.Itoa(pass))
+ }
+ return name
+}
+
+// wholeRowRef stands for a whole-row reference among column names. No
+// column can be named the empty string, so a column quoted as "*" stays
+// distinct.
+const wholeRowRef = ""
+
+// checkColumns appends to names the distinct columns of table that msg's
+// parse tree references, resolving each reference with checkColumn. The
+// walk covers every message field, list, and map, so a reference inside a
+// function call, CASE, or operand counts like one at the top.
+//
+// Two forms read columns other than the reference they wrap, as the
+// server's analysis resolves them. A field selected from the whole row,
+// `(t).qty` or `(t.*).qty`, reads that column alone. A whole-row star
+// inside ROW(...), `ROW(t.*)`, expands to every column of the table, which
+// takes columns to be known; without them it counts as the whole row.
+func checkColumns(msg protoreflect.Message, table string, columns map[string]bool, names []string) []string {
+ addName := func(name string) {
+ if !slices.Contains(names, name) {
+ names = append(names, name)
+ }
+ }
+ switch node := msg.Interface().(type) {
+ case *pganalyze.ColumnRef:
+ if name, ok := checkColumn(node.GetFields(), table, columns); ok {
+ addName(name)
+ }
+ return names
+ case *pganalyze.A_Indirection:
+ if field, ok := wholeRowField(node, table, columns); ok {
+ addName(field)
+ for _, ind := range node.GetIndirection()[1:] {
+ names = checkColumns(ind.ProtoReflect(), table, columns, names)
+ }
+ return names
+ }
+ case *pganalyze.RowExpr:
+ if columns != nil {
+ for _, arg := range node.GetArgs() {
+ if ref := arg.GetColumnRef(); ref != nil && isWholeRowStar(ref, table) {
+ for column := range columns {
+ addName(column)
+ }
+ continue
+ }
+ names = checkColumns(arg.ProtoReflect(), table, columns, names)
+ }
+ return names
+ }
+ }
+ msg.Range(func(field protoreflect.FieldDescriptor, value protoreflect.Value) bool {
+ if field.Kind() != protoreflect.MessageKind {
+ return true
+ }
+ switch {
+ case field.IsMap():
+ if field.MapValue().Kind() == protoreflect.MessageKind {
+ value.Map().Range(func(_ protoreflect.MapKey, item protoreflect.Value) bool {
+ names = checkColumns(item.Message(), table, columns, names)
+ return true
+ })
+ }
+ case field.IsList():
+ list := value.List()
+ for i := 0; i < list.Len(); i++ {
+ names = checkColumns(list.Get(i).Message(), table, columns, names)
+ }
+ default:
+ names = checkColumns(value.Message(), table, columns, names)
+ }
+ return true
+ })
+ return names
+}
+
+// checkColumn resolves a column reference in a CHECK on table to the column
+// it reads, or wholeRowRef, the way the server's transformColumnRef does
+// with the table as the only relation in scope. A qualified reference
+// (`t.qty`, `s.t.qty`, `c.s.t.qty`) names the column last, or the whole row
+// with a star; a field of a composite column is written `(addr).zip`, which
+// wraps a reference to addr rather than extending it. A bare name is a
+// column unless columns, when known, has no such column and the name is
+// the table's, which makes it the whole row.
+func checkColumn(fields []*pganalyze.Node, table string, columns map[string]bool) (string, bool) {
+ if len(fields) == 0 {
+ return "", false
+ }
+ last := fields[len(fields)-1]
+ if last.GetAStar() != nil {
+ return wholeRowRef, true
+ }
+ name := last.GetString_().GetSval()
+ if len(fields) == 1 && name == table && columns != nil && !columns[name] {
+ return wholeRowRef, true
+ }
+ return name, true
+}
+
+// wholeRowField reports the column a field selection from the table's
+// whole row names: `(t).qty` and `(t.*).qty` read qty.
+func wholeRowField(ind *pganalyze.A_Indirection, table string, columns map[string]bool) (string, bool) {
+ ref := ind.GetArg().GetColumnRef()
+ if ref == nil || len(ind.GetIndirection()) == 0 {
+ return "", false
+ }
+ field := ind.GetIndirection()[0].GetString_()
+ if field == nil {
+ return "", false
+ }
+ if name, ok := checkColumn(ref.GetFields(), table, columns); !ok || name != wholeRowRef {
+ return "", false
+ }
+ return field.GetSval(), true
+}
+
+// isWholeRowStar reports `t.*` or a bare `*`, the forms ROW(...) expands
+// to the table's columns.
+func isWholeRowStar(ref *pganalyze.ColumnRef, table string) bool {
+ fields := ref.GetFields()
+ if len(fields) == 0 || fields[len(fields)-1].GetAStar() == nil {
+ return false
+ }
+ return len(fields) == 1 || fields[len(fields)-2].GetString_().GetSval() == table
+}
+
// parseSingle parses sql and requires exactly one statement, returning its
// root node for in-place rewriting.
func parseSingle(sql string) (*pganalyze.Node, error) {
diff --git a/pkg/suggest/residue_integration_test.go b/pkg/suggest/residue_integration_test.go
index c78c0ea2..44f09443 100644
--- a/pkg/suggest/residue_integration_test.go
+++ b/pkg/suggest/residue_integration_test.go
@@ -5,7 +5,9 @@ import (
"fmt"
"testing"
+ "github.com/jackc/pgx/v5"
"github.com/jackc/pgx/v5/pgconn"
+ "github.com/jackc/pgx/v5/pgxpool"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
@@ -96,3 +98,44 @@ func TestSetNotNullSequenceResidueMatchesCaveat(t *testing.T) {
schema).Scan(¬Null))
assert.True(t, notNull, "resuming from VALIDATE converges on the declared end state")
}
+
+// Two-oracle check for a script: the advice for two unnamed CHECKs on one
+// column runs on a server, and leaves the same constraint names as the
+// original script run directly.
+func TestAdviceForUnnamedChecksInOneScriptRunsUnderTheServerNames(t *testing.T) {
+ pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: testutil.StartPostgres(t)})
+ require.NoError(t, err)
+ t.Cleanup(pool.Close)
+ script := "ALTER TABLE %[1]s.t ADD CHECK (qty > 0); ALTER TABLE %[1]s.t ADD CHECK (qty < 100);"
+
+ serverSchema := testutil.NewSchema(t, pool)
+ adviceSchema := testutil.NewSchema(t, pool)
+ for _, schema := range []string{serverSchema, adviceSchema} {
+ _, err := pool.Exec(t.Context(), fmt.Sprintf("CREATE TABLE %s.t (qty int)", schema))
+ require.NoError(t, err)
+ }
+ _, err = pool.Exec(t.Context(), fmt.Sprintf(script, serverSchema))
+ require.NoError(t, err)
+
+ report, err := suggest.Advise(fmt.Sprintf(script, adviceSchema))
+ require.NoError(t, err)
+ require.Len(t, report.Suggestions, 2)
+ for _, s := range report.Suggestions {
+ for _, sql := range s.Recommended {
+ _, err := pool.Exec(t.Context(), sql)
+ require.NoError(t, err, sql)
+ }
+ }
+ assert.Equal(t, checkNames(t, pool, serverSchema), checkNames(t, pool, adviceSchema))
+}
+
+// checkNames returns the sorted names of the CHECK constraints on schema.t.
+func checkNames(t *testing.T, pool *pgxpool.Pool, schema string) []string {
+ t.Helper()
+ rows, err := pool.Query(t.Context(), `SELECT conname::text FROM pg_catalog.pg_constraint
+ WHERE conrelid = ($1 || '.t')::regclass AND contype = 'c' ORDER BY conname`, schema)
+ require.NoError(t, err)
+ names, err := pgx.CollectRows(rows, pgx.RowTo[string])
+ require.NoError(t, err)
+ return names
+}
diff --git a/pkg/suggest/suggest.go b/pkg/suggest/suggest.go
index 05501754..e4b3de44 100644
--- a/pkg/suggest/suggest.go
+++ b/pkg/suggest/suggest.go
@@ -91,9 +91,8 @@ const (
// GuidanceAddColumnThenConstraint: an inline UNIQUE / PRIMARY KEY /
// FOREIGN KEY / CHECK on ADD COLUMN builds or validates under the ADD
// COLUMN's ACCESS EXCLUSIVE lock. Add the plain column first, then
- // build the constraint with its online pattern as a separate, named
- // ADD CONSTRAINT — named, because the unnamed ADD CHECK / ADD FOREIGN
- // KEY forms are themselves refused (GuidanceNameConstraintThenValidate).
+ // build the constraint with its online pattern as a separate ADD
+ // CONSTRAINT.
GuidanceAddColumnThenConstraint Guidance = "add-column-then-constraint"
// GuidancePrevalidatedCheck: ATTACH PARTITION scans the child under
// the parent's lock unless a validated CHECK matching the partition
@@ -106,11 +105,12 @@ const (
// constraint is a catalog flip — the same scaffold sequence the
// SET NOT NULL form gets constructed.
GuidanceNotNullScaffold Guidance = "not-null-scaffold"
- // GuidanceNameConstraintThenValidate: an unnamed ADD CHECK / ADD
- // FOREIGN KEY has no constructible rewrite because the online
- // sequence's VALIDATE CONSTRAINT step needs the constraint's name and
- // the server assigns one only at creation. Name the constraint, add
- // it NOT VALID, then VALIDATE it online.
+ // GuidanceNameConstraintThenValidate: an ADD CHECK / ADD FOREIGN KEY
+ // whose NOT VALID rewrite could not be constructed from the statement.
+ // Name the constraint, add it NOT VALID, then VALIDATE it online — the
+ // sequence the constructed rewrite emits, under the server's default
+ // name when the statement names none. Keeps ManualGuidance total over
+ // every constraint kind the classifier can mark safer-idiom.
GuidanceNameConstraintThenValidate Guidance = "name-constraint-then-validate"
// GuidanceUniqueIndexThenConstraint: an ADD PRIMARY KEY / ADD UNIQUE
// whose USING INDEX rewrite could not be constructed. Build the unique
@@ -213,7 +213,9 @@ type Report struct {
}
// Advise maps a DDL script to its advisory results: every statement is
-// parsed with the PostgreSQL grammar and classified with zero live facts,
+// parsed with the PostgreSQL grammar and classified with zero live facts
+// beyond the constraint names the script's earlier statements claim
+// (planner.ScriptClassifier),
// and each risky-as-written operation yields a Suggestion — the safer
// sequence when the planner could construct it, typed guidance when it
// could not. Nothing is executed and no database is touched. A parse
@@ -224,8 +226,9 @@ func Advise(sql string) (Report, error) {
return Report{}, err
}
report := Report{FormatVersion: FormatVersion, Suggestions: []Suggestion{}}
+ var classifier planner.ScriptClassifier
for i, stmt := range stmts {
- suggestions, err := adviseStatement(i+1, stmt)
+ suggestions, err := adviseStatement(&classifier, i+1, stmt)
if err != nil {
return Report{}, fmt.Errorf("statement %d: %w", i+1, err)
}
@@ -238,8 +241,8 @@ func Advise(sql string) (Report, error) {
// safer-idiom decision. The operation list and decision list are
// index-aligned by the planner's contract (one decision per operation, in
// order); a mismatch is a contract violation and fails closed.
-func adviseStatement(index int, stmt statement.SourceStatement) ([]Suggestion, error) {
- plan, err := planner.Classify(stmt.SQL, planner.Facts{})
+func adviseStatement(classifier *planner.ScriptClassifier, index int, stmt statement.SourceStatement) ([]Suggestion, error) {
+ plan, err := classifier.Classify(stmt.SQL)
if err != nil {
return nil, err
}
@@ -330,8 +333,9 @@ func ManualGuidance(op statement.Op, multi bool) (Guidance, error) {
case statement.ConstraintNotNull:
return GuidanceNotNullScaffold, nil
case statement.ConstraintCheck, statement.ConstraintForeignKey:
- // Reached only for the unnamed form: a named CHECK / FOREIGN
- // KEY gets the NOT VALID → VALIDATE rewrite constructed.
+ // Reached only when the NOT VALID → VALIDATE rewrite could
+ // not be constructed from the statement; named or not, a
+ // single CHECK / FOREIGN KEY gets it constructed.
return GuidanceNameConstraintThenValidate, nil
case statement.ConstraintPrimaryKey, statement.ConstraintUnique:
// Reached only when the USING INDEX rewrite could not be
diff --git a/pkg/suggest/suggest_test.go b/pkg/suggest/suggest_test.go
index 4e715d33..784ff00d 100644
--- a/pkg/suggest/suggest_test.go
+++ b/pkg/suggest/suggest_test.go
@@ -145,28 +145,60 @@ func TestAdviseInlineConstraintGetsAddColumnThenConstraintGuidance(t *testing.T)
assert.Equal(t, suggest.GuidanceAddColumnThenConstraint, s.Guidance)
}
-// An unnamed CHECK or FOREIGN KEY has no constructible rewrite — the
-// VALIDATE step needs the name the server assigns only at creation — so
-// the advice is the typed manual path, not an error.
-func TestAdviseUnnamedCheckAndForeignKeyGetNamingGuidance(t *testing.T) {
- for _, sql := range []string{
- "ALTER TABLE t ADD CHECK (age > 0)",
- "ALTER TABLE t ADD FOREIGN KEY (o) REFERENCES orders (id)",
+// An unnamed CHECK or FOREIGN KEY gets the NOT VALID → VALIDATE rewrite
+// constructed under the name the server would choose first, which the
+// VALIDATE step needs: the advice is runnable SQL, not a manual path.
+func TestAdviseUnnamedCheckAndForeignKeyGetTheRewriteUnderTheServerName(t *testing.T) {
+ for _, tc := range []struct {
+ sql string
+ recommended []string
+ }{
+ {
+ sql: "ALTER TABLE t ADD CHECK (age > 0)",
+ recommended: []string{
+ "ALTER TABLE t ADD CONSTRAINT t_age_check CHECK (age > 0) NOT VALID",
+ `ALTER TABLE "t" VALIDATE CONSTRAINT "t_age_check"`,
+ },
+ },
+ {
+ sql: "ALTER TABLE t ADD FOREIGN KEY (o) REFERENCES orders (id)",
+ recommended: []string{
+ "ALTER TABLE t ADD CONSTRAINT t_o_fkey FOREIGN KEY (o) REFERENCES orders (id) NOT VALID",
+ `ALTER TABLE "t" VALIDATE CONSTRAINT "t_o_fkey"`,
+ },
+ },
} {
- report, err := suggest.Advise(sql)
- require.NoError(t, err, "statement: %s", sql)
- require.Len(t, report.Suggestions, 1, "statement: %s", sql)
+ report, err := suggest.Advise(tc.sql)
+ require.NoError(t, err, "statement: %s", tc.sql)
+ require.Len(t, report.Suggestions, 1, "statement: %s", tc.sql)
s := report.Suggestions[0]
- assert.Empty(t, s.Recommended, "statement: %s", sql)
- assert.Equal(t, suggest.GuidanceNameConstraintThenValidate, s.Guidance, "statement: %s", sql)
+ assert.Equal(t, tc.recommended, s.Recommended, "statement: %s", tc.sql)
+ assert.Empty(t, s.Guidance, "statement: %s", tc.sql)
}
}
+// Two unnamed CHECKs on one column in one script: running the script, the
+// server names the second past the first, so the advice does too and its
+// two sequences never collide.
+func TestAdviseUnnamedChecksInOneScriptGetDistinctNames(t *testing.T) {
+ report, err := suggest.Advise("ALTER TABLE t ADD CHECK (qty > 0); ALTER TABLE t ADD CHECK (qty < 100);")
+ require.NoError(t, err)
+ require.Len(t, report.Suggestions, 2)
+ assert.Equal(t, []string{
+ "ALTER TABLE t ADD CONSTRAINT t_qty_check CHECK (qty > 0) NOT VALID",
+ `ALTER TABLE "t" VALIDATE CONSTRAINT "t_qty_check"`,
+ }, report.Suggestions[0].Recommended)
+ assert.Equal(t, []string{
+ "ALTER TABLE t ADD CONSTRAINT t_qty_check1 CHECK (qty < 100) NOT VALID",
+ `ALTER TABLE "t" VALIDATE CONSTRAINT "t_qty_check1"`,
+ }, report.Suggestions[1].Recommended)
+}
+
// Following add-column-then-constraint literally must not land on a second
// refusal: the plain column add is clean, and the separate, named ADD
// CONSTRAINT the guidance describes gets a constructed rewrite — not
// guidance again. This chains the two hops for both inline forms whose
-// unnamed ADD CONSTRAINT counterpart is itself refused.
+// counterpart takes the NOT VALID → VALIDATE rewrite.
func TestAdviseAddColumnGuidanceChainEndsInConstructedRewrite(t *testing.T) {
for _, tc := range []struct {
inline string