fix: track altering identities

[sam-mosleh]

Fixes #279

[greptile-apps]

Greptile Overview

Greptile Summary

This PR adds diff/plan support for identity-generation changes and serial↔non-serial transitions by:

• Detecting nextval('...') defaults to create/drop sequences around default changes.
• Emitting DROP IDENTITY / ADD GENERATED {ALWAYS|BY DEFAULT} AS IDENTITY when identity is added/removed or generation changes.
• Adding an online rewrite step that replays ADD GENERATED ... AS IDENTITY and then runs setval(pg_get_serial_sequence(...), MAX(col)+1) to sync the identity sequence.
• Adding a new regression fixture under testdata/diff/create_table/alter_identity/.

Main issues to address before merge are around correctness/safety of the generated SQL: unquoted identifiers for sequences, dropping sequences based only on defaults (can drop still-in-use objects), and the identity rewrite’s pg_get_serial_sequence call using schema-qualified table names as unescaped string literals. The rewrite dispatch is also currently substring-based and can duplicate identity DDL rather than rewriting the intended step.

Confidence Score: 2/5

• This PR is not safe to merge until SQL generation/rewrites are corrected for quoting and identity sequence syncing.
• Core behavior change is correct in intent, but the current implementation can generate invalid SQL for schema-qualified/mixed-case identifiers and can drop sequences that are still in use; the rewrite matching can also duplicate statements. Tests could not be executed in this environment (no Go toolchain).
• internal/diff/column.go, internal/plan/rewrite.go (and the new alter_identity fixtures to match the corrected behavior)

Important Files Changed

Filename	Overview
internal/diff/column.go	Adds sequence creation/drop based on nextval defaults and emits DROP/ADD IDENTITY when identity generation changes; contains unsafe/unquoted identifiers and potentially incorrect sequence dropping.
internal/plan/rewrite.go	Adds rewrite generator to sync identity sequence after ADD GENERATED; uses unescaped table name in SQL string literals and may generate invalid pg_get_serial_sequence args for schema-qualified tables.
testdata/diff/create_table/alter_identity/diff.sql	Adds expected diff output for serial/identity transitions; reflects unqualified names and missing quoting from implementation.
testdata/diff/create_table/alter_identity/new.sql	Adds desired schema fixture covering serial and identity columns.
testdata/diff/create_table/alter_identity/old.sql	Adds current schema fixture covering serial and identity columns.
testdata/diff/create_table/alter_identity/plan.json	Adds plan fixture including setval rewrites; includes pg_get_serial_sequence('table1', ...) calls that may not work with schema-qualified tables.
testdata/diff/create_table/alter_identity/plan.sql	Adds plan SQL fixture including setval rewrites; mirrors potential schema-qualification issue.
testdata/diff/create_table/alter_identity/plan.txt	Adds plan text fixture reflecting identity rewrite steps; mirrors potential schema-qualification issue.

Sequence Diagram

sequenceDiagram
  participant Planner as plan.generateRewrite
  participant Diff as diff.ColumnDiff.generateColumnSQL
  participant IR as ir.Column
  participant Plan as plan.generateColumnIdentityRewrite
  
  Planner->>Diff: DiffTypeTableColumn alter
  Diff->>Diff: extractSequenceNameFromDefault(old/new)
  alt serial default added
    Diff->>Diff: CREATE SEQUENCE IF NOT EXISTS ... OWNED BY table.column
    Diff->>Diff: ALTER COLUMN ... SET DEFAULT nextval(...)
  else serial default removed
    Diff->>Diff: DROP SEQUENCE ...
  end
  alt identity changed or removed
    Diff->>Diff: ALTER COLUMN ... DROP IDENTITY
  end
  alt identity added or generation changed
    Diff->>Diff: ALTER COLUMN ... ADD GENERATED {ALWAYS|BY DEFAULT} AS IDENTITY
  end
  
  Planner->>Planner: scan d.Statements for "ADD GENERATED"
  Planner->>Plan: generateColumnIdentityRewrite
  Plan->>Plan: ALTER TABLE ... ADD GENERATED ... AS IDENTITY
  Plan->>Plan: SELECT setval(pg_get_serial_sequence(table, col), COALESCE(MAX(col),0)+1)

Loading

[greptile-apps]

