fix: only collapse genuinely owned sequences into SERIAL (#573)

[tianzhou]

Summary

pgschema treated any integer column with a nextval() default as a SERIAL column, and the sequence inspector query synthesized "ownership" for any sequence merely referenced by a column default. As a result a shared sequence (Hibernate-style nsl_global_seq used by many tables) or a custom-named sequence disappeared from dumps and plans while the column was rendered as SERIAL/BIGSERIAL. On apply PostgreSQL created <table>_<column>_seq and rewired the default to it, so plans were wrong and not idempotent.

Changes:

• Inspector query: ownership now comes only from pg_depend (SERIAL / OWNED BY / identity). The fallback join that inferred ownership from column_default LIKE '%nextval%' is removed.
• IR: new Column.IsSerial, set during normalization only when the default's sequence is owned by the column and has PostgreSQL's default <table>_<column>_seq name (with makeObjectName truncation). Only such columns render as SERIAL; everything else keeps its explicit DEFAULT nextval(...) and gets an explicit CREATE SEQUENCE.
• Diff: the "skip sequence, CREATE TABLE will create it" logic keys off IsSerial instead of raw ownership. For an explicitly created sequence whose owning column is created in the same migration, OWNED BY is deferred to an ALTER SEQUENCE ... OWNED BY after the tables, matching pg_dump ordering.

The Sakila dump fixture changes because that schema declares its sequences explicitly without OWNED BY; they are now dumped explicitly, which is the faithful representation (SERIAL would have made them owned on re-apply).

Fixes #573
Fixes #574
Fixes #576

Test plan

• New testdata/diff/create_sequence/issue_573_shared_sequence_default covering a shared sequence used by several tables, a custom-named owned sequence, and a genuine bigserial for contrast. Passes plan, apply, and the second-plan idempotency check.
• New unit tests for serialSequenceName, nextvalSequenceName, and markSerialColumns.

PGSCHEMA_TEST_FILTER="create_sequence/issue_573" go test -v ./internal/diff -run TestDiffFromFiles
PGSCHEMA_TEST_FILTER="create_sequence/issue_573" go test -v ./cmd -run TestPlanAndApply
go test ./ir
go test ./internal/diff
go test ./cmd/dump

All of the above pass locally.

🤖 Generated with Claude Code

[greptile-apps]

Greptile Summary

This PR narrows SERIAL recognition to conventionally named, column-owned sequences and preserves shared or custom sequence defaults explicitly.

• Removes default-expression-based ownership inference from sequence inspection.
• Adds normalized IsSerial classification and uses it for table and sequence rendering.
• Defers ownership attachment for explicitly created sequences and adds regression fixtures for shared, custom-owned, and genuine SERIAL sequences.

Confidence Score: 3/5

This PR should not merge until explicit sequence ownership is ordered after added columns and structural sequence changes survive transitions away from SERIAL.

An owned sequence targeting a new column on an existing table is attached before that column exists, and sequence parameter changes are silently dropped when only one side of a transition is classified as SERIAL.

Files Needing Attention: internal/diff/diff.go

Important Files Changed

Filename	Overview
ir/queries/queries.sql	Removes inferred sequence ownership from column defaults and relies on pg_depend ownership metadata.
ir/normalize.go	Derives IsSerial from ownership, integer type, plain nextval default, and PostgreSQL’s conventional sequence name.
internal/diff/diff.go	Uses IsSerial for sequence lifecycle decisions, but mishandles ownership ordering for added columns and suppresses structural changes across SERIAL classification transitions.
internal/diff/sequence.go	Supports creating explicit sequences without inline ownership and attaching ownership in a later statement.
internal/diff/table.go	Renders SERIAL only from the normalized IsSerial flag and otherwise preserves explicit defaults.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  Inspect[Inspect sequences and pg_depend ownership] --> Normalize[Normalize columns and mark genuine SERIAL]
  Normalize --> Compare[Compare old and desired IR]
  Compare --> Serial{Is implicit SERIAL?}
  Serial -->|Yes| TableDDL[Create table with SERIAL]
  Serial -->|No| SeqDDL[Create explicit sequence]
  SeqDDL --> ColumnDDL[Create or alter owning column]
  ColumnDDL --> Ownership[ALTER SEQUENCE OWNED BY]

Loading

Comments Outside Diff (1)

1. internal/diff/diff.go, line 1109-1117 (link)

   Serial transition drops sequence changes

   When a SERIAL-backed column becomes an explicit sequence user while retaining the sequence name, the old-side serial classification takes this comment-only branch and discards structural sequence changes, causing settings such as INCREMENT, CACHE, bounds, or CYCLE to remain unapplied and the resulting schema to differ from the desired state.

   Knowledge Base Used: Schema object diff generation

_{Reviews (1): Last reviewed commit: "fix: only collapse genuinely owned seque..." | Re-trigger Greptile}

[Copilot]

🟡 Changes recommended

The new deferred ALTER SEQUENCE ... OWNED BY step is currently labeled as a “create” operation in diff metadata, which makes plan summaries/JSON misleading and should be corrected before approval.

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

Pull request overview

Fixes sequence/serial handling so pgschema only collapses genuine SERIAL-backed sequences (owned and default <table>_<column>_seq naming), preventing shared or custom-named sequences from disappearing in dumps/plans and avoiding non-idempotent apply behavior (addresses #573, #574, #576).

Changes:

• Tightened sequence ownership detection to rely solely on pg_depend (removed “infer ownership from column default nextval()” fallback).
• Added IR-level Column.IsSerial derived during normalization using Postgres makeObjectName truncation logic and nextval() parsing.
• Updated diff generation to skip/create sequences based on IsSerial, and to defer OWNED BY to an ALTER SEQUENCE ... OWNED BY when the owning column is created later in the same migration.

File summaries

File	Description
testdata/dump/sakila/pgschema.sql	Updates fixture to emit explicit sequences and explicit DEFAULT nextval(...) for non-owned sequences instead of rendering as SERIAL.
testdata/diff/create_sequence/issue_573_shared_sequence_default/new.sql	New regression case covering shared sequence defaults, custom-named owned sequence, and genuine serial.
testdata/diff/create_sequence/issue_573_shared_sequence_default/diff.sql	Expected DDL now includes explicit CREATE SEQUENCE and deferred ALTER SEQUENCE ... OWNED BY.
testdata/diff/create_sequence/issue_573_shared_sequence_default/plan.sql	Expected plan SQL for the new regression case.
testdata/diff/create_sequence/issue_573_shared_sequence_default/plan.txt	Expected human plan output for the new regression case.
testdata/diff/create_sequence/issue_573_shared_sequence_default/plan.json	Expected JSON plan output for the new regression case.
testdata/diff/create_sequence/issue_573_shared_sequence_default/old.sql	Empty baseline for the new regression case.
ir/queries/queries.sql	Removes ownership inference from information_schema.columns and documents pg_depend-only ownership.
ir/queries/queries.sql.go	Regenerated sqlc output reflecting the updated sequences query.
ir/ir.go	Adds non-serialized Column.IsSerial flag to IR for accurate SERIAL rendering decisions.
ir/normalize.go	Adds markSerialColumns, serialSequenceName, and nextvalSequenceName to compute IsSerial.
ir/normalize_test.go	Adds unit tests for serial sequence naming, nextval parsing, and serial marking behavior.
internal/diff/table.go	Switches SERIAL detection to rely on Column.IsSerial instead of “integer + contains nextval”.
internal/diff/diff.go	Changes sequence skip logic to key off serial-ness; adds deferred sequence ownership emission.
internal/diff/sequence.go	Adds deferred OWNED BY handling and makes create SQL optionally omit inline ownership.
internal/diff/qualify_schema_test.go	Updates tests for new generateSequenceSQL signature.
internal/diff/identifier_quote_test.go	Updates tests for new generateSequenceSQL signature.

Review details

Files not reviewed (1)

• ir/queries/queries.sql.go: Generated file

• Files reviewed: 15/17 changed files
• Comments generated: 1
• Review effort level: Lite

---

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

[tianzhou]

Addressed the review feedback and the CI failures in 5626b5c:

• Deferred OWNED BY ordering (greptile): moved to the modify phase after ALTER TABLE ... ADD COLUMN; fixture extended with that scenario.
• Plan summary labeling (copilot): the deferred ALTER SEQUENCE ... OWNED BY is part of adding the sequence, so it stays a create step but the summary now counts multi-step additions of one object once (keyed on the source object, so overloaded functions sharing a path remain separate). widget_custom_seq no longer appears twice in the plan.
• CI: sequences owned by an ignored table are now skipped in the inspector (ignore tests), and the query test that asserted ownership inferred from column defaults was replaced with one asserting pg_depend-only ownership.

[Copilot]

🔵 Needs a closer look

It changes core schema introspection, IR normalization, and migration ordering behavior around sequences/SERIAL across multiple subsystems and warrants final human validation across supported PostgreSQL versions and fixtures.

Review details

Files not reviewed (1)

• ir/queries/queries.sql.go: Generated file

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

ir/normalize.go:41

• The comment says sequences are normalized before markSerialColumns(schema) runs, but normalizeSchema doesn't normalize sequences (only tables/functions/etc). This can confuse future readers about ordering assumptions; the call only needs normalized column types/defaults plus the already-inspected sequence metadata.

• Files reviewed: 19/20 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟢 Approval recommended

The changes align with the stated issues and are backed by targeted regression fixtures plus unit tests, and the updated diff/planning behavior appears consistent and internally coherent.

Review details

Files not reviewed (1)

• ir/queries/queries.sql.go: Generated file

• Files reviewed: 19/20 changed files
• Comments generated: 0 new
• Review effort level: Lite