Skip to content

schemachange: refuse a new identity or serial column at the shadow build - #271

Open
Kiran01bm wants to merge 1 commit into
mainfrom
kiran01bm/cs-pd9-identity-column-refusal
Open

Kiran01bm wants to merge 1 commit into
mainfrom
kiran01bm/cs-pd9-identity-column-refusal

Conversation

@Kiran01bm

Copy link
Copy Markdown
Collaborator

The shadow build now refuses a gated statement that gives the shadow an identity or serial column the source does not have, with a new typed cause shadow-statement-new-sequence, instead of copying every row and stopping at the swap.

Why

The swap carries only the source's sequences to the live table: each is re-owned by the shadow column that kept its source column's name, and the swap's catalog re-read proves the live table has exactly the source's identity columns and sequences. A sequence the statement minted on the shadow — ADD COLUMN … GENERATED … AS IDENTITY, ADD COLUMN … serial, ALTER COLUMN … ADD GENERATED, or a dropped column re-added as one — has no source counterpart in that handoff, so a run carrying one did the whole copy and then failed cutover-swap-mismatch, leaving the shadow and the hold on the source for --restart. That is knowable the moment the statement has run on the empty shadow, so ST-6 says refuse it there.

This is an engine-layer guard, not a CLI-visible change. migrate already routes every one of these shapes away before the build: a new identity or serial column is a volatile-default ADD COLUMN the copy-and-swap backend does not carry (backend-unavailable), and ADD GENERATED on an existing column is unsupported-operation. The copy-then-swap-failure was reachable only through the library (schemachange.Runner, BuildShadow). The docs claimed the re-add case "builds, copies, and then stops at the swap" as if that were the design; this PR corrects that too.

What

  • pkg/schemachange/new_sequence_column.go: readSequenceOwnedColumns reads, inside the build's transaction, every shadow column with attidentity <> '' or a pg_depend 'a'/'i' edge to a sequence; refuseNewSequenceColumns refuses the first with CauseStatementNewSequence (class capability-boundary, invariant ST-6). The shadow is built LIKE … EXCLUDING IDENTITY, a kept source identity becomes DEFAULT nextval(<source seq>) without ownership, and a source serial's default is copied without its OWNED BY edge, so before the statement no shadow column owns a sequence — every hit was minted by the statement, and the fixture's id bigserial makes every existing build test a no-false-positive check.
  • buildShadow runs the check after refuseUnfillableReadds (so a re-added source identity keeps shadow-statement-readd) and before verifyIdentityDefaults.
  • Tests: three unit tests on the refusal; integration tests for a new identity column beside a retype, a new serial, ADD GENERATED on a kept column, a retype beside the source's own serial key (admitted), the Runner path (no shadow, fence, or checkpoint row), and the former TestBuildShadowDoesNotRefuseAPlainColumnReaddedAsAnIdentity inverted to assert the new cause.
  • Docs: refusal-classes.md new row and corrected shadow-statement-readd row; limitations.md new row and corrected re-add row; invariants.md ST-6 Enforced line and ST-11 cross-reference; capabilities.yaml → capabilities.md; README pointer.

Before / after

One statement through the library path against the orders fixture (id bigserial PRIMARY KEY, qty integer NOT NULL, note text, sku text, 2500 rows): ALTER TABLE orders ADD COLUMN seqno bigint GENERATED BY DEFAULT AS IDENTITY, ALTER COLUMN qty TYPE bigint. Before, the build committed and the run copied every row before the swap's catalog re-read found a sequence it had not carried; after, the build refuses inside its own transaction and nothing exists to clean up. The migrate CLI refuses this statement as backend-unavailable before the build on both sides, unchanged.

