feat(postgres): implement declarative planning via pg-sprite diffplan - #1008
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
morgo
left a comment
There was a problem hiding this comment.
🤖 Approving on Morgan's behalf (automated review, escalation rules apply). The verdict mapping is genuinely fail-closed and nothing here touches the MySQL/Vitess production paths, so stamping — but two notes worth acting on:
Interaction with #1004 (sequencing): blockPostgresPlanWithoutClassifierVerdicts stamps every postgres table change to blocked without checking for an existing verdict (its own test blocks a change that carries one). Once both PRs land, the executable changes this PR produces get re-stamped blocked and the specific ModeReasons (copy-and-swap / refuse / rewrite-required) are overwritten with the generic "verdict unavailable" message. Fail-closed, so safe — but it nullifies this PR's stated outcome, and no test spans the two layers. Suggest making the gate verdict-aware, or noting the intended lift/sequencing explicitly.
Ops disclosure: worth stating in the PR body that diffplan planning is not read-only — pg-sprite realizes desired state by executing the schema-file DDL on the live target in a random scratch schema inside an always-rolled-back transaction. The mitigations check out (ParseDesired admits only one unqualified CREATE TABLE plus non-concurrent CREATE INDEX, pgx.Identifier sanitization, SET LOCAL search_path, 3s lock / 30s statement timeouts), but operators should get to acknowledge the privilege/execution model.
Smaller notes: flattening ExecSQL into TableChange rows drops pg-sprite's autocommit execution contract — steps can include CREATE INDEX CONCURRENTLY, which would fail (closed) if a future Apply wraps them in a transaction, so the persisted plan loses information Apply will need; an execute-disposition report with empty ExecSQL maps to the misleading verdict label "unrecognized"; and planSchemas has no direct unit coverage (nil-namespace, multi-file ordering, NoChanges).
Verified clean: proto enum additions are additive and skew-safe, converters round-trip tested, errors wrapped not swallowed, the testcontainers 0.43/moby migration is behavior-neutral test infra.
Use pg-sprite's routed diff plan to expose executable native DDL while blocking unsupported or unrecognized routes before apply.
Plan IDs are now per-request unique like the other engines; the persisted plan identifier is globally UNIQUE, so a fingerprint-derived ID would collide across databases and PRs producing identical DDL. Index statements get first-class change types end to end (operation strings and new proto enum values) so a DROP INDEX can never materialize remotely as a table drop and trip the unsafe opt-in gate. Completes the testcontainers-go 0.43 sweep in the integration-tagged packages the dependency bump broke, opens the planning pool through pg-sprite's dbconn so sessions run under bounded timeouts, and covers the format-version, rewrite-required, and destructive-safety verdict branches with tests.
aca38c9 to
37a8ea6
Compare
There was a problem hiding this comment.
Pull request overview
Implements PostgreSQL schema planning by integrating pg-sprite’s routed diff planner so SchemaBot can produce a reviewable, fail-closed plan for declarative Postgres schema changes (while Apply/Start/Stop/Progress remain stubs).
Changes:
- Add
postgres.Engine.Planimplementation using pg-spritestatement.ParseDesired+diffplan.Plan, mapping routed statements to executable vs blockedengine.TableChanges with sanitized reasons. - Extend DDL/proto plumbing to represent index operations distinctly end-to-end (
create_index/drop_index) to avoid conflating index work with table create/drop. - Upgrade
testcontainers-goand migrate container/network types to Moby APIs across integration tests and helpers.
Reviewed changes
Copilot reviewed 16 out of 18 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/engine/postgres/postgres.go | Implements Postgres Plan via pg-sprite diff planning; converts planner output to SchemaBot PlanResult / TableChanges with fail-closed verdicts. |
| pkg/engine/postgres/postgres_test.go | Unit tests for verdict mapping and Report→TableChange conversion (including ordered ExecSQL steps and fail-closed behavior). |
| pkg/engine/postgres/postgres_integration_test.go | Integration test proving Plan can derive an executable create-table plan against a live Postgres container. |
| pkg/ddl/statement_type.go | Adds operation-string round-trips for index statement types (create_index, drop_index). |
| pkg/ddl/statement_type_test.go | Tests op↔statement-type round-trip for the new index operation strings. |
| pkg/proto/tern.proto | Adds proto ChangeType enum values for create/drop index. |
| pkg/proto/ternv1/tern.pb.go | Regenerated proto output reflecting new change-type enum values. |
| pkg/api/proto_helpers.go | Maps new proto change types to/from operation strings for API/proto conversions. |
| pkg/api/proto_helpers_test.go | Extends change-type round-trip coverage to include create/drop index. |
| pkg/tern/state_converters.go | Maps ddl statement types to tern proto change types (and back) for index operations. |
| pkg/tern/state_converters_test.go | Adds a round-trip test to ensure index ops preserve their change types across conversions. |
| pkg/testutil/container.go | Updates testcontainers port-mapping helper to the newer port type API and numeric accessors. |
| pkg/localscale/testcontainer.go | Updates mapped-port access to the newer testcontainers API. |
| pkg/engine/spirit/spirit_integration_test.go | Updates SQL wait/port types for testcontainers + Moby networking APIs. |
| pkg/namedlock/namedlock_integration_test.go | Updates SQL wait/port types for testcontainers + Moby networking APIs. |
| pkg/auth/dex_integration_test.go | Updates Docker port binding types to Moby APIs (container/network) for the testcontainers bump. |
| go.mod | Adds github.com/block/pg-sprite and bumps testcontainers-go (plus related dependency shifts). |
| go.sum | Updates dependency checksums for pg-sprite addition and transitive upgrades (including testcontainers and Moby libs). |
Files not reviewed (1)
- pkg/proto/ternv1/tern.pb.go: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…k' into kiran01bm/pg-backend-nucleus * origin/kiran01bm/sqlstore-joined-dml-13k: refactor(storage): reject bind placeholders in JoinedUpdate join conditions refactor(storage): tighten the JoinedUpdate dialect contract refactor(storage): render joined UPDATEs through the dialect feat(postgres): implement declarative planning via pg-sprite diffplan (#1008) refactor(storage): make remaining sqlstore SQL dialect-portable (#1007) test(e2e): deflake multi-table stop/start resume and MySQL cold starts (#1005) ci: verify golangci config against a vendored schema (#997) feat(api): fail-closed verdict gating for postgres plans (#1004) feat(tern): route postgres targets to the postgres engine (#1003) feat(storage): stamp remaining sqlstore timestamps explicitly (#1006) # Conflicts: # pkg/storage/internal/sqlstore/apply_comments.go # pkg/storage/internal/sqlstore/dialect.go # pkg/storage/internal/sqlstore/dialect_test.go # pkg/storage/internal/sqlstore/settings.go # pkg/storage/internal/sqlstore/storage.go # pkg/storage/internal/sqlstore/updated_at_lint_test.go
…-joined-dml-13l * origin/main: refactor(storage): render joined UPDATEs through the dialect (#1009) fix(planetscale): hold the cutover when the operator defers it (#978) fix(observability): make telemetry resource schema-tolerant (#1014) feat(postgres): implement declarative planning via pg-sprite diffplan (#1008) refactor(storage): make remaining sqlstore SQL dialect-portable (#1007) test(e2e): deflake multi-table stop/start resume and MySQL cold starts (#1005) ci: verify golangci config against a vendored schema (#997) feat(api): fail-closed verdict gating for postgres plans (#1004) feat(tern): route postgres targets to the postgres engine (#1003) feat(storage): stamp remaining sqlstore timestamps explicitly (#1006) # Conflicts: # pkg/storage/internal/sqlstore/apply_comments.go # pkg/storage/internal/sqlstore/dialect.go # pkg/storage/internal/sqlstore/dialect_test.go # pkg/storage/internal/sqlstore/settings.go # pkg/storage/internal/sqlstore/storage.go # pkg/storage/internal/sqlstore/updated_at_lint_test.go
…re-public-13n * origin/main: refactor(storage): portable lease-guarded joined DML for the operation store (#1011) fix(github): align lint warnings formatting with issues and fold long lists (#959) fix(github): lead with the database's operators on command-rejection comments (#960) docs: regenerate stale tables of contents (#968) fix(engine): heartbeat the row a local drive actually owns (#915) fix(github): scope auto-plan to the schema a pull request proposes (#1016) fix(vitess): dispatch task-less VSchema-only work operations over gRPC (#961) feat(storage): add PostgreSQL dialect nucleus to the shared store core (#1010) refactor(storage): render joined UPDATEs through the dialect (#1009) fix(planetscale): hold the cutover when the operator defers it (#978) fix(observability): make telemetry resource schema-tolerant (#1014) feat(postgres): implement declarative planning via pg-sprite diffplan (#1008) refactor(storage): make remaining sqlstore SQL dialect-portable (#1007) test(e2e): deflake multi-table stop/start resume and MySQL cold starts (#1005) ci: verify golangci config against a vendored schema (#997) feat(api): fail-closed verdict gating for postgres plans (#1004) feat(tern): route postgres targets to the postgres engine (#1003) feat(storage): stamp remaining sqlstore timestamps explicitly (#1006)
…lect-factory-14b * origin/main: feat(github): flag destructive changes to tables another open PR owns (#1017) feat(storage): add public postgresstore constructor (#1012) refactor(storage): portable lease-guarded joined DML for the operation store (#1011) fix(github): align lint warnings formatting with issues and fold long lists (#959) fix(github): lead with the database's operators on command-rejection comments (#960) docs: regenerate stale tables of contents (#968) fix(engine): heartbeat the row a local drive actually owns (#915) fix(github): scope auto-plan to the schema a pull request proposes (#1016) fix(vitess): dispatch task-less VSchema-only work operations over gRPC (#961) feat(storage): add PostgreSQL dialect nucleus to the shared store core (#1010) refactor(storage): render joined UPDATEs through the dialect (#1009) fix(planetscale): hold the cutover when the operator defers it (#978) fix(observability): make telemetry resource schema-tolerant (#1014) feat(postgres): implement declarative planning via pg-sprite diffplan (#1008) refactor(storage): make remaining sqlstore SQL dialect-portable (#1007) test(e2e): deflake multi-table stop/start resume and MySQL cold starts (#1005) ci: verify golangci config against a vendored schema (#997) feat(api): fail-closed verdict gating for postgres plans (#1004) feat(tern): route postgres targets to the postgres engine (#1003) feat(storage): stamp remaining sqlstore timestamps explicitly (#1006)
Summary
Implements
Planin the PostgreSQL engine using pg-sprite's routed diff planner (pkg/diffplan). Statements that pg-sprite routes as native and executable surface as executable table changes; every other verdict fails closed to a blocked change with a sanitized reason. Apply/Start/Stop/Progress remain fail-closed stubs.Why
This is the planning half of the PostgreSQL engine: SchemaBot can now show a reviewable plan for declarative PostgreSQL schema changes, executing only what pg-sprite's router proves safe for the native path and blocking everything else with a clear reason instead of guessing.
What
postgresconn.Open(keeping SchemaBot's transport policy on the connection path), then opens the planning pool through pg-sprite'sdbconn.NewPoolfrom the same normalized DSN, so planning sessions run under pg-sprite's boundedlock_timeout/statement_timeoutdefaults.statement.ParseDesiredand callsdiffplan.Planper table.ExecSQLsteps intoengine.TableChange, classifying each statement with the PostgreSQL DDL parser. StandaloneCREATE INDEX/DROP INDEXstatements carry first-class operation strings and proto change types end to end, so an index drop is never conflated with a table drop.execute/RouteNative/BackendNativewith rendered SQL; copy-and-swap, refuse, rewrite-required, and unrecognized verdicts map toExecutionModeBlockedwith an operator-safe reason (planner explanations are not copied into operator-facing text).block/pg-sprite; this transitively bumpstestcontainers-go0.40 → 0.43, whose container-configuration API moved to the Moby types — all integration-tagged callers are updated accordingly.Known limitations (planning-only scope, follow-ups tracked): deleting a table's declarative schema file yields
NoChangesrather than a drop plan (the planner only diffs declared files), andOriginalFilesrollback capture is not yet populated for PostgreSQL plans — rollback of a stored PostgreSQL plan fails closed until Apply lands.Before / after