diff --git a/CHANGELOG.md b/CHANGELOG.md index 13c26bb..baaca2d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,23 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Changed — observable outcomes for automation callers +- **Index builds on partitioned parents now refuse with exit 2 and reason + `unsupported-partitioned-parent`** instead of reaching a mid-change exit-1 + PostgreSQL failure. Concurrent and blocking builds carry distinct typed + causes; blocking builds are refused by policy even though PostgreSQL supports + them. Leaf partitions retain the blocking-to-`CONCURRENTLY` substitution + described below. +- **Partitioned-parent `ADD CONSTRAINT ... USING INDEX` now refuses before + execution** on every supported PostgreSQL version, with typed cause + `parent-index-adoption`. +- **Partitioned-parent foreign keys added `NOT VALID` refuse on PostgreSQL + before 18** with reason `unsupported-partitioned-parent`; PostgreSQL 18 and + later retain the supported in-place path. +- **Dry-run and declarative diff reports now account for partitioned-parent + admission**, reporting disposition `refuse` and reason + `unsupported-partitioned-parent` instead of advertising execution. Refused + statements now carry the reason directly and omit impossible safer-SQL advice. + - **A plain (blocking) `CREATE INDEX` now succeeds instead of refusing.** The engine substitutes `CREATE INDEX CONCURRENTLY` and drives it to a verified valid index: exit 0, with the substitution disclosed in the diff --git a/docs/README.md b/docs/README.md index 8ce469c..a7d3de5 100644 --- a/docs/README.md +++ b/docs/README.md @@ -33,6 +33,7 @@ combination is the product is [vision.md](vision.md); start there. | [invariants.md](invariants.md) | The canonical **invariant registry** — testable runtime MUST-statements (correctness, locking, state/resume, refusals, orchestration), each with its enforcement point and source. Mined from this doc set plus [Spirit](https://github.com/block/spirit)'s stated safety invariants and [SchemaBot](https://github.com/block/schemabot)'s control-plane discipline; the build plan's phases carry per-invariant test obligations. | | [tcb-model.md](tcb-model.md) | The **TCB model** — the trusted-computing-base partition of the engine: which components are the small trusted core that enforces the invariant registry vs the untrusted periphery, the never-trust-callers rule, domain types that make illegal states unrepresentable, the in-TCB engineering rules (from TigerBeetle TIGER_STYLE, s2n-tls, qmail, bitcoin-core), the verification ladder, and the per-side AI-assisted development policy. | | [plan-report.md](plan-report.md) | The **plan report contract** — the versioned JSON shape both front doors emit for dry-run plans: fields, closed vocabularies, the fingerprint identity, required consumer behavior for unknown versions/values, and one generated example per source (pinned by test). | +| [limitations.md](limitations.md) | The **current limitations** — schema changes pg-sprite refuses today, why they are unsafe or unsupported, and where an operator must act outside the engine. | | [lint-report.md](lint-report.md) | The **lint report contract** — the versioned JSON shape `pg-sprite lint` emits for offline CI gating: finding fields (verbatim SQL, line/column), the codes table, severities and exit behavior, the offline-conservatism rules, and how the contract versions relative to the plan report. | | [suggest-report.md](suggest-report.md) | The **suggest report contract** — the versioned JSON shape `pg-sprite suggest` emits for offline advice: the typed caveat vocabulary (what changes about how you must run a safer form, and what a failed step leaves behind), the typed guidance codes for rewrites the planner cannot construct, and the operation → safer form → caveats table (pinned by test). | | [engine-role.md](engine-role.md) | The **engine-role provisioning contract** — the tiered minimum access a PostgreSQL user needs to run schema changes against tables it does not own: role membership for owner-gated DDL, schema `CREATE` for index builds and shadow objects, `SET ROLE` for owner-correct shadow creation, replication access for CDC, and the explicit list of powers the engine role must *not* have. Preflight refusals name the missing `GRANT` and point here. | diff --git a/docs/engine-role.md b/docs/engine-role.md index c856843..b3b977b 100644 --- a/docs/engine-role.md +++ b/docs/engine-role.md @@ -103,3 +103,8 @@ evaluates the tier checks for the change's planned strategy. A missing requireme typed refusal naming the exact `GRANT` statement that would satisfy it — the same fail-closed posture as every other refusal in the engine, and the reason this page exists: the refusal points here, and this page says what to provision and why it is safe. + +Preflight also records the target's relation kind. Leaf partitions are ordinary tables and +use the same paths as any other table. A partitioned parent may run supported in-place changes; +the parent-specific refusal boundaries and their rationale are documented in +[current limitations](limitations.md). diff --git a/docs/invariants.md b/docs/invariants.md index 2e04a40..ff2f0d7 100644 --- a/docs/invariants.md +++ b/docs/invariants.md @@ -267,6 +267,11 @@ Each refusal is a preflight **error with a stated reason** — never a warning, - **RF-5** — The dangerous literal never runs silently: risky statements with a safer native idiom get the idiom (reported) or a recommendation; running as-submitted requires the loud, typed, audited `--force`. *Source:* [high-level-design § advisory mode](high-level-design.md#advisory-mode-suggest-the-safe-rewrite-dont-silently-run-the-risky-one). +- **RF-6** — A partitioned-parent sequence is refused before its first step when it would build + an index, or on PostgreSQL before 18 when it would add a foreign key `NOT VALID`. pg-sprite + does not substitute a blocking parent build for the missing partition-aware online flow. + *Enforced:* preflight and sequence-executor admission. *Source:* PostgreSQL relation-kind and + version capabilities. ## Orchestration / control-plane (OC) @@ -330,7 +335,7 @@ about **how we write and review the code**. | Invariant | Landed by phase | Test obligation | | --- | --- | --- | -| CO-7, RF-1..RF-5 | 1–2 (gate/linter), full at 2 | golden refusal/parse tests | +| CO-7, RF-1..RF-6 | 1–3 (gate/linter/executor) | golden refusal/parse and executor-admission tests | | LK-1 | 0–1 (before any executing mode ships) | two-instance mutual-exclusion + keepalive-loss test | | LK-2 | 3 (native), 7 (cutover) | lock-bounding + CIC-exception tests | | CO-1, CO-2, CO-3 | 5 (gate), 8 (watermark/divergence policy) | inject-divergence, repair-invalidates-watermark | diff --git a/docs/limitations.md b/docs/limitations.md new file mode 100644 index 0000000..5a2998c --- /dev/null +++ b/docs/limitations.md @@ -0,0 +1,11 @@ +# Current limitations + +pg-sprite refuses a schema change when it cannot provide its online-safety +guarantees. These are current capability boundaries, not escape hatches: + +| Change | Current behavior | +| --- | --- | +| Index builds on a partitioned parent | PostgreSQL cannot build an index concurrently at the parent level. PostgreSQL supports a plain blocking build, but pg-sprite refuses it by policy because it takes `ACCESS EXCLUSIVE`; `--force` does not bypass this decision. An operator who chooses a maintenance-window blocking build must run it outside pg-sprite. The partition-aware `CREATE INDEX ON ONLY` → per-partition `CREATE INDEX CONCURRENTLY` → `ATTACH PARTITION` flow is planned but not yet implemented. | +| `ADD CONSTRAINT ... USING INDEX` on a partitioned parent | PostgreSQL does not support adopting an existing index on a partitioned parent in any supported version. pg-sprite refuses before execution. | +| `ADD FOREIGN KEY ... NOT VALID` on a partitioned parent | PostgreSQL does not support this before version 18, so pg-sprite refuses it on versions 14–17. It is supported on version 18 and later. | +| Copy-and-swap | The copy-and-swap backend is not yet available. Statements that require it route to `refuse`; pg-sprite never falls through to a blocking rewrite. | diff --git a/docs/plan-report.md b/docs/plan-report.md index 15e023f..592fbd0 100644 --- a/docs/plan-report.md +++ b/docs/plan-report.md @@ -12,7 +12,7 @@ the examples at the end of this page. Every report carries `format_version`. A consumer that does not recognize the version must **reject the report** — never guess at field semantics. The version covers more than the -field shape: the closed vocabularies below (sources, routes, reasons, backends, dispositions, +field shape: the closed vocabularies below (sources, routes, backends, dispositions, kinds) and the fingerprint serialization are all pinned to it. Adding a vocabulary value or changing the fingerprint definition is a contract change and bumps `format_version`, even if no field is added or renamed. @@ -23,10 +23,11 @@ two version independently. Lint findings embed this contract's Reasons vocabular ## Consumer behavior for unknown values -Every enum field in the report draws from a closed vocabulary listed here. A consumer that -meets a value it does not recognize must **treat the statement as unknown and refuse it** — -never ignore it and proceed. This is the same fail-closed posture the engine itself takes -with SQL it does not fully understand. +Except for target-dependent refusal `reason` fields, every enum field in the report draws +from a closed vocabulary listed here. Refusal reasons are open and owned by `pkg/verdict`. +A consumer that meets any value it does not recognize must **treat the statement as unknown +and refuse it** — never ignore it and proceed. This is the same fail-closed posture the +engine itself takes with SQL it does not fully understand. ## Report fields @@ -39,6 +40,7 @@ with SQL it does not fully understand. | `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 | diff source only | Whether the live table was found. Absent means "not introspected" (alter source); `false` means the plan is the full desired schema. | | `disposition` | string | always | Aggregate disposition across all statements (see Dispositions). | +| `reason` | string | target-dependent refusal only | Aggregate typed refusal cause when target facts override an otherwise executable route. | | `fingerprint` | string | always | The plan's stable identity (see Fingerprint). | | `statements` | array | always | The ordered plan; `[]` (never `null`) means nothing to do. | @@ -52,6 +54,7 @@ with SQL it does not fully understand. | `route` | string | always | The planner's aggregate route for the statement (see Routes). | | `backend` | string | except refusals | The assigned execution strategy (see Backends); absent for refusals. | | `disposition` | string | always | What execution would do with this statement now (see Dispositions). | +| `reason` | string | target-dependent refusal only | Typed refusal cause for this statement; currently `unsupported-partitioned-parent`. An unknown value must be treated as refused. | | `decisions` | array | always | The planner's per-operation classifications (below). | | `exec_sql` | array | native route | The ordered SQL the native backend would run — the safer sequence when the planner constructed one. Absent for non-native routes. | | `execution` | string | with `exec_sql` | The typed execution contract for `exec_sql` (see Execution contracts). A consumer that runs the statements itself branches on this — it is what says the steps must not be wrapped in a transaction block. Present exactly when `exec_sql` is. | @@ -85,7 +88,7 @@ with SQL it does not fully understand. | `copy-and-swap` | Needs a table rewrite; only the engine's shadow copy + cutover can do it online. | | `refuse` | No known safe path; not executed. | -### Reasons (`reason`) +### Planner decision reasons (`decisions[].reason`) | Value | Meaning | |---|---| @@ -102,6 +105,19 @@ with SQL it does not fully understand. | `partition-parent-lock` | Partition attach/detach in its lock-taking form. | | `unsupported-operation` | The planner does not recognize the operation or knows no safe path for it. | +### Target-dependent refusal reasons (`reason`, `statements[].reason`) + +This vocabulary is owned by `pkg/verdict` and is open: new values may appear without changing +the meaning of existing values. A consumer that sees an unknown reason must fail closed and +treat the statement and report as refused. + +| Value | Meaning | +|---|---| +| `unsupported-partitioned-parent` | Target facts show that the statement cannot run safely on a partitioned parent. | + +On the apply path, refusal checks have deterministic precedence: table size, then partition +support, then privileges. + ### Backends (`backend`) | Value | Meaning | diff --git a/internal/cli/dryrun.go b/internal/cli/dryrun.go index a7eeb83..c90eebd 100644 --- a/internal/cli/dryrun.go +++ b/internal/cli/dryrun.go @@ -10,6 +10,7 @@ import ( "github.com/block/pg-sprite/pkg/dbconn" "github.com/block/pg-sprite/pkg/plan" "github.com/block/pg-sprite/pkg/planner" + "github.com/block/pg-sprite/pkg/preflight" "github.com/block/pg-sprite/pkg/router" "github.com/block/pg-sprite/pkg/schemadiff" "github.com/block/pg-sprite/pkg/statement" @@ -35,7 +36,7 @@ func (c *MigrateCmd) runDryRun(ctx context.Context, out io.Writer) error { } defer pool.Close() - facts, err := dryRunFacts(ctx, pool, st) + facts, targetFacts, err := dryRunFacts(ctx, pool, st) if err != nil { return err } @@ -64,6 +65,18 @@ func (c *MigrateCmd) runDryRun(ctx context.Context, out io.Writer) error { for _, rs := range routed.Statements { report.Statements = append(report.Statements, plan.FromRouted(rs)) } + if targetFacts.Partitioned() { + refused := make([]bool, len(report.Statements)) + for i := range report.Statements { + var cause preflight.PartitionRefusalCause + cause, err = preflight.RefusesPartitionedParent(targetFacts.ServerMajor(), report.Statements[i].ExecSQL) + if err != nil { + return err + } + refused[i] = cause != "" + } + plan.RefuseUnsupportedPartitionedParent(&report, refused) + } report.Fingerprint = plan.Fingerprint(report.Statements) if c.JSON { @@ -92,16 +105,20 @@ func resolvedSchema(st statement.Statement) string { // dryRunFacts introspects the statement's target table for classifier // facts. Statements without a single table target (index drops, REINDEX) // and missing tables classify with zero facts. -func dryRunFacts(ctx context.Context, pool *pgxpool.Pool, st statement.Statement) (planner.Facts, error) { +func dryRunFacts(ctx context.Context, pool *pgxpool.Pool, st statement.Statement) (planner.Facts, preflight.TargetFacts, error) { if st.Table() == "" { - return planner.Facts{}, nil + return planner.Facts{}, preflight.TargetFacts{}, nil } live, err := schemadiff.Introspect(ctx, pool, resolvedSchema(st), st.Table()) switch { case errors.Is(err, schemadiff.ErrTableNotFound): - return planner.Facts{}, nil + return planner.Facts{}, preflight.TargetFacts{}, nil case err != nil: - return planner.Facts{}, err + return planner.Facts{}, preflight.TargetFacts{}, err + } + targetFacts, err := preflight.LookupTargetFacts(ctx, pool, resolvedSchema(st), st.Table()) + if err != nil { + return planner.Facts{}, preflight.TargetFacts{}, err } - return planner.FactsFrom(live), nil + return planner.FactsFrom(live), targetFacts, nil } diff --git a/internal/cli/dryrun_integration_test.go b/internal/cli/dryrun_integration_test.go index 4ce0d84..195b63a 100644 --- a/internal/cli/dryrun_integration_test.go +++ b/internal/cli/dryrun_integration_test.go @@ -14,6 +14,7 @@ import ( "github.com/block/pg-sprite/pkg/plan" "github.com/block/pg-sprite/pkg/planner" "github.com/block/pg-sprite/pkg/router" + "github.com/block/pg-sprite/pkg/verdict" ) // dryRunPlan runs migrate --dry-run --json and decodes the plan report. @@ -57,6 +58,25 @@ func TestMigrateDryRunRoutesRewriteWithoutExecuting(t *testing.T) { assert.Equal(t, "integer", typ, "dry-run must not execute the change") } +func TestMigrateDryRunRefusesPartitionedParentIndexPlan(t *testing.T) { + url := testutil.StartPostgres(t) + pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: url}) + require.NoError(t, err) + defer pool.Close() + schema := createPartitionFixture(t, pool) + report := dryRunPlan(t, url, fmt.Sprintf("CREATE INDEX p_v_idx ON %s.p (v)", schema)) + assert.Equal(t, router.DispositionRefuse, report.Disposition) + assert.Equal(t, verdict.ReasonUnsupportedPartitionedParent, report.Reason) + require.Len(t, report.Statements, 1) + assert.Equal(t, router.DispositionRefuse, report.Statements[0].Disposition) + assert.Equal(t, verdict.ReasonUnsupportedPartitionedParent, report.Statements[0].Reason) + assert.Empty(t, report.Statements[0].ExecSQL) + for _, decision := range report.Statements[0].Decisions { + assert.Empty(t, decision.SaferSQL) + assert.Empty(t, decision.SaferSQLExecution) + } +} + // Live facts feed the imperative dry-run: a widen the classifier can only // prove with the live column type routes native, and still executes nothing. func TestMigrateDryRunUsesLiveFacts(t *testing.T) { diff --git a/internal/cli/migrate.go b/internal/cli/migrate.go index bc8eea1..fd5a13c 100644 --- a/internal/cli/migrate.go +++ b/internal/cli/migrate.go @@ -58,7 +58,7 @@ func (c *MigrateCmd) run(ctx context.Context, out io.Writer) error { } } - facts, err := dryRunFacts(ctx, pool, st) + facts, _, err := dryRunFacts(ctx, pool, st) if err != nil { return err } @@ -188,6 +188,29 @@ func (c *MigrateCmd) auditForce(st statement.Statement, rs router.Statement) { func (c *MigrateCmd) execute(ctx context.Context, out io.Writer, pool *pgxpool.Pool, st statement.Statement, execSQL []string, plan planner.Plan, substituted, forced bool, logger *slog.Logger) error { + limit := int64(c.MaxTableSize) + if !sizeGuardApplies(plan, substituted) { + limit = preflight.NoSizeLimit + } + pt, err := preflight.CheckTable(ctx, pool, st.Schema(), st.Table(), limit) + var sizeErr *preflight.SizeError + if errors.As(err, &sizeErr) { + return c.emit(out, sizeGuardVerdict(st, sizeErr, forced)) + } + if err != nil { + return err + } + serverMajor, err := dbconn.ServerMajor(ctx, pool) + if err != nil { + return err + } + if err := preflight.CheckPartitionSupport(pt, serverMajor, execSQL); err != nil { + var partitionErr *preflight.UnsupportedPartitionedParentError + if errors.As(err, &partitionErr) { + return c.emit(out, partitionedParentVerdict(st, partitionErr, forced)) + } + return err + } tier, err := preflight.RequiredTier(execSQL) if err != nil { return err @@ -203,19 +226,6 @@ func (c *MigrateCmd) execute(ctx context.Context, out io.Writer, pool *pgxpool.P } logger.Debug("privilege preflight passed", "role", priv.Role(), "owner", priv.Owner(), "tier", tier.String()) - - limit := int64(c.MaxTableSize) - if !sizeGuardApplies(plan, substituted) { - limit = preflight.NoSizeLimit - } - pt, err := preflight.CheckTable(ctx, pool, st.Schema(), st.Table(), limit) - var sizeErr *preflight.SizeError - if errors.As(err, &sizeErr) { - return c.emit(out, sizeGuardVerdict(st, sizeErr, forced)) - } - if err != nil { - return err - } logger.Debug("preflight passed", "table", qualified(st), "total_bytes", pt.TotalBytes(), "limit_bytes", limit) if substituted { @@ -471,6 +481,20 @@ func privilegeVerdict(st statement.Statement, privErr *preflight.PrivilegeError, } } +// partitionedParentVerdict refuses unsupported execution steps on a +// partitioned parent before the sequence executor runs anything. +func partitionedParentVerdict(st statement.Statement, partitionErr *preflight.UnsupportedPartitionedParentError, + forced bool) verdict.Verdict { + return verdict.Verdict{ + Outcome: verdict.OutcomeRefused, + Reason: verdict.ReasonUnsupportedPartitionedParent, + Statement: st.SQL(), + Table: qualified(st), + Forced: forced, + Detail: partitionErr.Error(), + } +} + // rewriteRequiredVerdict is the refusal for a statement whose submitted // form blocks but for which the planner could not construct the safer // native sequence — a multi-operation statement, or a pattern it cannot diff --git a/internal/cli/migrate_integration_test.go b/internal/cli/migrate_integration_test.go index 70eb122..0b21346 100644 --- a/internal/cli/migrate_integration_test.go +++ b/internal/cli/migrate_integration_test.go @@ -14,6 +14,7 @@ import ( "time" "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgxpool" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -66,6 +67,153 @@ func TestMigrateExecutesInstantChange(t *testing.T) { assert.Equal(t, "integer", typ) } +// Partitioned parents need a partition-aware online index flow. Until that +// flow exists, preflight refuses every routed index-building shape while +// preserving ordinary behavior for leaf partitions and parent ALTERs that +// do not build indexes. +func TestMigratePartitionedTableAwareness(t *testing.T) { + url := testutil.StartPostgres(t) + pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: url}) + require.NoError(t, err) + defer pool.Close() + + t.Run("parent explicit concurrent index", func(t *testing.T) { + schema := createPartitionFixture(t, pool) + assertPartitionedParentRefusal(t, url, pool, schema, + fmt.Sprintf("CREATE INDEX CONCURRENTLY p_v_idx ON %s.p (v)", schema)) + }) + + t.Run("forced parent plain index", func(t *testing.T) { + schema := createPartitionFixture(t, pool) + sql := fmt.Sprintf("CREATE INDEX p_v_idx ON %s.p (v)", schema) + cmd := newMigrateCmd(url, sql) + cmd.JSON = true + cmd.Force = schema + ".p" + var out strings.Builder + err := cmd.run(t.Context(), &out) + require.ErrorIs(t, err, verdict.ErrRefused) + var v verdict.Verdict + require.NoError(t, json.Unmarshal([]byte(out.String()), &v)) + assert.Equal(t, verdict.ReasonUnsupportedPartitionedParent, v.Reason) + assert.True(t, v.Forced) + }) + + t.Run("parent unique constraint", func(t *testing.T) { + schema := createPartitionFixture(t, pool) + assertPartitionedParentRefusal(t, url, pool, schema, + fmt.Sprintf("ALTER TABLE %s.p ADD CONSTRAINT p_id_unique UNIQUE (id)", schema)) + }) + + t.Run("parent using index adoption", func(t *testing.T) { + schema := createPartitionFixture(t, pool) + _, err := pool.Exec(t.Context(), fmt.Sprintf("CREATE UNIQUE INDEX ix_p_id ON %s.p (id)", schema)) + require.NoError(t, err) + cmd := newMigrateCmd(url, + fmt.Sprintf("ALTER TABLE %s.p ADD CONSTRAINT p_pk PRIMARY KEY USING INDEX ix_p_id", schema)) + cmd.JSON = true + var out strings.Builder + err = cmd.run(t.Context(), &out) + require.ErrorIs(t, err, verdict.ErrRefused) + var v verdict.Verdict + require.NoError(t, json.Unmarshal([]byte(out.String()), &v)) + assert.Equal(t, verdict.OutcomeRefused, v.Outcome) + assert.Equal(t, verdict.ReasonUnsupportedPartitionedParent, v.Reason) + var constraints int + require.NoError(t, pool.QueryRow(t.Context(), ` + SELECT count(*) FROM pg_constraint + WHERE conrelid = to_regclass($1) AND conname = 'p_pk'`, schema+".p").Scan(&constraints)) + assert.Zero(t, constraints, "refusal must not adopt the index") + }) + + t.Run("leaf index-building changes execute", func(t *testing.T) { + schema := createPartitionFixture(t, pool) + for _, sql := range []string{ + fmt.Sprintf("CREATE INDEX CONCURRENTLY p1_v_idx ON %s.p1 (v)", schema), + fmt.Sprintf("ALTER TABLE %s.p1 ADD CONSTRAINT p1_id_unique UNIQUE (id)", schema), + } { + cmd := newMigrateCmd(url, sql) + 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) + } + }) + + t.Run("parent add column executes", func(t *testing.T) { + schema := createPartitionFixture(t, pool) + cmd := newMigrateCmd(url, fmt.Sprintf("ALTER TABLE %s.p ADD COLUMN added int", schema)) + 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) + var columns int + require.NoError(t, pool.QueryRow(t.Context(), ` + SELECT count(*) FROM information_schema.columns + WHERE table_schema = $1 AND table_name IN ('p', 'p1') AND column_name = 'added'`, schema).Scan(&columns)) + assert.Equal(t, 2, columns) + }) + + t.Run("parent not valid foreign key follows server support", func(t *testing.T) { + schema := createPartitionFixture(t, pool) + _, err := pool.Exec(t.Context(), fmt.Sprintf("CREATE TABLE %s.ref (id int PRIMARY KEY)", schema)) + require.NoError(t, err) + cmd := newMigrateCmd(url, fmt.Sprintf( + "ALTER TABLE %s.p ADD CONSTRAINT p_ref_fk FOREIGN KEY (id) REFERENCES %s.ref(id) NOT VALID", schema, schema)) + cmd.JSON = true + var out strings.Builder + err = cmd.run(t.Context(), &out) + major, majorErr := dbconn.ServerMajor(t.Context(), pool) + require.NoError(t, majorErr) + var v verdict.Verdict + require.NoError(t, json.Unmarshal([]byte(out.String()), &v)) + if major < 18 { + require.ErrorIs(t, err, verdict.ErrRefused) + assert.Equal(t, verdict.ReasonUnsupportedPartitionedParent, v.Reason) + } else { + require.NoError(t, err) + assert.Equal(t, verdict.OutcomeExecuted, v.Outcome) + } + }) +} + +func createPartitionFixture(t *testing.T, pool *pgxpool.Pool) string { + t.Helper() + schema := testutil.NewSchema(t, pool) + _, err := pool.Exec(t.Context(), fmt.Sprintf( + "CREATE TABLE %s.p (id int, v text) PARTITION BY RANGE (id)", schema)) + require.NoError(t, err) + _, err = pool.Exec(t.Context(), fmt.Sprintf( + "CREATE TABLE %s.p1 PARTITION OF %s.p FOR VALUES FROM (0) TO (100)", schema, schema)) + require.NoError(t, err) + return schema +} + +func assertPartitionedParentRefusal(t *testing.T, url string, pool *pgxpool.Pool, schema, sql string) { + t.Helper() + cmd := newMigrateCmd(url, sql) + cmd.JSON = true + var out strings.Builder + err := cmd.run(t.Context(), &out) + require.ErrorIs(t, err, verdict.ErrRefused) + + var v verdict.Verdict + require.NoError(t, json.Unmarshal([]byte(out.String()), &v)) + assert.Equal(t, verdict.OutcomeRefused, v.Outcome) + assert.Equal(t, verdict.ReasonUnsupportedPartitionedParent, v.Reason) + assert.Empty(t, v.ExecutedSQL) + + var indexes int + require.NoError(t, pool.QueryRow(t.Context(), ` + SELECT count(*) FROM pg_indexes + WHERE schemaname = $1 AND tablename IN ('p', 'p1')`, schema).Scan(&indexes)) + assert.Zero(t, indexes, "preflight refusal must execute no index-building step") +} + // ALTER TABLE ... RENAME COLUMN parses as a RenameStmt, not an // AlterTableStmt, but is a table-targeted instant catalog change: the front // door must route it through, not refuse it as unsupported. diff --git a/pkg/dbconn/dbconn.go b/pkg/dbconn/dbconn.go index da118e1..915785d 100644 --- a/pkg/dbconn/dbconn.go +++ b/pkg/dbconn/dbconn.go @@ -107,6 +107,15 @@ func ServerVersion(ctx context.Context, pool *pgxpool.Pool) (string, error) { return v, nil } +// ServerMajor reads the connected server's numeric major version. +func ServerMajor(ctx context.Context, pool *pgxpool.Pool) (int, error) { + var major int + if err := pool.QueryRow(ctx, "SELECT current_setting('server_version_num')::int / 10000").Scan(&major); err != nil { + return 0, fmt.Errorf("read server major version: %w", err) + } + return major, nil +} + // buildPoolConfig translates Config into a pgxpool configuration. It is pure // (no dialing), so every option's wiring is unit-testable without a server. func buildPoolConfig(cfg Config) (*pgxpool.Config, error) { diff --git a/pkg/diffplan/diffplan.go b/pkg/diffplan/diffplan.go index 628ac1e..daadf75 100644 --- a/pkg/diffplan/diffplan.go +++ b/pkg/diffplan/diffplan.go @@ -25,6 +25,7 @@ import ( "github.com/block/pg-sprite/pkg/dbconn" "github.com/block/pg-sprite/pkg/plan" "github.com/block/pg-sprite/pkg/planner" + "github.com/block/pg-sprite/pkg/preflight" "github.com/block/pg-sprite/pkg/router" "github.com/block/pg-sprite/pkg/schemadiff" "github.com/block/pg-sprite/pkg/statement" @@ -98,6 +99,23 @@ func Plan(ctx context.Context, pool *pgxpool.Pool, req Request) (plan.Report, er if report.Statements, report.Disposition, err = classifyChanges(changes, facts); err != nil { return plan.Report{}, err } + if tableExists { + targetFacts, checkErr := preflight.LookupTargetFacts(ctx, pool, req.Schema, ds.Table()) + if checkErr != nil { + return plan.Report{}, checkErr + } + if targetFacts.Partitioned() { + refused := make([]bool, len(report.Statements)) + for i := range report.Statements { + cause, causeErr := preflight.RefusesPartitionedParent(targetFacts.ServerMajor(), report.Statements[i].ExecSQL) + if causeErr != nil { + return plan.Report{}, causeErr + } + refused[i] = cause != "" + } + plan.RefuseUnsupportedPartitionedParent(&report, refused) + } + } report.Fingerprint = plan.Fingerprint(report.Statements) return report, nil } diff --git a/pkg/diffplan/diffplan_integration_test.go b/pkg/diffplan/diffplan_integration_test.go index baa2199..ce6d539 100644 --- a/pkg/diffplan/diffplan_integration_test.go +++ b/pkg/diffplan/diffplan_integration_test.go @@ -15,6 +15,7 @@ import ( "github.com/block/pg-sprite/pkg/router" "github.com/block/pg-sprite/pkg/schemadiff" "github.com/block/pg-sprite/pkg/statement" + "github.com/block/pg-sprite/pkg/verdict" ) // parseDesired parses a desired-state schema the way a library caller would @@ -89,6 +90,32 @@ func TestPlanDerivesOrderedRoutedPlan(t *testing.T) { "the same desired state against the same live table plans deterministically") } +func TestPlanRefusesPartitionedParentIndexChange(t *testing.T) { + url := testutil.StartPostgres(t) + pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: url}) + require.NoError(t, err) + defer pool.Close() + schema := testutil.NewSchema(t, pool) + _, err = pool.Exec(t.Context(), fmt.Sprintf( + "CREATE TABLE %s.events (id bigint, name text) PARTITION BY RANGE (id); "+ + "CREATE TABLE %s.events_1 PARTITION OF %s.events FOR VALUES FROM (0) TO (100)", schema, schema, schema)) + require.NoError(t, err) + ds := parseDesired(t, + "CREATE TABLE events (id bigint, name text) PARTITION BY RANGE (id);\n"+ + "CREATE INDEX events_name_idx ON events (name);") + report, err := diffplan.Plan(t.Context(), pool, diffplan.Request{Schema: schema, Desired: ds}) + require.NoError(t, err) + assert.Equal(t, router.DispositionRefuse, report.Disposition) + assert.Equal(t, verdict.ReasonUnsupportedPartitionedParent, report.Reason) + require.Len(t, report.Statements, 1) + assert.Equal(t, verdict.ReasonUnsupportedPartitionedParent, report.Statements[0].Reason) + assert.Empty(t, report.Statements[0].ExecSQL) + for _, decision := range report.Statements[0].Decisions { + assert.Empty(t, decision.SaferSQL) + assert.Empty(t, decision.SaferSQLExecution) + } +} + // A desired state that needs a table rewrite routes to the copy-and-swap // backend, and the routed plan says that backend is unavailable in this // build — a library caller sees the same honest refusal the CLI prints. diff --git a/pkg/executor/code.go b/pkg/executor/code.go index 8ab03cf..7780812 100644 --- a/pkg/executor/code.go +++ b/pkg/executor/code.go @@ -41,6 +41,9 @@ const ( // CodeUnsupportedSequenceStep: a step is not a shape the sequence // executor can run safely. CodeUnsupportedSequenceStep Code = "unsupported-sequence-step" + // CodeUnsupportedPartitionedParent identifies partitioned-parent + // admission refusals. + CodeUnsupportedPartitionedParent Code = "unsupported-partitioned-parent" // CodeNotConcurrentIndexBuild: the statement handed to the concurrent // build executor is not a CREATE INDEX CONCURRENTLY. CodeNotConcurrentIndexBuild Code = "not-concurrent-index-build" @@ -106,6 +109,8 @@ func sentinelCode(err error) Code { return CodeEmptySequence case errors.Is(err, ErrUnsupportedSequenceStep): return CodeUnsupportedSequenceStep + case errors.Is(err, ErrUnsupportedPartitionedParent): + return CodeUnsupportedPartitionedParent case errors.Is(err, ErrNotConcurrentIndexBuild): return CodeNotConcurrentIndexBuild case errors.Is(err, ErrUnnamedIndex): diff --git a/pkg/executor/sequence.go b/pkg/executor/sequence.go index b719730..f8aea86 100644 --- a/pkg/executor/sequence.go +++ b/pkg/executor/sequence.go @@ -51,6 +51,9 @@ var ( // DETACH PARTITION CONCURRENTLY leaves a detach-pending partition // state this executor does not own detecting or recovering. ErrUnsupportedSequenceStep = errors.New("step is not a shape the sequence executor can run safely") + // ErrUnsupportedPartitionedParent is returned when the target is a + // partitioned parent and the admitted sequence is not supported there. + ErrUnsupportedPartitionedParent = errors.New("sequence is not supported on a partitioned parent") ) // StepKind is the typed execution class a step was admitted under; @@ -234,6 +237,19 @@ func RunSequence(ctx context.Context, pool *pgxpool.Pool, pt preflight.Preflight if err != nil { return rep, err } + partitioned, serverMajor, err := sequenceTargetFacts(ctx, pool, pt.Schema(), pt.Table()) + if err != nil { + return rep, err + } + if partitioned { + cause, checkErr := preflight.RefusesPartitionedParent(serverMajor, steps) + if checkErr != nil { + return rep, checkErr + } + if cause != "" { + return rep, fmt.Errorf("%s: %w", cause, ErrUnsupportedPartitionedParent) + } + } // INV: LK-2 — the concurrent executor's pool guard is re-proven for // the whole sequence before the first step executes: a too-small pool // is decidable now, and letting BuildIndexConcurrently discover it @@ -278,6 +294,19 @@ func RunSequence(ctx context.Context, pool *pgxpool.Pool, pt preflight.Preflight return rep, nil } +// sequenceTargetFacts re-verifies the relation kind and server version at +// executor admission rather than trusting a caller's earlier classification. +func sequenceTargetFacts(ctx context.Context, pool *pgxpool.Pool, schema, table string) (bool, int, error) { + facts, err := preflight.LookupTargetFacts(ctx, pool, schema, table) + if errors.Is(err, preflight.ErrTableNotFound) { + return false, 0, fmt.Errorf("admit sequence target %s: %w", qualifiedName(schema, table), ErrTableNotFound) + } + if err != nil { + return false, 0, fmt.Errorf("admit sequence target %s: %w", qualifiedName(schema, table), err) + } + return facts.Partitioned(), facts.ServerMajor(), nil +} + // sequenceHasConcurrentBuild reports whether any admitted step is a // concurrent index build — the class whose executor needs the two-connection // pool guarantee. diff --git a/pkg/executor/sequence_integration_test.go b/pkg/executor/sequence_integration_test.go index f517cb5..b821f85 100644 --- a/pkg/executor/sequence_integration_test.go +++ b/pkg/executor/sequence_integration_test.go @@ -411,3 +411,35 @@ func TestRunSequenceRefusesSingleConnectionPoolBeforeAnyStep(t *testing.T) { exists, _ := constraintState(t, pool, schema, "t", "v_positive") assert.False(t, exists, "nothing may execute when the sequence cannot finish") } + +func TestRunSequenceRefusesPartitionedParentBeforeAnyStep(t *testing.T) { + pool, schema := newPool(t) + _, err := pool.Exec(t.Context(), fmt.Sprintf( + "CREATE TABLE %s.p (id int, v int) PARTITION BY RANGE (id); "+ + "CREATE TABLE %s.p1 PARTITION OF %s.p FOR VALUES FROM (0) TO (100)", schema, schema, schema)) + require.NoError(t, err) + pt := mustPreflight(t, pool, schema, "p") + steps := []string{ + fmt.Sprintf("ALTER TABLE %s.p ADD CONSTRAINT v_positive CHECK (v > 0) NOT VALID", schema), + fmt.Sprintf("CREATE INDEX CONCURRENTLY p_v_idx ON %s.p (v)", schema), + } + _, err = executor.RunSequence(t.Context(), pool, pt, steps, runBudget, executor.DefaultRetryPolicy()) + require.ErrorIs(t, err, executor.ErrUnsupportedPartitionedParent) + exists, _ := constraintState(t, pool, schema, "p", "v_positive") + assert.False(t, exists, "partition refusal must precede every sequence step") +} + +func TestRunSequenceReportsVanishedTargetAsTableNotFound(t *testing.T) { + pool, schema := newPool(t) + _, err := pool.Exec(t.Context(), fmt.Sprintf("CREATE TABLE %s.t (id int)", schema)) + require.NoError(t, err) + pt := mustPreflight(t, pool, schema, "t") + _, err = pool.Exec(t.Context(), fmt.Sprintf("DROP TABLE %s.t", schema)) + require.NoError(t, err) + + _, err = executor.RunSequence(t.Context(), pool, pt, + []string{fmt.Sprintf("ALTER TABLE %s.t ADD COLUMN v int", schema)}, + runBudget, executor.DefaultRetryPolicy()) + require.ErrorIs(t, err, executor.ErrTableNotFound) + assert.Equal(t, executor.CodeTableNotFound, executor.OutcomeCode(err)) +} diff --git a/pkg/plan/plan.go b/pkg/plan/plan.go index f80d99b..d08b1dc 100644 --- a/pkg/plan/plan.go +++ b/pkg/plan/plan.go @@ -13,6 +13,7 @@ import ( "github.com/block/pg-sprite/pkg/planner" "github.com/block/pg-sprite/pkg/router" "github.com/block/pg-sprite/pkg/schemadiff" + "github.com/block/pg-sprite/pkg/verdict" ) // FormatVersion identifies the report contract. A consumer must reject a @@ -67,6 +68,8 @@ type Statement struct { Backend router.Backend `json:"backend,omitempty"` // Disposition is what execution would do with the statement now. Disposition router.Disposition `json:"disposition"` + // Reason is the typed cause when target facts refuse this statement. + Reason verdict.Reason `json:"reason,omitempty"` // Decisions are the planner's per-operation classifications. Decisions []planner.Decision `json:"decisions"` // ExecSQL is the ordered SQL the native backend would run — the safer @@ -107,6 +110,9 @@ type Report struct { // Disposition is the aggregate disposition across all statements: // what would happen if the engine executed this plan now. Disposition router.Disposition `json:"disposition"` + // Reason is the typed refusal cause when target facts make an otherwise + // executable routed plan unsafe. + Reason verdict.Reason `json:"reason,omitempty"` // Fingerprint is the plan's stable identity (see Fingerprint). An // approver pins it when the plan is reviewed; an executor recomputes it // at apply time and refuses on mismatch — that is how "the plan a @@ -117,6 +123,31 @@ type Report struct { Statements []Statement `json:"statements"` } +// RefuseUnsupportedPartitionedParent marks executable statements as refused +// when partition-aware admission rejects their execution steps. +func RefuseUnsupportedPartitionedParent(report *Report, refused []bool) { + any := false + for i := range report.Statements { + if i >= len(refused) || !refused[i] { + continue + } + any = true + report.Statements[i].Backend = "" + report.Statements[i].Disposition = router.DispositionRefuse + report.Statements[i].Reason = verdict.ReasonUnsupportedPartitionedParent + report.Statements[i].ExecSQL = nil + report.Statements[i].Execution = "" + for j := range report.Statements[i].Decisions { + report.Statements[i].Decisions[j].SaferSQL = nil + report.Statements[i].Decisions[j].SaferSQLExecution = "" + } + } + if any { + report.Disposition = router.DispositionRefuse + report.Reason = verdict.ReasonUnsupportedPartitionedParent + } +} + // NewReport returns an empty report for source with the contract version // stamped and Statements non-nil, so an empty plan serializes as [] rather // than null. diff --git a/pkg/plan/plan_test.go b/pkg/plan/plan_test.go index 41d30b8..939a10f 100644 --- a/pkg/plan/plan_test.go +++ b/pkg/plan/plan_test.go @@ -12,6 +12,7 @@ import ( "github.com/block/pg-sprite/pkg/planner" "github.com/block/pg-sprite/pkg/router" "github.com/block/pg-sprite/pkg/schemadiff" + "github.com/block/pg-sprite/pkg/verdict" ) func TestNewReportStampsVersionAndEmptyStatements(t *testing.T) { @@ -72,6 +73,34 @@ func TestFromRoutedDerivesDestructiveFromDecisions(t *testing.T) { assert.Empty(t, st.Execution, "no exec_sql means no execution contract") } +func TestRefuseUnsupportedPartitionedParentWithdrawsExecutionAdvice(t *testing.T) { + r := plan.Report{ + Disposition: router.DispositionExecute, + Statements: []plan.Statement{{ + Route: planner.RouteNative, + Backend: router.BackendNative, + Disposition: router.DispositionExecute, + Decisions: []planner.Decision{{ + Route: planner.RouteNative, Reason: planner.ReasonSaferIdiom, + SaferSQL: []string{"CREATE INDEX CONCURRENTLY i ON t (c)"}, + SaferSQLExecution: planner.ExecutionAutocommit, + }}, + ExecSQL: []string{"CREATE INDEX CONCURRENTLY i ON t (c)"}, Execution: planner.ExecutionAutocommit, + }}, + } + plan.RefuseUnsupportedPartitionedParent(&r, []bool{true}) + + assert.Equal(t, router.DispositionRefuse, r.Disposition) + assert.Equal(t, verdict.ReasonUnsupportedPartitionedParent, r.Reason) + assert.Equal(t, planner.RouteNative, r.Statements[0].Route) + assert.Equal(t, verdict.ReasonUnsupportedPartitionedParent, r.Statements[0].Reason) + assert.Empty(t, r.Statements[0].Backend) + assert.Empty(t, r.Statements[0].ExecSQL) + assert.Empty(t, r.Statements[0].Execution) + assert.Empty(t, r.Statements[0].Decisions[0].SaferSQL) + assert.Empty(t, r.Statements[0].Decisions[0].SaferSQLExecution) +} + // The JSON shape is the adapter-facing contract: exact keys, exact // omissions. A consumer pins format_version 1 against this test. func TestReportJSONShape(t *testing.T) { diff --git a/pkg/preflight/partition.go b/pkg/preflight/partition.go new file mode 100644 index 0000000..c1a95e0 --- /dev/null +++ b/pkg/preflight/partition.go @@ -0,0 +1,110 @@ +package preflight + +import ( + "fmt" + + "github.com/block/pg-sprite/pkg/statement" +) + +// PartitionRefusalCause identifies which unsupported shape triggered a +// partitioned-parent refusal. The zero value means the steps are supported. +type PartitionRefusalCause string + +const ( + // PartitionCauseConcurrentIndexBuild means a step attempts a concurrent + // index build on the parent. + PartitionCauseConcurrentIndexBuild PartitionRefusalCause = "parent-concurrent-index-build" + // PartitionCauseBlockingIndexBuild means a step would build an index on + // the parent while holding ACCESS EXCLUSIVE. + PartitionCauseBlockingIndexBuild PartitionRefusalCause = "parent-blocking-index-build" + // PartitionCauseIndexAdoption means a step adopts an existing index as a + // primary-key or unique constraint on the parent. + PartitionCauseIndexAdoption PartitionRefusalCause = "parent-index-adoption" + // PartitionCauseNotValidForeignKey means a step adds a NOT VALID + // foreign key, which the server version cannot do on a partitioned + // table. + PartitionCauseNotValidForeignKey PartitionRefusalCause = "parent-not-valid-foreign-key" +) + +// UnsupportedPartitionedParentError reports that an execution plan contains +// a step pg-sprite cannot safely run on a partitioned parent. Its rendered +// message is a fixed English sentence with no interpolated identifiers or +// server text, so orchestrator-facing surfaces may render it verbatim. This +// is a deliberate property to preserve. +type UnsupportedPartitionedParentError struct { + // Cause is the unsupported shape that triggered the refusal. + Cause PartitionRefusalCause +} + +// Error implements the error interface. +func (e *UnsupportedPartitionedParentError) Error() string { + switch e.Cause { + case PartitionCauseNotValidForeignKey: + return "PostgreSQL before version 18 cannot add a NOT VALID foreign key on a partitioned table; " + + "pg-sprite refuses the plan rather than failing mid-change" + case PartitionCauseIndexAdoption: + return "PostgreSQL does not support ALTER TABLE ... ADD CONSTRAINT ... USING INDEX on partitioned tables in any supported version" + case PartitionCauseBlockingIndexBuild: + return "PostgreSQL supports a blocking parent-level index build, but pg-sprite refuses to run it under ACCESS EXCLUSIVE; " + + "the partition-aware concurrent flow is not yet supported" + case PartitionCauseConcurrentIndexBuild: + return "PostgreSQL cannot build parent-level indexes concurrently; pg-sprite does not yet support " + + "the partition-aware CREATE INDEX ON ONLY, per-partition CREATE INDEX CONCURRENTLY, and ATTACH PARTITION flow" + default: + return "pg-sprite refuses an unsupported operation on a partitioned table" + } +} + +// CheckPartitionSupport verifies that the execution steps are safe +// for the target's relation kind. Ordinary tables and leaf partitions pass +// unchanged. Supported in-place parent ALTER TABLE operations remain available. +func CheckPartitionSupport(table PreflightedTable, serverMajor int, execSQL []string) error { + if !table.Partitioned() { + return nil + } + cause, err := RefusesPartitionedParent(serverMajor, execSQL) + if err != nil { + return err + } + if cause != "" { + return &UnsupportedPartitionedParentError{Cause: cause} + } + return nil +} + +// RefusesPartitionedParent reports the cause that makes steps unsupported on +// a partitioned parent, or the zero value when they are supported. It is the +// shared static policy used by preflight, plan reporting, and executor +// admission. +func RefusesPartitionedParent(serverMajor int, execSQL []string) (PartitionRefusalCause, error) { + for _, sql := range execSQL { + st, err := statement.ParseOne(sql) + if err != nil { + return "", fmt.Errorf("check partitioned-parent support: %w", err) + } + // INV: RF-6 — no partitioned-parent sequence starts when pg-sprite + // cannot complete it without an ACCESS EXCLUSIVE index build, an + // unsupported index adoption, or a server-version-unsupported NOT + // VALID foreign key. + ops, err := statement.ParseOps(sql) + if err != nil { + return "", fmt.Errorf("check partitioned-parent operations: %w", err) + } + for _, op := range ops { + if op.Kind == statement.OpAddConstraint && op.UsingIndex { + return PartitionCauseIndexAdoption, nil + } + if serverMajor < 18 && op.Kind == statement.OpAddConstraint && + op.Constraint == statement.ConstraintForeignKey && op.NotValid { + return PartitionCauseNotValidForeignKey, nil + } + } + if st.BuildsIndex() { + if st.Concurrent() { + return PartitionCauseConcurrentIndexBuild, nil + } + return PartitionCauseBlockingIndexBuild, nil + } + } + return "", nil +} diff --git a/pkg/preflight/partition_test.go b/pkg/preflight/partition_test.go new file mode 100644 index 0000000..b77cbcd --- /dev/null +++ b/pkg/preflight/partition_test.go @@ -0,0 +1,58 @@ +package preflight + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestCheckPartitionSupport(t *testing.T) { + plain := PreflightedTable{relkind: "r"} + require.NoError(t, CheckPartitionSupport(plain, 16, []string{"CREATE INDEX i ON s.t (id)"})) + + parent := PreflightedTable{relkind: "p"} + err := CheckPartitionSupport(parent, 16, []string{"CREATE INDEX i ON s.t (id)"}) + var unsupported *UnsupportedPartitionedParentError + require.ErrorAs(t, err, &unsupported) + assert.Equal(t, PartitionCauseBlockingIndexBuild, unsupported.Cause) + + err = CheckPartitionSupport(parent, 16, []string{"ALTER TABLE s.t THIS IS NOT SQL"}) + require.Error(t, err) + assert.False(t, errors.As(err, &unsupported), "parse failures are operational errors, not refusals") +} + +func TestRefusesPartitionedParentDistinguishesIndexBuilds(t *testing.T) { + cause, err := RefusesPartitionedParent(16, []string{"CREATE INDEX CONCURRENTLY i ON s.t (id)"}) + require.NoError(t, err) + assert.Equal(t, PartitionCauseConcurrentIndexBuild, cause) + + cause, err = RefusesPartitionedParent(16, []string{"CREATE INDEX i ON s.t (id)"}) + require.NoError(t, err) + assert.Equal(t, PartitionCauseBlockingIndexBuild, cause) +} + +func TestRefusesPartitionedParentAdoptsExistingIndex(t *testing.T) { + for _, major := range []int{14, 15, 16, 17, 18} { + cause, err := RefusesPartitionedParent(major, + []string{`ALTER TABLE public.p ADD CONSTRAINT p_pk PRIMARY KEY USING INDEX ix_p_id`}) + require.NoError(t, err) + require.Equal(t, PartitionCauseIndexAdoption, cause) + } +} + +func TestRefusesPartitionedParentNotValidForeignKeyByServerVersion(t *testing.T) { + steps := []string{"ALTER TABLE s.t ADD CONSTRAINT t_fk FOREIGN KEY (parent_id) REFERENCES s.parent(id) NOT VALID"} + cause, err := RefusesPartitionedParent(17, steps) + require.NoError(t, err) + assert.Equal(t, PartitionCauseNotValidForeignKey, cause) + cause, err = RefusesPartitionedParent(18, steps) + require.NoError(t, err) + assert.Empty(t, cause) + + checkSteps := []string{"ALTER TABLE s.t ADD CONSTRAINT t_check CHECK (parent_id > 0) NOT VALID"} + cause, err = RefusesPartitionedParent(14, checkSteps) + require.NoError(t, err) + assert.Empty(t, cause) +} diff --git a/pkg/preflight/preflight.go b/pkg/preflight/preflight.go index 53a8d4c..b1f8318 100644 --- a/pkg/preflight/preflight.go +++ b/pkg/preflight/preflight.go @@ -55,6 +55,7 @@ func (e *SizeError) Error() string { type PreflightedTable struct { schema string table string + relkind string totalBytes int64 relTuples float64 } @@ -66,6 +67,10 @@ func (t PreflightedTable) Schema() string { return t.schema } // Table returns the verified table name. func (t PreflightedTable) Table() string { return t.table } +// Partitioned reports whether the verified target is a partitioned parent. +// Leaf partitions have relkind 'r' and therefore report false. +func (t PreflightedTable) Partitioned() bool { return t.relkind == "p" } + // TotalBytes returns the measured on-disk size across all partitions, // including indexes and TOAST. func (t PreflightedTable) TotalBytes() int64 { return t.totalBytes } @@ -75,6 +80,43 @@ func (t PreflightedTable) TotalBytes() int64 { return t.totalBytes } // bytes on disk. func (t PreflightedTable) RelTuples() float64 { return t.relTuples } +// TargetFacts are the cheap target facts needed by planning and executor +// admission without measuring the relation or its partition tree. +type TargetFacts struct { + partitioned bool + serverMajor int +} + +// Partitioned reports whether the target is a partitioned parent. +func (f TargetFacts) Partitioned() bool { return f.partitioned } + +// ServerMajor returns the PostgreSQL server major version. +func (f TargetFacts) ServerMajor() int { return f.serverMajor } + +// LookupTargetFacts verifies that the target is an ordinary or partitioned +// table and returns its relation kind and server major in one catalog query. +func LookupTargetFacts(ctx context.Context, pool *pgxpool.Pool, schema, table string) (TargetFacts, error) { + const q = ` + SELECT c.relkind::text, current_setting('server_version_num')::int / 10000 + FROM pg_class c + WHERE c.oid = to_regclass( + CASE WHEN $1 = '' THEN quote_ident($2) + ELSE quote_ident($1) || '.' || quote_ident($2) END)` + var relkind string + var serverMajor int + err := pool.QueryRow(ctx, q, schema, table).Scan(&relkind, &serverMajor) + if errors.Is(err, pgx.ErrNoRows) { + return TargetFacts{}, fmt.Errorf("%w: %s", ErrTableNotFound, qualifiedName(schema, table)) + } + if err != nil { + return TargetFacts{}, fmt.Errorf("look up table facts %s: %w", qualifiedName(schema, table), err) + } + if relkind != "r" && relkind != "p" { + return TargetFacts{}, fmt.Errorf("%w: %s has relkind %q", ErrNotTable, qualifiedName(schema, table), relkind) + } + return TargetFacts{partitioned: relkind == "p", serverMajor: serverMajor}, nil +} + // CheckTable verifies that schema.table (search_path when schema is empty) // exists, is an ordinary or partitioned table, and is at most limitBytes on // disk. Above the limit it returns a *SizeError; on success it returns the @@ -124,7 +166,9 @@ func CheckTable(ctx context.Context, pool *pgxpool.Pool, schema, table string, l if totalBytes > limitBytes { return PreflightedTable{}, &SizeError{TotalBytes: totalBytes, LimitBytes: limitBytes} } - return PreflightedTable{schema: schema, table: table, totalBytes: totalBytes, relTuples: relTuples}, nil + return PreflightedTable{ + schema: schema, table: table, relkind: relkind, totalBytes: totalBytes, relTuples: relTuples, + }, nil } // qualifiedName renders schema.table for error messages, omitting the dot diff --git a/pkg/verdict/verdict.go b/pkg/verdict/verdict.go index 899a0bd..f20a3ba 100644 --- a/pkg/verdict/verdict.go +++ b/pkg/verdict/verdict.go @@ -61,6 +61,10 @@ const ( // the change needs; Detail names the exact missing GRANT (see // docs/engine-role.md). ReasonInsufficientPrivileges Reason = "insufficient-privileges" + // ReasonUnsupportedPartitionedParent: the routed plan builds an index + // on a partitioned parent, for which the required partition-aware + // online sequence is not implemented. + ReasonUnsupportedPartitionedParent Reason = "unsupported-partitioned-parent" // ReasonBudgetExceeded: the optimistic attempt exceeded its lock or // statement budget and was cancelled. ReasonBudgetExceeded Reason = "not-native-safe-budget-exceeded" diff --git a/pkg/verdict/verdict_test.go b/pkg/verdict/verdict_test.go index 8051d34..76d8f0b 100644 --- a/pkg/verdict/verdict_test.go +++ b/pkg/verdict/verdict_test.go @@ -84,6 +84,7 @@ func TestReasonAndCauseTokensAreFlat(t *testing.T) { string(ReasonTableTooLarge), string(ReasonBudgetExceeded), string(ReasonInsufficientPrivileges), + string(ReasonUnsupportedPartitionedParent), string(CauseLockBudget), string(CauseStatementBudget), } {