converge additive postgresql storage schema drift at startup

[Kiran01bm]

Flip the PostgreSQL startup schema-drift tripwire into additive convergence: missing columns and standalone indexes are now created automatically under the bootstrap advisory lock instead of failing startup or warning for manual DDL.

Why

On an already-bootstrapped PostgreSQL store, a schema file gaining a column failed startup and a new non-unique index only warned — both required an operator to run DDL by hand before deploying. MySQL already converges via Spirit; this closes the gap for the additive cases PostgreSQL can apply safely, using the ADD COLUMN synthesis seam from the previous PR so the DDL always derives from the embedded desired CREATE TABLE.

What

• Drift discovery (postgresSchemaDriftFor): missing table → full schema file; missing column → ddl.SynthesizePostgresAddColumn; missing index → the schema file's own CREATE INDEX verbatim.
• Same concurrency shape as table creation: fast-path inspect without the lock, advisory lock on drift, re-inspect under the lock (another pod may have converged), per-table transactional apply, then re-verify — unresolved drift still fails startup.
• Fail-closed bounds: a missing NOT NULL column without a DEFAULT aborts with a manual-remediation error before any DDL runs (safe convergence may need a deliberate backfill); a live non-unique index where the schema requires a unique one aborts rather than altering it. Extra columns/indexes stay tolerated for binary rollback; column checks remain presence-only.
• Integration tests on real PostgreSQL: column/unique-index/non-unique-index convergence, idempotency, extras tolerated, NOT NULL-without-DEFAULT rejection, concurrent pods, under-lock re-check. Docs + AGENTS.md operator notes updated.

Before / after

Before                                    After
┌───────────────────────────────┐        ┌────────────────────────────────────┐
│ verify shape                  │        │ discover drift (no lock)           │
│  ├─ missing column → FATAL    │        │  └─ drift? → advisory lock         │
│  ├─ missing unique ix → FATAL │  ───▶  │      → re-check under lock         │
│  └─ missing plain ix → WARN,  │        │      → per-table tx:               │
│      operator runs DDL by hand│        │         ADD COLUMN (synthesized)   │
│                               │        │         CREATE INDEX (verbatim)    │
│                               │        │      → re-verify (drift = FATAL)   │
│                               │        │ NOT NULL w/o DEFAULT → FATAL,      │
│                               │        │ actionable error (no DDL executed) │
└───────────────────────────────┘        └────────────────────────────────────┘

