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