Before                                                  After
──────                                                  ─────
BuildShadow                                             BuildShadow
  CREATE TABLE orders__shadow (LIKE orders …)             CREATE TABLE orders__shadow (LIKE orders …)
  ALTER TABLE orders__shadow ADD COLUMN seqno …           ALTER TABLE orders__shadow ADD COLUMN seqno …
    IDENTITY, ALTER COLUMN qty TYPE bigint                  IDENTITY, ALTER COLUMN qty TYPE bigint
  refuseUnfillableReadds → ok (no re-add)                 refuseUnfillableReadds → ok (no re-add)
  (no sequence check)                                     readSequenceOwnedColumns
                                                            → [seqno: identity]   ◀── minted by the statement;
  COMMIT  ── shadow exists, checkpoint row written              id's sequence is the source's, not owned
                                                          refuseNewSequenceColumns
Copy 2500 rows in chunks ─────────── minutes to hours       → ErrInvariantViolation,
Verify checksum                                                  cause shadow-statement-new-sequence
Cutover                                                   ROLLBACK ── no shadow, no fence, no checkpoint row
  lock, rename, carry source sequences
  catalog re-read: live table owns a sequence           Caller re-plans: add seqno plain here,
    the source never had                                  ADD GENERATED in a later schema change
  → cutover-swap-mismatch, ROLLBACK
  shadow + hold on orders stay for --restart

🤖 Generated with Amp (Claude Opus 4.6)

The swap carries only the source's sequences, so a sequence the
statement minted on the shadow was found only after the full copy, at
cutover-swap-mismatch. Refuse it in the build transaction instead (ST-6).
Library-path guard: the migrate CLI already routes these shapes away.
@Kiran01bm
Kiran01bm marked this pull request as ready for review October 28, 2026 02:48
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ⚠️ Failed 2026-10-28T02:48:27.839011Z
We couldn’t complete this review. Comment @codex review to try again.
c2930d2 Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@Kiran01bm
Kiran01bm requested a review from morgo as a code owner October 28, 2026 02:48
@chatgpt-codex-connector

Copy link
Copy Markdown

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

@JashLal

JashLal commented Oct 28, 2026

Copy link
Copy Markdown
Collaborator

🤖 Adversarial correctness review by Jash's AI agent at c2930d253e8da5337e85900256bbafccc11f82c8.

Verdict: the inspected changes support the intended early refusal; no confirmed correctness blocker found, with verification incomplete. The new catalog query checks identity attributes and serial-style sequence ownership separately, qualifies catalog operators, and propagates read failures. The guard follows the re-add check, preserving its refusal priority, and precedes the handoff proof. The added Runner test asserts the typed cause, no shadow, no fence, no checkpoint row, and a writable source.

The renamed re-added-identity test deliberately replaces successful-build assertions with the new refusal and no-shadow assertions; that matches the behavior change, and the separate re-add unit coverage remains. No unjustified test deletion found. All 16 reported CI checks succeeded, including PostgreSQL 14–18, build and unit tests. Exact-head repository instructions, unchanged surrounding source and full inline threads were unavailable because API reads failed connecting to api.github.com. I did not independently verify the unchanged transaction cleanup or sequence handoff implementation, or run a local build/tests.

Generated by Codex.

@JashLal

JashLal commented Oct 28, 2026

Copy link
Copy Markdown
Collaborator

🤖 OSS adoption ease: refusing before copying rows gives callers earlier, structured feedback. Optional documentation improvement in docs/limitations.md and the remedy in pkg/schemachange/new_sequence_column.go: clarify that a populated-table identity conversion may need a backfill and NOT NULL prerequisites, and identify the standalone SQL follow-up because ADD GENERATED remains unsupported through the CLI. The proposed two-step remedy should not imply that both steps already run through pg-sprite.

SchemaBot integration: the public refusal cause, capability-boundary classification and invariant mapping allow structured handling without parsing the message. The consuming adapter was not verified in this review.

Generated by Codex.

@JashLal JashLal left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Approving on Jash's behalf after the review at c2930d2. No confirmed correctness blocker found in the full diff; the optional staged-identity remedy clarification is recorded in the product review. Exact-head surrounding source and inline threads were unavailable, and no local build or tests ran. Approval does not certify those unverified paths. Generated by Codex.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants