Skip to content

fix: commit enum ADD VALUE before statements that use the new label (#600) - #618

Merged
tianzhou merged 3 commits into
mainfrom
fix/issue-600-enum-add-value-commit-boundary
Sep 20, 2026
Merged

tianzhou merged 3 commits into
mainfrom
fix/issue-600-enum-add-value-commit-boundary

Conversation

@tianzhou

Copy link
Copy Markdown
Contributor

Summary

Adding an enum label and using it in the same plan (e.g. as a column default) produced a plan that failed on apply:

ERROR: unsafe use of new value "active" of enum type status (SQLSTATE 55P04)

PostgreSQL requires ALTER TYPE ... ADD VALUE to be committed before the new label can be used, but groupDiffs batched every non-rewrite statement into one transaction group.

Fix:

  • Add SQLStatement.RequiresCommitAfter (not serialized) and set it for enum ADD VALUE statements.
  • groupDiffs closes the current execution group right after such a statement. ADD VALUE still shares a transaction with the steps before it; everything after it starts a new group.

The plan now shows the boundary explicitly:

-- Transaction Group #1
ALTER TYPE status ADD VALUE 'archived' AFTER 'pending';

-- Transaction Group #2
ALTER TABLE work ALTER COLUMN state SET DEFAULT 'archived'::status;

Atomicity note: if a later group fails, the added label stays. That is harmless — re-planning sees the label as present and emits only the remaining steps.

Fixes #600

Test plan

Folded the scenario into testdata/diff/create_type/add_type (the existing enum ADD VALUE case): a table whose default switches to the newly added label. Failed with 55P04 before the fix, passes after.

PGSCHEMA_TEST_FILTER="create_type/" go test -v ./internal/diff -run TestDiffFromFiles
PGSCHEMA_TEST_FILTER="create_type/" go test -v ./cmd -run TestPlanAndApply
go test ./internal/plan

🤖 Generated with Claude Code

…600)

ALTER TYPE ... ADD VALUE was batched into the same transaction group as
later statements, so a plan that added a label and used it (e.g. as a
column default) failed on apply with SQLSTATE 55P04. Mark ADD VALUE
statements as requiring a commit afterwards and close the execution
group right after them.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings September 20, 2026 07:49
@greptile-apps

greptile-apps Bot commented Sep 20, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 4/5

The PR is not yet safe to merge because plans still fail when a newly created object uses a label being added to an existing enum.

Findings

  1. P1 Enum consumers can run first ▶

Summary

This PR marks enum ADD VALUE statements as requiring a following transaction boundary and updates plan grouping so later statements execute after that transaction commits.

  • Propagates commit-boundary metadata from enum diff generation into planning.
  • Splits execution groups immediately after each enum-label addition.
  • Adds a regression fixture for changing an existing table default to the new label.
  • The fix remains incomplete for newly created objects that use the label because create operations are ordered before enum modifications.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Existing enum gains archived label]
    B[Create phase]
    C["CREATE TABLE work<br/>DEFAULT archived::status"]
    D[Modify phase]
    E["ALTER TYPE status<br/>ADD VALUE archived"]
    F[Commit boundary after ADD VALUE]
    G[Apply fails before reaching ADD VALUE]

    A --> B
    B --> C
    C --> G
    A --> D
    D --> E
    E --> F
Loading

Reviews (1) · Last reviewed commit: "fix: commit enum ADD VALUE before statem..."

Comment thread internal/plan/plan.go

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Create-phase objects can still reference a new enum label before its addition is emitted.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity

Open (1)
What changed in this PR

Adds a commit boundary after PostgreSQL enum values are added so later statements can safely use them.

Changes:

  • Marks enum ADD VALUE statements as requiring a commit.
  • Splits execution groups at the new boundary.
  • Extends the enum fixture with a dependent column default.
File Description
testdata/​diff/​create_type/​add_type/​plan.txt Updates human-readable plan output.
testdata/​diff/​create_type/​add_type/​plan.sql Adds the default alteration SQL.
testdata/​diff/​create_type/​add_type/​plan.json Expects two execution groups.
testdata/​diff/​create_type/​add_type/​old.sql Adds the existing test table.
testdata/​diff/​create_type/​add_type/​new.sql Uses the new enum label as a default.
testdata/​diff/​create_type/​add_type/​diff.sql Updates expected migration SQL.
internal/​plan/​plan.go Closes groups after commit-required statements.
internal/​diff/​type.go Marks enum additions as commit-required.
internal/​diff/​diff.go Adds statement commit-boundary metadata.
internal/​diff/​collector.go Propagates commit-boundary metadata.

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread internal/diff/type.go Outdated
A newly created object (e.g. a table with a column default) may use a
label being added to an existing enum. Emit enum label additions ahead
of the create phase so they run, and commit, before any consumer.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

The current ordering can commit preceding destructive drops if a later migration group fails.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Resolved since last review (1)

Comment thread internal/diff/diff.go Outdated
ADD VALUE forces a commit right after it, so drops emitted ahead of it
were committed too and could not roll back if a later group failed.
Emit enum label additions first so the unavoidable partial commit is
limited to the additive enum change.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Apply must retain one database session across execution groups so session-scoped settings remain effective.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Resolved since last review (1)

Comment thread internal/plan/plan.go
@tianzhou
tianzhou merged commit d862424 into main Sep 20, 2026
2 checks passed
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.

Adding an enum label and using it as a default produces a plan that fails with SQLSTATE 55P04

2 participants