Skip to content

Commit 4dd5cdd

Browse files
asheshvdpage
asheshv
authored and
dpage
committed
fix(schema-diff): reverse-engineer SERIAL columns by default (pgadmin-org#10020)
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 SELECT/INSERT/ UPDATE generators are unaffected regardless: they emit only column names, which SERIAL detection never touches (it rewrites the column type and clears `defval`, leaving the name intact). The table edit path is likewise safe: `get_sql` diffs each changed column against a freshly re-fetched old column state that bypasses SERIAL detection, so the reprojected `old_data` columns are never consulted for column DDL. 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
1 parent 833504f commit 4dd5cdd

13 files changed

Lines changed: 418 additions & 5 deletions

‎docs/en_US/release_notes_9_16.rst‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,3 +29,4 @@ Bug fixes
2929
*********
3030

3131
| `Issue #9892 <https://github.com/pgadmin-org/pgadmin4/issues/9892>`_ - Fix blank difference counts on the top-level group rows in Schema Diff.
32+
| `Issue #9896 <https://github.com/pgadmin-org/pgadmin4/issues/9896>`_ - Fix invalid DDL reconstruction for SERIAL columns in Schema Diff and the generated SQL/CREATE Script so the output round-trips on a clean target.

‎web/pgadmin/browser/server_groups/servers/databases/schemas/tables/__init__.py‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1706,7 +1706,7 @@ def get_drop_sql(self, sid, did, scid, tid):
17061706
return sql
17071707

17081708
@BaseTableView.check_precondition
1709-
def fetch_tables(self, sid, did, scid, tid=None, with_serial_cols=False):
1709+
def fetch_tables(self, sid, did, scid, tid=None, with_serial_cols=True):
17101710
"""
17111711
This function will fetch the list of all the tables
17121712
and will be used by schema diff.
@@ -1715,7 +1715,9 @@ def fetch_tables(self, sid, did, scid, tid=None, with_serial_cols=False):
17151715
:param did: Database Id
17161716
:param scid: Schema Id
17171717
:param tid: Table Id
1718-
:param with_serial_cols:
1718+
:param with_serial_cols: when True (default), reverse-engineer
1719+
SERIAL columns back to ``serial`` / ``smallserial`` /
1720+
``bigserial`` so emitted DDL is round-trippable. Issue #9896.
17191721
:return: Table dataset
17201722
"""
17211723

‎web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/utils.py‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -229,10 +229,20 @@ def parse_options_for_column(db_variables):
229229
@get_template_path
230230
def get_formatted_columns(conn, tid, data, other_columns,
231231
table_or_type, template_path=None,
232-
with_serial=False):
232+
with_serial=True):
233233
"""
234234
This function will iterate and return formatted data for all
235235
the columns.
236+
237+
Serial-column detection is on by default: a column whose default
238+
is ``nextval('<table>_<col>_seq'...)`` with an integer/smallint/
239+
bigint type is the reverse-engineered form of a SERIAL declaration,
240+
and we reproject it back to ``serial`` / ``smallserial`` /
241+
``bigserial`` so callers can emit valid round-trippable DDL. Pass
242+
``with_serial=False`` only if you genuinely need the raw libpq
243+
representation (e.g. a low-level introspection caller that handles
244+
the sequence itself). Issue #9896.
245+
236246
:param conn: Connection Object
237247
:param tid: Table ID
238248
:param data: Data

‎web/pgadmin/browser/server_groups/servers/databases/schemas/tables/tests/pg/12_plus/create_table_with_serial_cols.sql‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
-- Table: public.table_with_serial_cols
2+
3+
-- DROP TABLE IF EXISTS public.table_with_serial_cols;
4+
5+
CREATE TABLE IF NOT EXISTS public.table_with_serial_cols
6+
(
7+
id_serial serial NOT NULL,
8+
id_bigserial bigserial NOT NULL,
9+
id_smallserial smallserial NOT NULL,
10+
payload text COLLATE pg_catalog."default"
11+
)
12+
13+
TABLESPACE pg_default;
14+
15+
ALTER TABLE IF EXISTS public.table_with_serial_cols
16+
OWNER to <OWNER>;
17+
18+
COMMENT ON TABLE public.table_with_serial_cols
19+
IS 'round-trip SERIAL columns (issue #9896)';

‎web/pgadmin/browser/server_groups/servers/databases/schemas/tables/tests/pg/12_plus/create_table_with_serial_cols_msql.sql‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
CREATE TABLE public.table_with_serial_cols
2+
(
3+
id_serial serial NOT NULL,
4+
id_bigserial bigserial NOT NULL,
5+
id_smallserial smallserial NOT NULL,
6+
payload text
7+
);
8+
9+
ALTER TABLE IF EXISTS public.table_with_serial_cols
10+
OWNER to <OWNER>;
11+
12+
COMMENT ON TABLE public.table_with_serial_cols
13+
IS 'round-trip SERIAL columns (issue #9896)';

‎web/pgadmin/browser/server_groups/servers/databases/schemas/tables/tests/pg/12_plus/test.json‎

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2151,6 +2151,105 @@
21512151
"data": {
21522152
"name": "partition_table_with_collate_$%{}[]()&*^!@\"'`\\/#"
21532153
}
2154+
},
2155+
{
2156+
"type": "create",
2157+
"name": "Create Table with serial columns",
2158+
"endpoint": "NODE-table.obj",
2159+
"sql_endpoint": "NODE-table.sql_id",
2160+
"msql_endpoint": "NODE-table.msql",
2161+
"data": {
2162+
"name": "table_with_serial_cols",
2163+
"relowner": "<OWNER>",
2164+
"relacl": [],
2165+
"description": "round-trip SERIAL columns (issue #9896)",
2166+
"coll_inherits": "[]",
2167+
"hastoasttable": true,
2168+
"primary_key": [],
2169+
"partitions": [],
2170+
"partition_type": "range",
2171+
"is_partitioned": false,
2172+
"schema": "public",
2173+
"columns": [
2174+
{
2175+
"name": "id_serial",
2176+
"cltype": "serial",
2177+
"attacl": [],
2178+
"is_primary_key": false,
2179+
"attnotnull": true,
2180+
"attlen": null,
2181+
"attprecision": null,
2182+
"attidentity": "",
2183+
"colconstype": "n",
2184+
"attoptions": [],
2185+
"seclabels": []
2186+
},
2187+
{
2188+
"name": "id_bigserial",
2189+
"cltype": "bigserial",
2190+
"attacl": [],
2191+
"is_primary_key": false,
2192+
"attnotnull": true,
2193+
"attlen": null,
2194+
"attprecision": null,
2195+
"attidentity": "",
2196+
"colconstype": "n",
2197+
"attoptions": [],
2198+
"seclabels": []
2199+
},
2200+
{
2201+
"name": "id_smallserial",
2202+
"cltype": "smallserial",
2203+
"attacl": [],
2204+
"is_primary_key": false,
2205+
"attnotnull": true,
2206+
"attlen": null,
2207+
"attprecision": null,
2208+
"attidentity": "",
2209+
"colconstype": "n",
2210+
"attoptions": [],
2211+
"seclabels": []
2212+
},
2213+
{
2214+
"name": "payload",
2215+
"cltype": "text",
2216+
"attacl": [],
2217+
"is_primary_key": false,
2218+
"attnotnull": false,
2219+
"attlen": null,
2220+
"attprecision": null,
2221+
"attidentity": "a",
2222+
"colconstype": "n",
2223+
"attoptions": [],
2224+
"seclabels": []
2225+
}
2226+
],
2227+
"foreign_key": [],
2228+
"check_constraint": [],
2229+
"unique_constraint": [],
2230+
"exclude_constraint": [],
2231+
"partition_keys": [],
2232+
"seclabels": [],
2233+
"forcerlspolicy": false,
2234+
"like_default_value": false,
2235+
"like_constraints": false,
2236+
"like_indexes": false,
2237+
"like_storage": false,
2238+
"like_comments": false,
2239+
"like_identity": false,
2240+
"like_statistics": false
2241+
},
2242+
"store_object_id": true,
2243+
"expected_sql_file": "create_table_with_serial_cols.sql",
2244+
"expected_msql_file": "create_table_with_serial_cols_msql.sql"
2245+
},
2246+
{
2247+
"type": "delete",
2248+
"name": "Delete Table with serial columns",
2249+
"endpoint": "NODE-table.obj_id",
2250+
"data": {
2251+
"name": "table_with_serial_cols"
2252+
}
21542253
}
21552254
]
21562255
}

‎web/pgadmin/browser/server_groups/servers/databases/schemas/tables/tests/pg/14_plus/create_table_with_serial_cols.sql‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
-- Table: public.table_with_serial_cols
2+
3+
-- DROP TABLE IF EXISTS public.table_with_serial_cols;
4+
5+
CREATE TABLE IF NOT EXISTS public.table_with_serial_cols
6+
(
7+
id_serial serial NOT NULL,
8+
id_bigserial bigserial NOT NULL,
9+
id_smallserial smallserial NOT NULL,
10+
payload text COLLATE pg_catalog."default"
11+
)
12+
13+
TABLESPACE pg_default;
14+
15+
ALTER TABLE IF EXISTS public.table_with_serial_cols
16+
OWNER to <OWNER>;
17+
18+
COMMENT ON TABLE public.table_with_serial_cols
19+
IS 'round-trip SERIAL columns (issue #9896)';

‎web/pgadmin/browser/server_groups/servers/databases/schemas/tables/tests/pg/14_plus/create_table_with_serial_cols_msql.sql‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
CREATE TABLE public.table_with_serial_cols
2+
(
3+
id_serial serial NOT NULL,
4+
id_bigserial bigserial NOT NULL,
5+
id_smallserial smallserial NOT NULL,
6+
payload text
7+
);
8+
9+
ALTER TABLE IF EXISTS public.table_with_serial_cols
10+
OWNER to <OWNER>;
11+
12+
COMMENT ON TABLE public.table_with_serial_cols
13+
IS 'round-trip SERIAL columns (issue #9896)';

‎web/pgadmin/browser/server_groups/servers/databases/schemas/tables/tests/pg/14_plus/test.json‎

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2283,6 +2283,105 @@
22832283
"data": {
22842284
"name": "simple_table_comp_$%{}[]()&*^!@\"'`\\/#"
22852285
}
2286+
},
2287+
{
2288+
"type": "create",
2289+
"name": "Create Table with serial columns",
2290+
"endpoint": "NODE-table.obj",
2291+
"sql_endpoint": "NODE-table.sql_id",
2292+
"msql_endpoint": "NODE-table.msql",
2293+
"data": {
2294+
"name": "table_with_serial_cols",
2295+
"relowner": "<OWNER>",
2296+
"relacl": [],
2297+
"description": "round-trip SERIAL columns (issue #9896)",
2298+
"coll_inherits": "[]",
2299+
"hastoasttable": true,
2300+
"primary_key": [],
2301+
"partitions": [],
2302+
"partition_type": "range",
2303+
"is_partitioned": false,
2304+
"schema": "public",
2305+
"columns": [
2306+
{
2307+
"name": "id_serial",
2308+
"cltype": "serial",
2309+
"attacl": [],
2310+
"is_primary_key": false,
2311+
"attnotnull": true,
2312+
"attlen": null,
2313+
"attprecision": null,
2314+
"attidentity": "",
2315+
"colconstype": "n",
2316+
"attoptions": [],
2317+
"seclabels": []
2318+
},
2319+
{
2320+
"name": "id_bigserial",
2321+
"cltype": "bigserial",
2322+
"attacl": [],
2323+
"is_primary_key": false,
2324+
"attnotnull": true,
2325+
"attlen": null,
2326+
"attprecision": null,
2327+
"attidentity": "",
2328+
"colconstype": "n",
2329+
"attoptions": [],
2330+
"seclabels": []
2331+
},
2332+
{
2333+
"name": "id_smallserial",
2334+
"cltype": "smallserial",
2335+
"attacl": [],
2336+
"is_primary_key": false,
2337+
"attnotnull": true,
2338+
"attlen": null,
2339+
"attprecision": null,
2340+
"attidentity": "",
2341+
"colconstype": "n",
2342+
"attoptions": [],
2343+
"seclabels": []
2344+
},
2345+
{
2346+
"name": "payload",
2347+
"cltype": "text",
2348+
"attacl": [],
2349+
"is_primary_key": false,
2350+
"attnotnull": false,
2351+
"attlen": null,
2352+
"attprecision": null,
2353+
"attidentity": "a",
2354+
"colconstype": "n",
2355+
"attoptions": [],
2356+
"seclabels": []
2357+
}
2358+
],
2359+
"foreign_key": [],
2360+
"check_constraint": [],
2361+
"unique_constraint": [],
2362+
"exclude_constraint": [],
2363+
"partition_keys": [],
2364+
"seclabels": [],
2365+
"forcerlspolicy": false,
2366+
"like_default_value": false,
2367+
"like_constraints": false,
2368+
"like_indexes": false,
2369+
"like_storage": false,
2370+
"like_comments": false,
2371+
"like_identity": false,
2372+
"like_statistics": false
2373+
},
2374+
"store_object_id": true,
2375+
"expected_sql_file": "create_table_with_serial_cols.sql",
2376+
"expected_msql_file": "create_table_with_serial_cols_msql.sql"
2377+
},
2378+
{
2379+
"type": "delete",
2380+
"name": "Delete Table with serial columns",
2381+
"endpoint": "NODE-table.obj_id",
2382+
"data": {
2383+
"name": "table_with_serial_cols"
2384+
}
22862385
}
22872386
]
22882387
}

‎web/pgadmin/browser/server_groups/servers/databases/schemas/tables/tests/pg/16_plus/create_table_with_serial_cols.sql‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
-- Table: public.table_with_serial_cols
2+
3+
-- DROP TABLE IF EXISTS public.table_with_serial_cols;
4+
5+
CREATE TABLE IF NOT EXISTS public.table_with_serial_cols
6+
(
7+
id_serial serial NOT NULL,
8+
id_bigserial bigserial NOT NULL,
9+
id_smallserial smallserial NOT NULL,
10+
payload text COLLATE pg_catalog."default"
11+
)
12+
13+
TABLESPACE pg_default;
14+
15+
ALTER TABLE IF EXISTS public.table_with_serial_cols
16+
OWNER to <OWNER>;
17+
18+
COMMENT ON TABLE public.table_with_serial_cols
19+
IS 'round-trip SERIAL columns (issue #9896)';

0 commit comments

Comments
 (0)