_{8 files reviewed, 4 comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Additional Comments (1)

internal/plan/rewrite.go
Over-broad rewrite match

The rewrite selection checks strings.Contains(stmt.SQL, "ADD GENERATED") across d.Statements. If any statement in the diff group contains that substring, generateColumnIdentityRewrite returns a new ADD GENERATED ... step, which can duplicate the base diff statement(s) or rewrite the wrong statement when multiple columns are altered in the same group. The rewrite dispatch should target the specific statement being rewritten (or rewrite the plan step in-place) rather than adding a second ADD GENERATED unconditionally.

Prompt To Fix With AI

This is a comment left during a code review.
Path: internal/plan/rewrite.go
Line: 91:105

Comment:
**Over-broad rewrite match**

The rewrite selection checks `strings.Contains(stmt.SQL, "ADD GENERATED")` across `d.Statements`. If any statement in the diff group contains that substring, `generateColumnIdentityRewrite` returns a new `ADD GENERATED ...` step, which can duplicate the base diff statement(s) or rewrite the wrong statement when multiple columns are altered in the same group. The rewrite dispatch should target the specific statement being rewritten (or rewrite the plan step in-place) rather than adding a second `ADD GENERATED` unconditionally.

How can I resolve this? If you propose a fix, please make it concise.

[greptile-apps]

Greptile Overview

Greptile Summary

This PR adds proper tracking and migration support for PostgreSQL identity column changes, addressing issue #279.

Key changes:

• Identity column alterations: Tracks changes to identity generation types (ALWAYS vs BY DEFAULT), properly generating DROP IDENTITY followed by ADD GENERATED ... AS IDENTITY statements
• Serial ↔ identity conversions: Handles conversions between serial columns (using nextval() defaults) and identity columns by managing associated sequences and skipping redundant DROP DEFAULT operations
• Sequence ownership tracking: Sequences now include OWNED BY clause during creation to properly track ownership relationships
• Smart sequence filtering: Only skips serial-owned sequences when the owning column doesn't exist in the other schema state, allowing proper sequence management when converting column types
• Identity synchronization: Added rewrite logic that syncs identity sequences with existing table data using setval(pg_get_serial_sequence(...)) to prevent primary key conflicts when adding identity to populated columns

The implementation correctly handles three scenarios demonstrated in the test case:

1. Converting regular column to serial (creates sequence + sets default)
2. Converting serial to identity (drops old sequence, adds identity, syncs sequence value)
3. Changing identity generation type (drops and re-adds identity with new generation)

Confidence Score: 5/5

• This PR is safe to merge with comprehensive test coverage and well-designed logic for identity column migrations.
• The implementation is thorough and handles the complex interactions between sequences, defaults, and identity columns correctly. The code includes comprehensive test cases that validate all three conversion scenarios, proper sequence synchronization to prevent data conflicts, and smart filtering logic that avoids dropping/creating sequences unnecessarily. The rewrite logic ensures data safety by syncing sequences before use.
• No files require special attention - all changes follow established patterns in the codebase.

Important Files Changed

Filename	Overview
internal/diff/column.go	Added identity column change tracking: drops old identity if generation type changes, adds new identity configuration. Skips DROP DEFAULT for nextval() defaults when converting from serial to identity.
internal/diff/diff.go	Modified sequence filtering logic to only skip serial-owned sequences when the owning column doesn't exist in the other schema state. Added columnExistsInTables helper function. Fixed whitespace formatting in privilege fields.
internal/diff/sequence.go	Added OWNED BY clause to sequence creation SQL to properly track sequence ownership. Fixed whitespace formatting in constants.
internal/plan/rewrite.go	Added generateColumnIdentityRewrite function to handle identity column additions by adding identity then syncing sequence value with existing data using setval(pg_get_serial_sequence(...)). Integrated into rewrite detection logic.

Sequence Diagram

sequenceDiagram
    participant User
    participant Diff as diff.GenerateMigration
    participant Column as ColumnDiff
    participant Sequence as Sequence Logic
    participant Rewrite as plan.generateRewrite
    
    User->>Diff: Compare old.sql vs new.sql
    
    Note over Diff: Scan for column changes<br/>(identity additions/changes)
    
    Diff->>Column: Detect c1: int → serial
    Note over Column: Old has no identity,<br/>new has no identity<br/>(serial uses default nextval)
    Column->>Sequence: Check if c1 sequence exists in old
    Sequence-->>Column: No (column didn't exist)
    Sequence->>Diff: Create table1_c1_seq
    Column->>Diff: SET DEFAULT nextval('table1_c1_seq'::regclass)
    
    Diff->>Column: Detect c2: serial → identity ALWAYS
    Note over Column: Check if nextval() default exists
    Column->>Column: Skip DROP DEFAULT (has nextval)
    Column->>Diff: ADD GENERATED ALWAYS AS IDENTITY
    Column->>Sequence: Check if c2_seq should be dropped
    Sequence->>Sequence: Column still exists in new state
    Sequence-->>Diff: DROP SEQUENCE table1_c2_seq
    
    Diff->>Column: Detect c3: identity ALWAYS → BY DEFAULT
    Note over Column: Identity generation changed
    Column->>Diff: DROP IDENTITY
    Column->>Diff: ADD GENERATED BY DEFAULT AS IDENTITY
    
    Diff->>Rewrite: Check for rewrite steps
    
    Rewrite->>Rewrite: Detect ADD GENERATED in c2
    Note over Rewrite: generateColumnIdentityRewrite(c2)
    Rewrite->>Diff: Add setval step for c2
    
    Rewrite->>Rewrite: Detect ADD GENERATED in c3
    Note over Rewrite: generateColumnIdentityRewrite(c3)
    Rewrite->>Diff: Add setval step for c3
    
    Diff-->>User: Migration plan with synced sequences

Loading

[tianzhou]

LGTM. Thanks for the contribution