[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 @aparajon and performed by their agent. Reviewed at head 2b83fbcf. Stacked on #1212 — scope here is this PR's delta.

Verdict: the convergence machinery is right — the lock/re-check/gate/apply/re-verify shape is careful, and gating the whole drift set before any DDL is the correct call — but the safety classifier has its generated/identity arm backwards, and I can prove it. That arm is the one place where the reasoning inverts: it green-lights, as needing no remediation, exactly the two column shapes that force a full-table rewrite under ACCESS EXCLUSIVE — the same hazard the volatile-DEFAULT arm right below it refuses a change for.

Findings

1. PostgresAddColumnManualReason returns "safe" for generated and identity columns, and both rewrite the whole table. The doc's justification is that "PostgreSQL computes values for existing rows itself, so no backfill is needed" — true, and it is the wrong test. Computing values for existing rows is the rewrite. I ran it against PostgreSQL 16 on a populated table and watched relfilenode move: ADD COLUMN g bigint GENERATED ALWAYS AS (id*2) STORED rewrote it, ADD COLUMN idc bigint GENERATED BY DEFAULT AS IDENTITY rewrote it again, and a constant DEFAULT 7 — the case the classifier permits — left it untouched. So the classifier's logic is exactly right for defaults and inverted for these two: the volatile-DEFAULT branch refuses because "a volatile default rewrites the whole table under an exclusive lock", while this branch waves through two shapes that always do. It also returns immediately on encountering either constraint, so it short-circuits the NOT NULL and volatile-DEFAULT checks for that same column. That matters because this classifier is the only thing standing between an embedded schema file and unattended DDL at startup, on a storage table that may be large, with EnsureSchemaTimeout as the only bound.

2. The same enumeration treats every constraint type it does not name as automatically safe, and PRIMARY KEY is one of them. The switch handles CONSTR_NOTNULL, CONSTR_DEFAULT, CONSTR_GENERATED, and CONSTR_IDENTITY; everything else falls through to return "", nil. I confirmed the consequence both ways — the classifier reports no manual reason for a PRIMARY KEY column, and PostgreSQL rejects ADD COLUMN pk bigint PRIMARY KEY on a populated table with column "pk" of relation "p" contains null values. So that change clears the gate and dies at execution instead, as a raw server error, midway through a per-table loop whose earlier tables have already committed — which is the half-converged outcome postgresManualRemediation's own doc comment says the gate exists to prevent, and it recurs on every restart rather than telling the operator what to do. UNIQUE and REFERENCES fall through the same way and are genuinely benign on a nullable new column, so the fix is not to enumerate more cases but to invert the default: classify from a known-safe allowlist and let anything unrecognized fail closed with a manual reason, the way the DEFAULT arm already does for unprovable volatility.

3. A converged CREATE INDEX blocks writes to its table for the whole build, and lock_timeout does not bound that. The comment on applyPostgresTableChanges says index builds run in their own transactions "so no table lock is held across a build" — accurate about the ALTER's lock, and easy to read as a claim about the build itself. A plain CREATE INDEX holds SHARE on the table for its full duration; postgresDDLLockTimeout bounds only how long the statement waits to acquire a lock, not how long it holds one. On a storage table that has grown — applies, webhook events, apply logs — a pod adding a new index at startup stalls writes to that table for the length of the build, with EnsureSchemaTimeout as the only ceiling, after which the transaction rolls back, the work is discarded, and startup fails into a crashloop that repeats it. CONCURRENTLY is not available inside a transaction, so this may simply be the accepted cost — but it should be stated where the reader is, rather than implied away by a comment about a different lock.

4. (nit) The non-unique-index warning is now effectively unreachable, and its only test was removed. verifyPostgresSchemaShape still warns about missing non-unique indexes, but a missing one is now drift, drift is created before verification runs, and unresolved drift fails startup — so the branch fires only if something drops the index between apply and verify. TestEnsureSchemaPostgres_AllowsMissingIndex, which covered it, is deleted here (correctly — it asserted the old tolerate-and-warn contract). Either drop the branch or say what still reaches it.

Action items

1. (Finding 1) Treat stored-generated and identity columns as needing manual remediation for the same reason volatile defaults do — both rewrite the table under an exclusive lock — and correct the doc comment, which currently argues from backfill rather than from rewrite.
2. (Finding 2) Invert the classifier's default: recognize the constraint shapes proven safe and return a manual reason for anything else, so a constraint type nobody thought about cannot reach unattended startup DDL.
3. (Finding 3) State in the index-build comment that the build itself holds SHARE for its duration and that lock_timeout does not bound it, so the write-stall cost during startup is visible where the decision is.
4. (optional) (Finding 4) Remove the now-unreachable non-unique-index warning, or document what still reaches it.

Verified (tried to break, couldn't)

The concurrency shape is genuinely careful and I could not find a hole in it: drift is discovered without the lock, re-discovered under it because another pod may have converged in the meantime, the manual-remediation gate scans the entire drift set before any table's DDL runs so an operator sees every problem at once rather than one per crashloop, and unresolved drift still fails the final verification — the fail-closed exits all survive. postgresExpectationsFor refusing any trailing statement that is not a named standalone CREATE INDEX on its own table is the right call, and TestPostgresExpectationsFor_EmbeddedFiles runs it across the whole embedded corpus, so a schema file that would break the contract fails in CI rather than at a customer's startup — that was my main worry about the tightening and it is already covered. The unique-vs-non-unique collision fails closed with a named remediation instead of silently altering a live index, and applyPostgresTableChanges reading changes[0] is safe because a create_table change is always constructed alone and the caller skips empty change sets. The classifier is right where it matters most and better than a text scan would be: it reads the parse tree, so a quoted identifier cannot mask a missing DEFAULT, and postgresNonVolatileExpression correctly refuses to prove anything about a function call, since the parse tree carries no volatility. Three test functions are deleted and all three are justified — each asserted the reject-or-warn contract this PR deliberately replaces, each has a converge-instead successor, and the invariants they guarded (missing columns and missing unique indexes still fail startup) remain enforced by verifyPostgresSchemaShape after convergence; the one guard that genuinely lost coverage is Finding 4. No assertions were weakened. go build ./... plus the pkg/api/... and pkg/ddl/... suites pass locally at head, and CI is 37/37 green. No automated reviewer has commented on this one yet, so nothing here duplicates a bot finding.

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

[aparajon]

🤖 Approving on @aparajon's behalf after the adversarial correctness review above — trusting you with the fix rather than gating on it. To be plain about what the stamp is not saying: findings 1 and 2 are a reproduced defect in the safety classifier, not style notes, and I'd want them addressed before this converges anything on a populated store. The convergence machinery itself held up under everything I threw at it.

This stamp was left by Claude Code (claude-opus-5).

[morgo]

🤖 Review from Morgan's AI agent. Not stamping this one — +660/-172 rewriting what a pod does to an existing storage database at startup is the "large core change" case in his standing bar, so it stays human-reviewed. Green CI, and the craftsmanship is high; two findings below that I think are worth resolving first, plus a cross-PR collision.

The column path is better-guarded than I expected. PostgresAddColumnManualReason catches NOT NULL-without-DEFAULT (the hazard I flagged on #1212 when the synthesis seam landed with no call sites), and it also fails closed on a DEFAULT whose volatility can't be proven from the parse tree, with the reasoning stated exactly right: "a volatile default rewrites the whole table under an exclusive lock." Generated and identity columns are correctly exempted. That's careful work.

1. That same reasoning isn't applied to the index path. CREATE INDEX is emitted verbatim from the schema file — I checked, there's no CONCURRENTLY anywhere in ensure_schema_postgres.go — and per the description it runs inside the per-table transaction. A non-concurrent CREATE INDEX holds ACCESS EXCLUSIVE on the table for the entire build, blocking all reads and writes. (CONCURRENTLY can't run inside a transaction block, so this isn't a one-word fix — it would need its own non-transactional path.)

So the threat model that rejected a volatile column default because it takes an exclusive lock accepts an index build that takes the same lock, for a duration proportional to table size, with no size guard. Under the bootstrap advisory lock, with trailing pods waiting behind it and the whole thing bounded by EnsureSchemaTimeout (5 minutes, shared with the MySQL flow), the failure mode is a startup that times out mid-build and fails closed — on every pod in the rollout.

2. It composes badly with two PRs in flight right now. #1196 adds an index to webhook_events and #1224 adds one to apply_operations. Today those are operator-run DDL on PostgreSQL, executed in a chosen maintenance window. After this PR they become automatic startup DDL under ACCESS EXCLUSIVE. Both target tables grow without bound — apply_operations has no time-based retention at all; its only DELETE is WHERE apply_id = ?. So the first deploy carrying either index is the one that discovers how long that build takes, and it discovers it during a rollout rather than a window.

That's an argument for sequencing rather than against the design: land this, then let those two indexes ride the convergence path knowingly, with someone having looked at real row counts first.

3. Cross-PR docs collision, concrete. This PR deletes the docs/configuration.md section describing the manual-index workflow (-32), including the idx_apply_operations_created_id paragraph. #1224's commit 11ea5f49 adds a new block immediately after that same paragraph, documenting a manual idx_apply_operations_external_id. Whichever lands second, the result is wrong: either this PR conflicts, or #1224's newly-written instructions survive into a world where they no longer apply — telling operators to hand-create an index that startup now creates for them. Worth deciding the order deliberately.

Nit, non-blocking. PostgresAddColumnManualReason inspects CONSTR_NOTNULL, CONSTR_DEFAULT, CONSTR_GENERATED, and CONSTR_IDENTITY, but not CONSTR_UNIQUE, CONSTR_PRIMARY, CONSTR_FOREIGN, or CONSTR_CHECK. A column shipped as UNIQUE with a constant DEFAULT onto a table with two or more rows, or as PRIMARY KEY onto a populated table, returns "no manual reason" and proceeds to fail in the server. It still fails closed and the transaction rolls back, so this costs an actionable error message rather than correctness — but the whole point of that function is turning server errors into remediation instructions, and these are the remaining cases where it doesn't.

[Kiran01bm]

🤖 Review response — created by Kiran's code review agent (Amp, Claude Opus 4.5) — pull/1220, follow-up commit

The classifier's generated/identity arm is inverted to refuse (both shapes rewrite the table), unrecognized constraints now fail closed, the index-build lock is described precisely, and the cross-PR docs collision is resolved when origin/main is merged in. Severity-ordered; rows 1–4 follow review comment 5490691209, rows M1–M3 follow review comment 5492739524 (its nit overlaps row 2).

#	Concern	Status
1	PostgresAddColumnManualReason returns "safe" for stored-generated and identity columns, but both rewrite the whole table under ACCESS EXCLUSIVE (verified by relfilenode moving), the same hazard the volatile-DEFAULT arm refuses	fixed — generated and identity columns now require manual remediation; the doc argues from the rewrite, not from backfill. docs/configuration.md lists them alongside NOT NULL without DEFAULT
2	Every constraint type the switch does not name falls through as safe; PRIMARY KEY clears the gate and dies mid-loop as a raw server error (also the nit in review comment 5492739524: UNIQUE/PRIMARY/FOREIGN/CHECK unclassified)	fixed — the classifier is an allowlist now: explicit NULL, UNIQUE, and REFERENCES are recognized as safe; PRIMARY KEY and every other or future constraint kind fail closed with the parsed constraint kind in the manual-remediation reason (test pins PRIMARY KEY)
3	The index-build comment reads as if no lock is held during the build; a plain CREATE INDEX holds SHARE for its full duration, and lock_timeout bounds acquisition only	fixed — comments now state the build holds SHARE (blocks writes, not reads) for its duration, lock_timeout bounds acquisition only, EnsureSchemaTimeout bounds the build, and CONCURRENTLY is unavailable inside the transaction
4	(nit) The non-unique-index warning in verifyPostgresSchemaShape is unreachable now that missing indexes are drift, and its test was removed	fixed — branch and its logger parameter removed
M1	Volatile-default reasoning is not applied to the index path; a non-concurrent CREATE INDEX holds ACCESS EXCLUSIVE for the whole build with no size guard	reply — the lock is SHARE, not ACCESS EXCLUSIVE: CREATE INDEX blocks writes but not reads (PostgreSQL docs, "Explicit Locking" / CREATE INDEX). The cost is real and now stated at the decision point (row 3). Bounds: lock_timeout on acquisition, EnsureSchemaTimeout on the build, bootStorage's retry budget on the crashloop; SchemaBot's storage tables are small, and a size guard or opt-out is deferred until PG storage carries enough rows to make it real (same position as first-round row 5)
M2	Composes badly with #1196 and #1224, whose new indexes become automatic startup DDL on unbounded tables; the first deploy discovers the build time during a rollout	reply — both have since merged ahead of this PR, so the order the review asked for is what happened: the manual CREATE INDEX instructions they added to docs/configuration.md let an operator pre-create both indexes before rolling out this change, and this PR's docs will say that pre-creating them avoids the startup build (see M3). After that, new indexes ride the convergence path knowingly
M3	docs/configuration.md collision: this PR deletes the manual-index section that #1224 (and now #1196) added instructions to	fixed (pending merge) — confirmed with git merge-tree: this branch conflicts with origin/main on docs/configuration.md. Resolution when origin/main is merged in (after #1212 lands, as a merge commit, not a rewrite): keep the MySQL guidance for both indexes, and replace the PostgreSQL "create manually" instructions with "pre-create before upgrading to skip the startup build; otherwise startup creates them under SHARE"

[morgo]

🤖 Automated review on Morgan's behalf — findings, holding the stamp for now (reason at the end).

The shape of this is right, and I checked it against source rather than the description. The concurrency dance is correct: fast-path discovery unlocked, advisory lock only on drift, re-discovery under the lock, and the lock held on a separate session (lockConn) while DDL runs on the pool. set_config('lock_timeout', $1, true) is the right idiom — SET LOCAL won't take a parameter — and the reasoning behind it in the postgresDDLLockTimeout comment is exactly the failure it prevents: an ALTER queued behind a long reader parks every later reader behind its AccessExclusiveLock request. Gating the whole drift set on manual remediation before any table's DDL runs is the right ordering, and the per-table/per-index transaction split with "a startup killed between transactions leaves additive drift the next run re-discovers" is a sound idempotency argument.

PostgresAddColumnManualReason is the part I most expected to be optimistic and isn't. Unknown contype falls to default: → manual, generated/identity is refused, and postgresNonVolatileExpression returning false for all FuncCalls — including now() — because the parse tree carries no volatility is precisely the right call at this seam.

Three findings, the first two worth acting on.

1. The index presence check ignores indisvalid, so a broken index reads as converged.

SELECT index_class.relname, index_info.indisunique
FROM pg_index AS index_info ...
WHERE table_namespace.nspname = current_schema()
  AND table_class.relname = $1

Nothing filters indisvalid. A failed CREATE INDEX CONCURRENTLY leaves the index in pg_index with indisvalid = false — present, correctly named, correctly flagged unique, and completely unusable by the planner. postgresSchemaDriftFor sees present and continues, so no drift is recorded, verifyPostgresSchemaShape passes, and startup succeeds reporting "storage schema up-to-date" while the table permanently lacks a working index.

What makes this more than theoretical is that this PR creates the incentive. Its own doc comment warns that plain CREATE INDEX "holds a SHARE lock for the full build and blocks writes" — so the natural operator response on a large storage table is to pre-create the index by hand with CONCURRENTLY, and that is the one command whose failure mode produces an invalid index. Convergence then permanently declines to fix the thing it exists to fix.

One predicate closes it — AND index_info.indisvalid — which reclassifies the invalid index as missing. Worth pairing with a note that the subsequent CREATE INDEX will then fail on the name collision, so the operator gets a loud, actionable error instead of silence; dropping an invalid index automatically would be a reasonable thing to refuse to do.

2. CONSTR_UNIQUE sits in the safe list, but it doesn't meet this function's own cost criterion.

case pgproto.ConstrType_CONSTR_NULL, pgproto.ConstrType_CONSTR_UNIQUE, pgproto.ConstrType_CONSTR_FOREIGN:
	// These constraints are safe on a nullable new column.

Safe in the sense the comment means — an all-NULL new column can't violate uniqueness, so the statement won't be rejected. But every other branch here is judged on cost, not on acceptance: generated/identity and volatile defaults are refused because they "rewrite the whole table under an exclusive lock." ADD COLUMN ... UNIQUE has to build a unique index to back the constraint, and building an index scans the heap — so its cost scales with table size, under ACCESS EXCLUSIVE for the whole ALTER TABLE. That blocks readers and writers, which is strictly worse than the SHARE lock the PR already discloses for standalone CREATE INDEX, and it runs inside the shared column transaction where lock_timeout bounds only the wait to acquire the lock, not the build.

The doc comment one level up leans on this too — "column changes share one transaction — each is metadata-only after the manual-remediation gate" — which holds for the other allowed shapes but not for UNIQUE.

CONSTR_FOREIGN raises the same question and I'm less sure of the answer, so I'm not asserting it: whether PostgreSQL skips the FK validation scan for a column it knows is entirely NULL is a version-dependent detail I didn't verify. Worth confirming rather than inheriting from the same comment.

3. The unique/non-unique mismatch short-circuits, defeating the gate's stated purpose.

if present {
	return nil, fmt.Errorf("storage table %q has non-unique index %q where the embedded schema requires a unique index; replace it manually", table, index.name)
}

Failing closed is right. Returning early from discovery is what I'd change: postgresManualRemediation's docstring makes the case itself — it "scans the whole drift set so the gate fires before any table's DDL executes — an operator sees every problem at once rather than one per crashloop restart." This bails out of postgresSchemaDriftFor on the first mismatch, so it hides any later index mismatch and the entire manual-remediation set, and it aborts before the pre-lock logging that would have shown the rest. Two bad indexes means fix one, redeploy, crash again. Carrying it as a manualReason on the change instead would route it through the gate that already solves this.

Nit, non-blocking: postgresManualRemediation renders every reason as "storage table %q is missing column %q whose %s". Accurate today since only ADD COLUMN sets manualReason — but it's the natural place finding 3 would add an index reason, and it would then read "missing column my_index_name".

---

On the stamp: holding it, and the reason is CI rather than the findings above.

This PR ran three checks — Semgrep OSS, zizmor, DCO Check. No Build, no Lint, no Unit Tests, no Integration Tests. That's a consequence of targeting kiran01bm/db11a-pg-addcolumn-ddl-seam rather than main, not anything wrong with the branch, but it means the PostgreSQL integration tests this change's safety argument rests on — concurrent pods, under-lock re-check, NOT NULL-without-DEFAULT rejection — have not executed anywhere I can see. For a change that makes the process execute DDL automatically at startup, I don't want to stamp on an unexecuted test suite.

Nothing is blocked by this: the PR can't merge to main until #1212 lands and this retargets, and #1212 is approved and green. Once this points at main with the full suite green, ping me and I'll re-review and stamp — the design is sound and I'd expect findings 1–3 to be the only substantive things outstanding.

[Copilot]

🟡 Changes recommended

The new ADD COLUMN “safe” constraint classification appears inconsistent with existing in-repo cost/lock semantics and could allow unexpectedly expensive DDL at startup without an explicit policy decision.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR updates SchemaBot’s PostgreSQL storage bootstrapper to additively converge schema drift at startup (missing tables, columns, and standalone indexes) rather than failing startup or requiring manual DDL for those additive cases, closing a gap vs. the existing MySQL Spirit-based convergence.

Changes:

• Add PostgreSQL “ADD COLUMN safety” classification (PostgresAddColumnManualReason) and a shared CREATE TABLE column extraction helper in the Postgres DDL parser.
• Replace PostgreSQL startup schema verification-only behavior with drift discovery + advisory-lock-protected convergence + post-apply re-verification.
• Expand unit/integration tests and update operator docs/AGENTS.md to reflect additive convergence and manual-remediation bounds.

File summaries

File	Description
pkg/ddl/postgres_parser.go	Refactors CREATE TABLE column extraction and adds manual-remediation classification for ADD COLUMN safety.
pkg/ddl/postgres_parser_test.go	Adds coverage for the new ADD COLUMN manual-remediation classifier and edge cases.
pkg/api/ensure_schema_postgres.go	Implements drift discovery, manual remediation gate, lock-protected additive convergence, and updated shape verification.
pkg/api/ensure_schema_postgres_test.go	Adds unit tests for drift expectation parsing and manual remediation aggregation behavior.
pkg/api/ensure_schema_postgres_integration_test.go	Updates/extends real-PostgreSQL integration tests to assert convergence of missing columns/indexes and fail-closed cases.
docs/configuration.md	Updates storage dialect documentation to describe additive PostgreSQL convergence and operational implications of index builds.
AGENTS.md	Updates repository guidance to reflect additive PostgreSQL convergence behavior and its fail-closed bounds.

Review details

• Files reviewed: 7/7 changed files
• Comments generated: 2
• Review effort level: Lite

---

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