feat(postgres): apply native-safe schema changes via pg-sprite

[Kiran01bm]

Summary

Implements the PostgreSQL engine's Apply verb for the native-safe (optimistic) path, driving pg-sprite in-process: bounded pool → privilege preflight → native execution → typed outcome mapping. Adds a minimal, honest Progress (phase, elapsed, step 1/1) with a marked seam for pg-sprite's richer progress surface.

Why

With Plan (#1008), wiring (#1003), and gates (#993/#1004) merged, Apply is the last verb needed for a native-safe PostgreSQL change to run end to end through SchemaBot. Scoped deliberately to the optimistic path — planner-produced sequence execution (RunSequence) follows as its own increment behind the same seam, keeping this PR single-purpose and reviewable.

What

• pkg/engine/postgres/apply.go: pool via dbconn.NewPool, preflight via preflight.RequiredTier / CheckPrivileges / CheckTable, execution via executor.ExecuteNative, bounded by a per-apply deadline.
• Plan-time verdict parity: planning runs preflight.RequiredTier per rendered statement, so shapes the native-safe path cannot execute (e.g. CREATE TABLE) are planned blocked instead of failing deterministically at apply; apply acceptance re-checks the same authority synchronously.
• Blocked-verdict enforcement at apply queueing: storage.Plan.BlockedChanges() + rejectBlockedStoredPlan refuse to queue an apply for a plan carrying a blocked change (the drive layer rebuilds engine requests without the verdict, so the queue gate is the enforcement point). No opt-in — the change must be rewritten and re-planned.
• Centralized refusal classification (classifyRefusal): privilege, statement-budget, oversized-table, missing-table, and not-a-table outcomes map to permanent refusals with typed detail (exact missing GRANT surfaced); lock-budget exhaustion stays a retryable operational failure. Other operational errors → failed with sanitized detail (raw error logged server-side).
• Respects the existing capability gates (no pending drops, no deferred cutover for PG); implements engine.Drainer so resume/recovery waits out in-flight applies; idle Progress emits the canonical No active schema change message so stale-task auto-resolution works.
• pg-sprite pinned to v0.0.0-20260814025010-d6cf677e4feb (privilege-preflight merge); go mod tidy bumped two AWS SDK transitive deps alongside.
• Tests: unit coverage for refusal classification and blocked-plan gating; integration coverage for instant apply, privilege refusal (GRANT in detail), table-not-found refusal, non-native shape refusal at acceptance, and operational failure. Typed assertions, no message-text matching.

Before
┌────────────────────┐   ┌───────────────────┐
│ SchemaBot          │──▶│ postgres engine   │   Plan     ✓ (#1008)
│ orchestrator       │   │                   │   Apply    ✗ not implemented
└────────────────────┘   └───────────────────┘   Progress ✗ not implemented

After
┌────────────────────┐   ┌───────────────────┐   ┌───────────────────────────────────┐
│ SchemaBot          │──▶│ postgres engine   │──▶│ pg-sprite (in-process)            │
│ orchestrator       │   │ Plan / Apply /    │   │ dbconn bounded pool               │
│                    │   │ Progress / Drain  │   │  ─▶ privilege preflight           │
│ blocked-plan gate  │   └─────────┬─────────┘   │  ─▶ ExecuteNative (lock/stmt      │
│ at apply queueing  │             │             │     budgets, optimistic attempt)  │
└────────▲───────────┘             ▼             └───────────────────────────────────┘
         │   success ───────────────────▶ completed
         │   PrivilegeError ────────────▶ blocked (missing GRANT surfaced)
         │   BudgetError (statement) ───▶ blocked (not native-safe)
         │   SizeError / no such table ─▶ blocked (typed refusal reason)
         │   BudgetError (lock) ────────▶ failed  (retryable: lock contention)
         └── other error ───────────────▶ failed  (sanitized detail, raw logged)

References

• preflight: engine-role privilege checks with typed GRANT-naming refusals pg-sprite#31 (privilege preflight this consumes)

[Copilot]

Pull request overview

Implements the PostgreSQL engine’s native-safe Apply path by running pg-sprite’s optimistic executor in-process and exposing minimal progress (phase/elapsed/step), plus adds integration coverage for success, privilege refusal, and operational failure cases.

Changes:

• Add pkg/engine/postgres/apply.go implementing native-safe Apply + Progress, including pg-sprite preflight and native execution with typed error mapping.
• Extend the Postgres engine struct to hold in-process progress state.
• Add integration tests validating success and failure-mode mapping; bump pg-sprite + AWS SDK transitive deps via go mod tidy.

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
pkg/engine/postgres/postgres.go	Adds engine state fields to support in-process Apply/Progress lifecycle.
pkg/engine/postgres/apply.go	Implements native-safe Apply execution via pg-sprite with basic progress reporting and outcome mapping.
pkg/engine/postgres/postgres_integration_test.go	Adds integration tests for success, privilege refusal, and operational failure scenarios.
go.mod	Pins pg-sprite to the referenced commit and updates AWS SDK deps.
go.sum	Updates checksums for the bumped dependencies.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[aparajon]

🤖 Adversarial correctness review, requested by Armand and performed by his agent. Reviewed at head 88276b30, built and run against live PostgreSQL.

Verdict: the refusal classification and the plan-time blocked verdict are right; the state the engine reports them through is not per-apply, and the retryable/permanent distinction the code builds has no consumer. I went at this from the direction the PR's own reasoning invites — the engine is a long-lived object shared across a target, and this is the first engine to run applies in a detached goroutine — so I attacked lifetime and identity rather than the pg-sprite seam, which #33 already covered. Two of the three findings are the same root cause; the third is the one I'd hold the merge on independently.

Findings

1. Progress() answers about an apply, not the apply — an in-flight schema change gets reported as completed. Engine holds one progress *engine.ProgressResult and Progress() discards both of its parameters, including the *engine.ProgressRequest that carries the caller's identity. One engine is shared for the lifetime of a target (TargetRouter.clientsByTarget caches one LocalClient, which builds one postgres.New()), applies run detached under a 5-minute ceiling, and Drain() is only called on the two resume paths — so a goroutine from an apply nobody is polling any more writes its terminal state over the apply that is being polled. This is not a display problem: pollTaskToCompletion terminalizes on result.State.IsTerminal() and copies result.Tables[0] onto the task, so the running statement's task is marked completed with the other statement's DDL and timestamps. That converts a started apply into a finished one on the strength of a stale writer, which is the case AGENTS.md singles out ("started applies remain authoritative"). The fix is already sitting in the request: sequentialEngineApplyRequest sets ResumeState.MigrationContext = task.TaskIdentifier, and line 485 threads the same ResumeState into every ProgressRequest — the engine only has to stamp it on progressResult and return the idle sentinel when the caller asks about an identifier it isn't running.

Repro (pkg/engine/postgres, fails at head in 2.00s)

Two lock-blocked applies on one engine, t_a then t_b. Releasing t_a's lock lets apply A finish; apply B is still executing.

func TestProgressIsNotOverwrittenByAnEarlierApply(t *testing.T) {
	// Two applies share one engine, as they do behind a cached LocalClient.
	// Progress for the second must never report the first's terminal state.
	...
	releaseLockOn(t, "t_a") // apply A completes; apply B is still blocked

	progress, err := eng.Progress(t.Context(), progressRequestFor(taskB))
	require.NoError(t, err)
	assert.Equal(t, "t_b", progress.Tables[0].Table)
	assert.False(t, progress.State.IsTerminal(),
		"apply B is still executing, so the engine must not report a terminal state for it")
}

progress after A finished, while B is still running:
  state=completed table=t_a ddl="ALTER TABLE public.t_a ADD COLUMN a text"

Error: Not equal: expected: "t_b" actual: "t_a"
Error: Should be false — apply B is still executing, so the engine must not
       report a terminal state for it

2. Every PostgreSQL failure is permanent, including the one the code goes out of its way to mark retryable. runOptimisticApply deliberately keeps lock-budget exhaustion out of classifyRefusal with a comment saying marking it blocked "would falsely tell the operator retrying cannot succeed", and surfaces it as "…; retry once lock contention subsides". But progressResult never sets ProgressResult.Retryable, and taskStateFromProgressResult is the only thing that reads it (result.State == engine.StateFailed && result.Retryable) — Spirit sets it at spirit.go:739, PostgreSQL doesn't set it anywhere. So the task lands failed, not failed_retryable, and finalizeSequentialApply takes the permanent branch: apply.State = failed, CompletedAt stamped, and every remaining pending task cancelled. Concretely: statement 2 of 5 loses a bounded 3-second lock race, and statements 3–5 are cancelled with an apply the operator cannot resume — the exact outcome the comment says it is avoiding. Separately, the classification's other output is inert: Metadata["execution_mode"] and Metadata["refusal_reason"] are set on the result and then dropped on the floor — the poller reads result.Tables[0] and progressFailureMessage and nothing else in the tree reads either key. The refusal taxonomy is good work, but right now it survives only as English inside task.ErrorMessage.

3. Drain() doesn't drain — it waits. engine.Drainer documents Drain as waiting for in-flight background work "and clears it", and Spirit implements exactly that (spirit.go:298-312: wait, then runningSchemaChange = nil). PostgreSQL's is e.wg.Wait() alone, so after a drain Progress() keeps returning the previous change's terminal snapshot instead of the "No active schema change" sentinel — the same sentinel this PR's own comment identifies as the cross-engine contract that stale-task recovery compares against verbatim. Both resume call sites (local_control_resume.go:331 and :705) drain precisely so the next poll reads a clean engine, so this lands where it matters most. Same root cause as finding 1, but worth fixing explicitly rather than as a side effect: whatever identity-keyed shape finding 1 takes, Drain() should end with the engine idle.

Action items

1. (Finding 1) Key engine progress to the apply that produced it — stamp ResumeState.MigrationContext on write, compare it on read, return the idle sentinel on a mismatch. Both sides of the identifier are already threaded through the requests.
2. (Finding 2) Set Retryable on the lock-budget path so it reaches failed_retryable and does not cancel the rest of the apply, and either consume execution_mode/refusal_reason at the drive layer or drop them until something does.
3. (Finding 3) Clear the stored progress in Drain(), matching the Drainer doc and Spirit.
4. (optional) Add one test that runs two applies through a single engine — every test in the new file constructs a fresh New(), which is why the shared-state hazard is invisible to a green suite.

Verified (tried to break, couldn't)

Checked whether the removed assert.Empty(t, change.ExecutionMode) hides a behaviour regression — it doesn't: the verdict for that shape genuinely changed, the replacement asserts the new value and its reason string, and the test's doc comment was updated to match, so it clears the no-silent-test-deletion bar; checked whether the plan/apply parity hole is real, i.e. whether a table with several statements can reach Apply and trip the len(req.Changes[0].TableChanges) != 1 guard — it can't, applyTaskChanges uses FlatDDLChanges() so each statement becomes its own task; checked that the synchronous RequiredTier re-derivation in validateOptimisticApply can't disagree with the plan-time one in tableChanges and reject an admitted change, and it can't, both call the same function on the same SQL; checked cloneMetadata/cloneTables for the aliasing Copilot raised, and the deep copy is complete including the time pointers and the shard slice; checked whether classifyRefusal's detail strings are safe on a public PR surface, and every one is built from typed fields and identifiers with no wrapped server output, matching the comment-sanitization rule; checked that the 5-minute ceiling can't fire on a legitimate run given the 30s statement limit and the retry policy's backoff, and the headroom is real; confirmed go build ./... clean and the pkg/engine/postgres, pkg/storage and pkg/api suites green at head; confirmed all three Copilot threads are resolved with the code actually changed, not just marked.

The thing I'd single out as right: putting the blocked verdict on the plan and gating the queue on it in rejectBlockedStoredPlan, with the comment explaining why the drive layer can't be the gate (task rows don't carry the verdict). That is the correct place for it and the reasoning is the reason it's correct.

This review was generated by Claude Code (claude-opus-5).

[aparajon]

🤖 Second pass on the same head (88276b30), through two lenses rather than correctness: what the first PostgreSQL adopter experiences and cross-engine parity. Correctness findings are in the separate comment above.

Lens 1 — the first PostgreSQL adopter

CREATE TABLE is blocked, and that is the first change anyone makes. TestEnginePlanCreateTable pins it deliberately, and the plan-time verdict is the right call — better a blocked check than an apply that dies at statement 1. But it means a team onboarding a PostgreSQL database gets a blocked plan on their very first PR, and the message they get is statement for table "users" is not a shape the native-safe path executes, followed by rejectBlockedStoredPlan telling them there is no opt-in and "the schema change itself must be rewritten and re-planned." There is no rewrite. CREATE TABLE has no native-safe form to move to — the honest statement is "this engine doesn't execute this shape yet", which is a very different instruction from "rewrite your change." The wording sends an operator looking for a fix that doesn't exist, on the most common change there is. Worth a distinct message for unsupported shape vs rewrite required, since the plan already knows which one it hit.

The 1 GiB ceiling will be hit by essentially every real table, and the refusal doesn't say whose limit it is. optimisticTableSizeLimit is a hard-coded 1 << 30 with no config seam, and the refusal surfaces SizeError's text — "table size N bytes exceeds the M-byte threshold for an optimistic attempt." Read cold, that sounds like a property of the change or of PostgreSQL; it is SchemaBot's own conservatism, and the operator has no lever. A production table crossing 1 GiB is unremarkable, so this is not an edge case — it is the second wall after CREATE TABLE. Whatever the number ends up being, the operator-facing line should say it is a SchemaBot threshold, and it should be reachable from config before this is offered to a real tenant.

The privilege refusal is the best thing in the PR from an adopter's seat. classifyRefusal hands back insufficient privileges; provision with: <GRANT> plus the hint — the operator gets the exact statement to run rather than a permission-denied they have to decode into a grant. That is the standard the other refusal messages should be held to, and it is worth saying out loud so it doesn't get flattened into a generic string later.

Blocked reasons quote a vocabulary from another repo. executionVerdict renders statement for table %q has blocked verdict %q with the raw planner disposition (rewrite-required, unavailable, refuse). Refusing to copy the planner's free-text explanation is exactly right and I wouldn't change it. But the typed value that survives is a term from pg-sprite's vocabulary with no gloss, shown to someone who has never heard of pg-sprite. A short mapping from each disposition to a sentence in SchemaBot's own words would keep the safety property and lose the jargon.

Nothing in the repo tells an adopter what PostgreSQL support currently covers. Between the shape restriction, the size ceiling, CREATE TABLE, and the partitioned-parent refusals from pg-sprite#33, the set of changes that actually execute today is narrow and precisely knowable. Right now it can only be reconstructed by reading RequiredTier, executionVerdict, and two other repos. For a first tenant that is the difference between "I know what I can ship" and finding out one blocked PR at a time.

Lens 2 — cross-engine parity

Every control operation answers "postgres engine not implemented". Stop, Cancel, Start, Cutover, Revert, SkipRevert and Volume are all stubs. They pre-date this PR and were harmless while the engine only planned — but this is the PR that makes an apply startable, and the moment an apply can start, an operator can want to stop it. AGENTS.md's control-operation bar is per-engine ("must provide, on every engine"), and the current answer to stop on a running PostgreSQL apply is an error string that reads like the whole engine is missing. The apply is ceiling-bounded to five minutes so the real exposure is small, but the message is the wrong one to show an operator mid-incident: not supported for PostgreSQL schema changes is true and actionable, not implemented is neither. Whether the ops themselves land in this PR is a scoping call — the strings shouldn't wait for it.

The sequential driver still has MySQL-shaped assumptions that PostgreSQL now walks into. shouldRetryEngineError is c.config.Type == storage.DatabaseTypeMySQL && engine.IsRetryable(err), and the namespace-credential resolution at local_apply_sequential.go:191 is likewise MySQL-gated. Neither is wrong today, but combined with ProgressResult.Retryable being Spirit-only (finding 2 in the other comment), the practical shape is: the sequential driver has one engine's error model baked in at three separate points, and PostgreSQL inherits the else branch of each by default rather than by decision. Worth a pass that makes each of those an explicit per-engine answer, because "PostgreSQL gets whatever MySQL doesn't" is not a contract anyone can reason about later.

Metadata map[string]string is used the way the engine-agnostic rule intends. execution_mode, refusal_reason, phase, step, steps_total, elapsed_ms are all engine-specific data carried in the generic field, with nothing PostgreSQL-shaped leaking into pkg/engine's types. Same for TableChange.EngineBlocked() and Plan.BlockedChanges() in pkg/storage — those are genuinely engine-agnostic predicates that Spirit's direct path can use too, and putting them there rather than in the PostgreSQL package is the right instinct. (The one qualifier is that nothing currently reads the two metadata keys; that's in the other comment.)

Drain()'s contract divergence is a parity issue, not just a bug. Covered as finding 3 above, but the framing matters here: Drainer is an optional interface with a documented behaviour, two engines now implement it, and they do different things. Optional interfaces with per-engine semantics are how cross-engine flows quietly stop being cross-engine — the stale-task recovery path compares against a sentinel that one of the two engines will never return after a drain.

Plan-time gating is the right architecture and worth naming as precedent. Putting the verdict on the stored plan and refusing at createStoredApply, with the comment explaining that the drive layer rebuilds engine requests from task rows that don't carry the verdict, is the pattern the MySQL direct path should converge on too. It keeps the "blocked check, not failed apply" property — the same seam pg-sprite#33 built its refused-vs-failed distinction around — and it does it in the one place that can't be bypassed.

This review was generated by Claude Code (claude-opus-5).

[aparajon]

🤖 Stamping on Armand's call. The two comments above stand as written — in particular finding 1 (engine progress is not keyed to the apply that produced it, so an abandoned goroutine can report an in-flight schema change as completed) is a correctness issue I reproduced at this head, and finding 2 (nothing sets ProgressResult.Retryable on the PostgreSQL path, so a lock-budget loss permanently fails the apply and cancels its remaining statements) changes operator-visible outcomes. Neither is a reason to hold the seam itself: the plan-time blocked verdict, the queue-time gate, and the refusal taxonomy are the right shapes, and both fixes are small and local to pkg/engine/postgres.

Approving on the understanding that finding 1 lands before a PostgreSQL apply is exercised against a real tenant — the identifier it needs (ResumeState.MigrationContext) is already threaded through both the apply and progress requests.

This review was generated by Claude Code (claude-opus-5).

[Kiran01bm]

Review response from Kiran's (@Kiran01bm) AI code review assessment agent (Amp / Claude Opus 4.5)

All three findings (and the optional test) are fixed — engine progress is now keyed to the apply that produced it, operational failures reach failed_retryable, and Drain() leaves the engine idle.

#	Concern	Status	Explanation
1	Progress() ignores caller identity; a stale apply's terminal write is reported — and terminalized — against the apply being polled	fixed	Progress is keyed by ResumeState.MigrationContext exactly as the finding prescribes: Apply claims the slot, background writers publish only while they own it (stale writes are logged and discarded), and a mismatched identity reads the idle sentinel. The stale-task conflict probe now sends task.TaskIdentifier so a probe about live in-flight work sees its running state and keeps blocking.
2	Every PostgreSQL failure lands permanent — the lock-budget path the code explicitly calls retryable cancels the apply's remaining statements; execution_mode/refusal_reason metadata is inert	fixed	Lock-budget exhaustion and generic operational failures now set ProgressResult.Retryable, so they reach failed_retryable and leave the apply's pending tasks intact; refusals stay permanent. Took the "drop until something consumes them" option for the metadata keys: the taxonomy survives in the typed refusal struct and the operator-facing detail message.
3	Drain() waits but retains the previous change's terminal snapshot, so post-drain polls read stale state instead of the idle sentinel	fixed	Drain() now waits, then clears the tracked change and its key — matching the Drainer doc and the Spirit implementation — so both resume call sites read a clean engine.
4	(optional) No test shares one engine across two applies, which is why the hazard was invisible	fixed	TestEngineSharedAcrossAppliesKeepsProgressPerApply runs two applies through a single engine and proves identity isolation, stale-writer discard, and Drain cleanup; unit tests cover the retryable/permanent split.

The "Verified (tried to break, couldn't)" section: no action — appreciated, particularly the confirmation that the plan/apply parity guard is unreachable via FlatDDLChanges().

Source: #1025 (comment 5291044139), adversarial review requested by Armand, performed by Claude Code (claude-opus-5).

[Kiran01bm]

Review response from Kiran's (@Kiran01bm) AI code review assessment agent (Amp / Claude Opus 4.5)

All four message-level findings are fixed in this PR (unsupported-shape wording, disposition gloss, size-ceiling attribution, control-op strings); the size-ceiling config seam, the coverage doc, and the per-engine driver pass are tracked as internal follow-ups.

#	Concern	Status	Explanation
L1-1	The unsupported-shape message ("not a shape the native-safe path executes" + "must be rewritten") sends an operator hunting for a rewrite that does not exist — on CREATE TABLE, the first change anyone makes	fixed	The shape refusal now says what is true: the statement "is a shape SchemaBot's PostgreSQL support does not execute yet; rewriting the change cannot make it eligible". The rewrite wording is reserved for the planner disposition that genuinely means a rewrite exists.
L2-1	Control operations answer "postgres engine not implemented" — the wrong message to show an operator mid-incident now that applies can start	fixed	All seven stubs answer per-op: "stop/cancel/start/cutover/revert/skip-revert/volume is not supported for PostgreSQL schema changes". The operations themselves are the next tracked item in the internal plan and gate removing the plan seam's force-block.
L1-4	Blocked reasons quote pg-sprite's vocabulary (rewrite-required, refuse, unavailable) with no gloss	fixed	Each known disposition now maps to a sentence in SchemaBot's own words (rewrite-and-re-plan, refused-as-unsafe-as-written, execution-path-not-provided-yet, unrecognized-planner-verdict); planner free-text explanations remain deliberately uncopied.
L1-2	The 1 GiB ceiling reads like a property of PostgreSQL or the change, and has no config lever	fixed (message) / deferred (config seam)	The refusal now appends "this threshold is SchemaBot's ceiling for a native-safe apply, not a PostgreSQL limit". Making the ceiling configurable is tracked as an internal follow-up gated before real-tenant rollout.
L2-2	The sequential driver has MySQL-shaped assumptions at three points; PostgreSQL inherits the else branch by default rather than by decision	deferred (one part fixed)	The Retryable leg is fixed in this pass (PostgreSQL now participates in failed_retryable). The explicit per-engine pass over shouldRetryEngineError and namespace-credential resolution is tracked as an internal follow-up.
L1-5	Nothing tells an adopter what PostgreSQL support currently covers; the envelope is only reconstructable from three repos	deferred	An adopter-facing coverage doc (shape set, ceiling, CREATE TABLE, partitioned parents) is tracked under the internal operator-UX follow-up for PG applies.
L2-4	Drain() contract divergence between the two Drainer implementations	fixed	Same commit as the correctness response above: Drain waits, then clears the tracked change, so both engines end a drain idle and the stale-task sentinel comparison holds cross-engine.
L1-3 / L2-3 / L2-5	Privilege-refusal actionability, engine-agnostic Metadata usage, plan-time gating as precedent	no action (confirmations)	Noted with thanks. One update to L2-3: the two inert metadata keys were dropped per the correctness review's finding 2, so the qualifier no longer applies.

Source: #1025 (comment 5291044909), adopter-experience and cross-engine-parity review requested by Armand, performed by Claude Code (claude-opus-5).