fix(schema-diff): reverse-engineer SERIAL columns by default#10020
Open
asheshv wants to merge 1 commit into
Open
fix(schema-diff): reverse-engineer SERIAL columns by default#10020asheshv wants to merge 1 commit into
asheshv wants to merge 1 commit into
Conversation
Schema Diff's "Generate Script" emitted CREATE TABLE DDL with
the libpq-expanded form of SERIAL columns:
col integer NOT NULL DEFAULT nextval('table_col_seq'::regclass)
instead of preserving the SERIAL declaration. Applied to a clean
target, that script fails with `relation "table_col_seq" does
not exist` because the sequence is never created — exactly the
symptom in issue pgadmin-org#9896.
SERIAL detection already existed in
columns/utils.py:get_formatted_columns (added in 115208c for
ERD generation, Nov 2023), but was gated behind a
``with_serial_cols`` parameter that defaulted to False. Only ERD
and the Schema Diff *comparison* phase opted in via explicit
True. The Schema Diff *Generate Script* path and the browser
tree's *CREATE Script* view went through `_get_resql_for_table`
→ `_formatter(data)` without specifying it, so SERIAL detection
was skipped and pgadmin-org#9896 reproduced.
There is no use case where pgAdmin should knowingly emit the
expanded libpq form: SERIAL is purely a CREATE-time macro that
PostgreSQL expands into an integer column plus an implicit
sequence owned by the column. The detection logic is reversing
that expansion, which is the correct interpretation in all
contexts (DDL emission, comparison, ERD, properties dialog).
Flip the default to True at all four declaration sites:
- columns/utils.py: get_formatted_columns
- utils.py: BaseTableView._formatter
- utils.py: BaseTableView.fetch_tables
- __init__.py: TableView.fetch_tables
All four existing explicit-True callers (ERD x2, Schema Diff
compare x2) are unaffected. Silent callers that previously got
the buggy False — `_get_resql_for_table` (Schema Diff Generate
Script and browser CREATE Script), `select_sql` / `insert_sql` /
`update_sql` in the table node, and one path in the partition
flow — now correctly get SERIAL detection. The INSERT/UPDATE
generators stay safe because they exclude default-bearing columns
via the `has_default_val` flag (computed from `att.atthasdef` in
the column SQL template), not from the `defval` string that
SERIAL detection clears.
Adds a resql regression in pg/{12_plus, 14_plus, 16_plus}: a
CREATE TABLE scenario covering ``serial`` / ``bigserial`` /
``smallserial`` columns plus a plain ``text`` column, with the
expected reverse-engineered DDL preserving the SERIAL keywords.
Would have failed before this fix.
Known coverage gaps (not blockers for this PR):
- PG11 (uses ``pg/11_plus`` / ``pg/default``): test intentionally
not added there because PG11 still emits ``WITH (OIDS = FALSE)``
which the existing expected-output convention reflects, and
without a live PG11 instance I can't verify the expected SQL
variant. The source-side fix still applies to PG11.
- PPAS (``ppas/*`` test dirs): not added because PPAS output may
diverge subtly (oraplus / hyphenated identifiers) and I don't
have a PPAS instance to verify against. The source-side fix
applies equally to PPAS.
- The SERIAL detection heuristic is name-based
(``<table>_<col>_seq``); sequence/table/column renames break
detection. Pre-existing in 115208c; could be replaced with
proper ``pg_depend``-based detection in a follow-up.
Fixes pgadmin-org#9896
WalkthroughThis PR enables SERIAL column reverse-engineering by default in pgAdmin's table introspection APIs to fix Schema Diff DDL generation. Four public methods now default ChangesSERIAL Column Round-Trip Fix for Schema Diff
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
🧹 Nitpick comments (1)
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/tests/pg/12_plus/create_table_with_serial_cols.sql (1)
10-10: 💤 Low valueMinor observation: COLLATE clause on text column.
The
payloadcolumn includesCOLLATE pg_catalog."default". For a round-trip SERIAL test, this clause is likely unnecessary unless the test also validates collation preservation. Consider simplifying to match the MSQL fixture (line 6 increate_table_with_serial_cols_msql.sql) if collation is not part of the test objective.🤖 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/tests/pg/12_plus/create_table_with_serial_cols.sql` at line 10, The payload column in create_table_with_serial_cols.sql currently specifies COLLATE pg_catalog."default"; remove the COLLATE clause from the payload text column so the DDL matches the MSQL fixture (create_table_with_serial_cols_msql.sql) and keeps the test focused on SERIAL round-trip behavior; update the column definition for payload in the CREATE TABLE statement to simply be "payload text" (no COLLATE) so collation is not part of this test.
🤖 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.
Nitpick comments:
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/tables/tests/pg/12_plus/create_table_with_serial_cols.sql`:
- Line 10: The payload column in create_table_with_serial_cols.sql currently
specifies COLLATE pg_catalog."default"; remove the COLLATE clause from the
payload text column so the DDL matches the MSQL fixture
(create_table_with_serial_cols_msql.sql) and keeps the test focused on SERIAL
round-trip behavior; update the column definition for payload in the CREATE
TABLE statement to simply be "payload text" (no COLLATE) so collation is not
part of this test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f0396229-3241-4fcd-ba66-2afa2c141989
📒 Files selected for processing (12)
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/__init__.pyweb/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/utils.pyweb/pgadmin/browser/server_groups/servers/databases/schemas/tables/tests/pg/12_plus/create_table_with_serial_cols.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/tables/tests/pg/12_plus/create_table_with_serial_cols_msql.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/tables/tests/pg/12_plus/test.jsonweb/pgadmin/browser/server_groups/servers/databases/schemas/tables/tests/pg/14_plus/create_table_with_serial_cols.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/tables/tests/pg/14_plus/create_table_with_serial_cols_msql.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/tables/tests/pg/14_plus/test.jsonweb/pgadmin/browser/server_groups/servers/databases/schemas/tables/tests/pg/16_plus/create_table_with_serial_cols.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/tables/tests/pg/16_plus/create_table_with_serial_cols_msql.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/tables/tests/pg/16_plus/test.jsonweb/pgadmin/browser/server_groups/servers/databases/schemas/tables/utils.py
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 #9896.
Schema Diff's Generate Script emitted CREATE TABLE DDL with the libpq-expanded form of SERIAL columns:
instead of preserving
SERIAL. Applied to a clean target, that script fails withrelation "table_col_seq" does not existbecause the sequence is never created — exactly what the reporter ran into.Root cause
SERIAL detection already existed in
columns/utils.py:get_formatted_columns(added in115208c8d, Nov 2023, for ERD generation), but was gated behind awith_serial_colsparameter that defaulted toFalse. Only ERD and the Schema Diff comparison phase opted in via explicitTrue. The Generate Script path and the browser tree's CREATE Script view went through_get_resql_for_table→_formatter(data)without specifying the flag, so SERIAL detection was skipped and #9896 reproduced.There's no legitimate use case where pgAdmin should knowingly emit the expanded libpq form:
SERIALis purely a CREATE-time macro that PostgreSQL expands into an integer column plus an implicit sequence owned by the column. The detection logic is reversing that expansion, which is the correct interpretation in every context (DDL emission, comparison, ERD, properties dialog).Fix
Flip the default to
Trueat all four declaration sites:columns/utils.py: get_formatted_columnsutils.py: BaseTableView._formatterutils.py: BaseTableView.fetch_tables__init__.py: TableView.fetch_tablesAll four existing explicit-
Truecallers (ERD × 2, Schema Diff compare × 2) are unaffected. Silent callers that previously got the buggyFalsenow correctly get SERIAL detection:_get_resql_for_table(utils.py:682)select_sql/insert_sql/update_sql(tables/init.py)has_default_val, computed fromatt.atthasdefin the column SQL template — not from thedefvalstring that SERIAL detection clears. SERIAL columns continue to be correctly excluded from INSERT/UPDATE samples.Regression test
Added a CREATE TABLE scenario covering
serial/bigserial/smallserialcolumns plus a plaintextcolumn topg/12_plus,pg/14_plus, andpg/16_plustest dirs, with the expected reverse-engineered DDL preserving the SERIAL keywords. Without this fix the test fails with thenextval('…_seq')form.Known coverage gaps (deliberately not in this PR)
WITH (OIDS = FALSE)in CREATE TABLE; without a live PG11 instance I can't confidently produce the expected SQL variant. Test scenario therefore not added topg/11_plusorpg/default.ppas/*test dirs) — source fix applies, regression not locked. PPAS DDL output can diverge from vanilla PG in subtle ways (oraplus, hyphenated identifiers). No PPAS instance available to verify.<table>_<col>_seqindefval). Sequence renames, table renames, column renames, and pathological substring collisions all break detection. Pre-existing from115208c8d; worth a follow-up that usespg_dependto derive the linkage directly rather than reconstructing the conventional name.Test plan
pycodestyle --config=.pycodestyleclean on the four modified Python files.python regression/runtests.py --pkg resql—Create Table with serial columns (MSQL)/(SQL)/Deleteall pass on PG18; full resql suite 0 failures.python regression/runtests.py --pkg tools.schema_diff— 2/2 pass.python regression/runtests.py --pkg browser.server_groups.servers.databases.schemas.tables— 464/464 pass.SERIALcolumn on source, empty target) through Schema Diff → Generate Script → confirm the emitted DDL saysSERIALand applies cleanly to the target.Summary by CodeRabbit
Release Notes
Bug Fixes
Tests