fix: use pg_depend ownership instead of name guessing for SERIAL detection#10167
Open
kundansable wants to merge 4 commits into
Open
fix: use pg_depend ownership instead of name guessing for SERIAL detection#10167kundansable wants to merge 4 commits into
kundansable wants to merge 4 commits into
Conversation
…ction
get_formatted_columns() guessed a column's owned sequence via
make_object_name(table, col, 'seq') and rewrote nextval(...) defaults
to SERIAL only when that literal name matched. This breaks in two
ways:
- Renaming a table/column changes the guessed name but not the
sequence's actual name, so the column stops being recognized as
SERIAL and the reverse-engineered script emits a broken
nextval('old_seq'::regclass) that fails on a fresh database.
- A column with a nextval() default that happens to reference an
unrelated, coincidentally-named sequence gets silently rewritten as
SERIAL, orphaning the real sequence.
The properties template already exposes the column's actual owned
sequence (seqrelid) via a LEFT JOIN on pg_depend. Use that instead of
guessing, so SERIAL is only emitted when a genuine ownership
dependency exists, independent of naming.
Fixes pgadmin-org#10100, pgadmin-org#10101
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough
ChangesSERIAL detection
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant properties_sql
participant pg_depend
participant get_formatted_columns
properties_sql->>pg_depend: resolve default sequence dependency
pg_depend-->>properties_sql: return defseqrelid
properties_sql-->>get_formatted_columns: provide column metadata
get_formatted_columns->>get_formatted_columns: compare seqrelid and defseqrelid
get_formatted_columns-->>get_formatted_columns: reproject eligible columns as SERIAL
Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/utils.py`:
- Around line 288-290: Update the serial-detection condition around
col.get('seqrelid') to require that the column default expression is also
recorded as depending on the same sequence OID (cs.oid) from properties.sql.
Ensure split ownership/default cases are excluded from serial conversion and
retain their original nextval default, and add a regression test covering a
column owned by one sequence whose default references another.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3275255d-ffcc-438c-9356-dcab5eae5b1e
📒 Files selected for processing (1)
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/utils.py
Ownership and default are independent dependencies in PostgreSQL: a
column can own one sequence (pg_depend deptype='a', the existing
seqrelid check) while its DEFAULT's nextval() call names a completely
different one (pg_depend deptype='n' from the pg_attrdef entry). The
previous check only verified ownership, so a column in this split
ownership/default state was still wrongly reprojected as SERIAL,
discarding its real, distinct nextval() default.
Expose the default's own sequence dependency as defseqrelid in
properties.sql and require it to equal seqrelid before treating a
column as SERIAL, so split ownership/default columns keep their
original explicit nextval() default.
Add a mock-based regression test covering the split case, a genuine
SERIAL column, and identity columns, following the pattern already
used for shared-server unit tests since BaseTestGenerator's setUp()
needs a live server connection.
Verified against a real PostgreSQL 13 instance: a column owning
seq_owned but defaulting to nextval('seq_used') round-trips as
DEFAULT nextval('seq_used'::regclass), not SERIAL.
E127 continuation-line indent and E501 line-length (82 > 79) on the UTILS_MODULE string and the _make_column() defaults dict.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/tests/test_serial_detection_unit.py (1)
47-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnnotate
scenariosasClassVar.Ruff RUF012 flags this mutable class attribute. It is intentionally shared test metadata, so annotate it as
ClassVarrather than suppressing the warning.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/tests/test_serial_detection_unit.py` around lines 47 - 54, Annotate the shared mutable scenarios class attribute with ClassVar in the relevant test class, adding the necessary typing import if absent. Preserve the existing scenario metadata and avoid suppressing Ruff RUF012.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/columns/sql/default/properties.sql`:
- Around line 40-47: Update the defseqdep join and defseqrelid derivation so
only sequence relations are considered and multiple dependency rows resolve
deterministically. Join defseqdep through pg_class with relkind = 'S', then add
the required stable tie-breaker or pre-collapse before comparing defseqrelid
with seqrelid; preserve the existing sequence dependency behavior.
---
Nitpick comments:
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/tests/test_serial_detection_unit.py`:
- Around line 47-54: Annotate the shared mutable scenarios class attribute with
ClassVar in the relevant test class, adding the necessary typing import if
absent. Preserve the existing scenario metadata and avoid suppressing Ruff
RUF012.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1f48241f-1544-4b4e-b4c2-e1813acc51ab
📒 Files selected for processing (3)
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/tests/test_serial_detection_unit.pyweb/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/utils.pyweb/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/columns/sql/default/properties.sql
🚧 Files skipped from review as they are similar to previous changes (1)
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/utils.py
The defseqdep join filtered the default's own pg_depend row down to classid='pg_attrdef' and deptype='n', but never checked that the referenced object (refobjid) is actually a sequence. A default expression can carry other deptype='n' dependencies on non-sequence pg_class objects too - e.g. a composite type referenced via a cast inside the expression - which would fan the LEFT JOIN out to multiple rows for the same column and let SELECT DISTINCT ON (att.attnum) pick an arbitrary one, occasionally resolving defseqrelid to something other than the intended sequence. Verified against a real server: a default expression referencing both a sequence and a composite type produces two deptype='n' pg_depend rows, one per object. Join through pg_class with relkind='S' so only sequence dependencies are considered, and collapse the lookup into a scalar subquery (the same pattern already used for typnspname/isdup elsewhere in this template) with an ORDER BY ... LIMIT 1 tie-breaker, so it can never fan out the outer query regardless of how many dependencies exist. Existing single-dependency behavior (the common case) is unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #10100 and #10101 — Schema Diff's "Generate Script" (and the browser tree's CREATE Script view) can emit wrong SQL for
SERIAL/identity columns, in two opposite ways: it fails to recognize a genuineSERIALcolumn after a rename, and it wrongly promotes an unrelated column toSERIALwhen a coincidentally-named sequence exists.Root Cause
get_formatted_columns()detects whether a column owns a sequence (and should be rendered asSERIAL) by guessing the sequence's name as<table>_<col>_seqand checking whether that literal string appears in the column'snextval(...)default. This guess breaks in both directions:DEFAULT nextval('old_name'::regclass)instead ofSERIAL— and that script fails on a fresh database withrelation "old_name" does not exist.<table>_<col>_seq, the guess matches even though the column doesn't actually own that sequence. The column gets wrongly rewritten asSERIAL, which creates and takes ownership of a new sequence, orphaning the real one.Both are the same underlying problem: guessing by name instead of asking PostgreSQL which sequence a column actually owns. That ownership is already tracked in
pg_depend, and the properties template already exposes it via aLEFT JOINasseqrelid— it just wasn't being used for this check.Fix
Replace the name-guessing check in
get_formatted_columns()with a check againstcol.get('seqrelid')(plus the existingnextval('+ integer-type guards), and additionally exclude identity columns (attidentity).SERIALis now emitted if and only if a genuinepg_dependownership dependency exists, independent of naming.Test Steps
invoices→ CREATE Script (or Schema Diff → Generate Script). Expectid serial NOT NULL— notDEFAULT nextval('orders_id_seq'::regclass).mytable. Expect the column to stay asDEFAULT nextval('mytable_id_seq'::regclass)— it must not becomeSERIAL.CREATE TABLE t (id serial PRIMARY KEY)with no rename, generate its script, confirm it still correctly emitsSERIAL.web/regression/runtests.py --pkg databases.schemas.tables), confirming no unrelated column tests regress.Summary by CodeRabbit
Bug Fixes
SERIAL,SMALLSERIAL, andBIGSERIALcolumns using sequence ownership information.nextval(...)cases) and removes redundant stored defaults.Tests
Documentation