Repository navigation
fix(diff): real-world PostgreSQL constructs mined from pgsql-parser fixtures — 57 e2e cases, 10 engine fixes, wider catalog oracle - #1969
Conversation
… pgsql-parser fixtures Partitions, inheritance, exclusion/NO INHERIT/FK/unique constraints, index variants, generated columns, trigger variants, event triggers, policies, domains, range/enum/composite types, collations, extensions, operators, casts, aggregates, procedures, function variants, view options, quoted identifiers, storage/reloptions, sequences, identity and comments/grants. Each case deploys v1 with rows, deploys the emitted migration, must reach the catalog of a fresh v2 and must revert back to v1 on PostgreSQL 18. Several cases fail on the base; fixes follow.
…ds, storage/compression and trigger enable/disable in place Table-level settings (WITH (..) reloptions, UNLOGGED/LOGGED, INHERITS, PARTITION OF bounds) were not part of the compared table shape, so a change to any of them produced no migration. They are now folded into the shape from CREATE TABLE and the ALTER TABLE commands that change them, and diffed to ALTER TABLE SET/RESET, SET LOGGED/UNLOGGED, INHERIT/NO INHERIT and DETACH/ATTACH PARTITION with exact inverses. A changed PARTITION BY is a named incomplete finding. Column STORAGE and COMPRESSION changes were reported as inexpressible; they are now ALTER COLUMN SET STORAGE / SET COMPRESSION (inverse: the previous setting, or DEFAULT). ENABLE/DISABLE TRIGGER (and the ALWAYS/REPLICA/ALL/USER variants) had no derivable inverse; they are now self-inverting pairs, address the trigger by name, and order after the trigger's creation and before its drop.
…sts, statistics, rules, opclasses, publications, event triggers and foreign objects; order extension and collation steps; snapshot aggregates
… and pathed as name(argtypes)
… OWNED BY around its column
…ostgres refuses in place
…default; order NOT NULL around identity
…bounds, inheritance, storage/compression, owners, every COMMENT ON and non-table grants
… grants to their own objects instead of the preceding statement's unit
… ... TYPE; ADD/DROP ATTRIBUTE work in place
…rigger enable/disable, INCLUDE indexes, security_invoker views, column storage/comments and function overloads
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
|
Review complete. 🟠 2 high · 🟡 4 medium 💬 Inline comments (6)
This PR substantially expands the
Reviewed commit: 7edc73e |
This comment has been minimized.
This comment has been minimized.
![tenki-reviewer[bot]](https://avatars.githubusercontent.com/in/2900641?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR deepens the pgpm diff projection engine and its e2e coverage — expanding catalog snapshots, generalizing statement completion, and adding overload-aware function diffing — but introduces two high-severity migration-correctness defects: NULL-erasing composite-array bridges and invalid storage reverts, plus a broken policy test fixture.
Key findings
- 🟠 jsonb bridge turns NULL composite[] columns into empty arrays — semantic-diff-driver.ts:440
- 🟠 Storage inverse emits invalid
SET STORAGE default— semantic-diff-driver.ts:1084 - 🟡 Overload signature keeps typmods, splitting overloads — semantic-diff-driver.ts:256
- 🟡 Create
fx_rolefor the policy test case — diff-constructs-e2e.test.ts:222 - 🟡 Overload detection re-parses every function statement — semantic-diff-driver.ts:268
- 🟡 Operator identity ignores argument types, colliding overloads — facts.ts:85
| ? `ARRAY(SELECT jsonb_populate_record(NULL::${elementType}, e) FROM jsonb_array_elements(${c}) AS e)` | ||
| : `jsonb_populate_record(NULL::${elementType}, ${c})`; |
There was a problem hiding this comment.
🟠 bug · high
jsonb bridge turns NULL composite[] columns into empty arrays
The jsonb cast-back for array-of-composite columns uses ARRAY(SELECT jsonb_populate_record(NULL::type, e) FROM jsonb_array_elements(c) e) (pgpm/transform/src/semantic-diff-driver.ts:440-441). jsonb_array_elements is a strict set-returning function, so a SQL NULL column value produces zero rows and ARRAY(SELECT ...) evaluates to the empty array '{} — never NULL. The pre step correctly stores NULL via to_jsonb, so the forward migration silently rewrites every NULL composite-array value into '{}', and the revert does the same in reverse.
📋 Prompt for AI Agents
In pgpm/transform/src/semantic-diff-driver.ts around line 440, in columnRebridge, change the array cast-back expression ARRAY(SELECT jsonb_populate_record(NULL::${elementType}, e) FROM jsonb_array_elements(${c}) AS e) to CASE WHEN ${c} IS NULL THEN NULL::${typeName} ELSE ARRAY(SELECT jsonb_populate_record(NULL::${elementType}, e) FROM jsonb_array_elements(${c}) AS e) END so NULL column values survive both the forward migration and the revert (a NULL jsonb value feeds a strict SRF, producing zero rows and therefore an empty array).
| return (fn.parameters ?? []) | ||
| .map(p => p.FunctionParameter) | ||
| .filter(p => p && p.mode !== 'FUNC_PARAM_OUT' && p.mode !== 'FUNC_PARAM_TABLE') | ||
| .map(p => Deparser.deparse({ TypeName: canonicalizeTypes(cleanTree(p!.argType)) } as any, { pretty: false })) | ||
| .join(', '); |
There was a problem hiding this comment.
🟡 bug · medium
Overload signature keeps typmods, splitting overloads
functionArgSignature builds the overload key from a deparse of the canonicalized TypeName including typmods (pgpm/transform/src/semantic-diff-driver.ts:256-260), but PostgreSQL does not treat typmod as part of a function's identity. f(numeric(10,2)) and f(numeric) — the same overload — produce different signature strings, so overloadedFunctions unions them across both sides and misclassifies the function as overloaded; overloadIdentity then mints different names for each side. The diff emits a spurious DROP + CREATE of an unchanged function.
📋 Prompt for AI Agents
In pgpm/transform/src/semantic-diff-driver.ts, in functionArgSignature (lines 247-261), after canonicalizeTypes(cleanTree(p.argType)), delete the typmods field from the TypeName before Deparser.deparse, so numeric(10,2) and numeric yield the same overload signature string. Keep alias canonicalization as-is.
| v1: `${S} CREATE TABLE fx.t (id int PRIMARY KEY); ALTER TABLE fx.t ENABLE ROW LEVEL SECURITY; CREATE POLICY p ON fx.t FOR INSERT TO fx_role WITH CHECK (true);`, | ||
| v2: `${S} CREATE TABLE fx.t (id int PRIMARY KEY); ALTER TABLE fx.t ENABLE ROW LEVEL SECURITY; CREATE POLICY p ON fx.t FOR UPDATE TO fx_role USING (id > 0); CREATE POLICY q ON fx.t FOR DELETE USING (false);`, | ||
| revert: 'DROP OWNED BY fx_role; DROP ROLE IF EXISTS fx_role;' | ||
| }, |
There was a problem hiding this comment.
🟡 bug · medium
Create fx_role for the policy test case
The policy-command-change-and-second-policy case references fx_role in its v1 and v2 policies (pgpm/cli/tests/diff-constructs-e2e.test.ts:222-225) but declares no roles entry, and the harness creates roles only for cases that list them (line 488). PostgreSQL rejects CREATE POLICY ... TO fx_role for a nonexistent role, so the case fails during v1 deployment before the diff runs.
📋 Prompt for AI Agents
In pgpm/cli/tests/diff-constructs-e2e.test.ts around lines 220-225, add roles: ['fx_role'], to the policy-command-change-and-second-policy case, matching how grants-non-table-objects declares roles: ['fx_reader'] — the v1/v2 SQL uses TO fx_role and the role is never created before deployment.
| function overloadedFunctions(sides: string[]): Set<string> { | ||
| const signatures = new Map<string, Set<string>>(); | ||
| for (const sql of sides) { | ||
| const bytes = sqlSourceBytes(sql); | ||
| for (const f of classifyStatements(sql)) { | ||
| if (f.nodeTag !== 'CreateFunctionStmt' || !f.creates[0]) continue; | ||
| const sig = functionArgSignature(sliceStatementBytes(bytes, f.span.start, f.span.len)); | ||
| if (sig === null) continue; | ||
| const name = `${f.creates[0].schema ?? ''}.${f.creates[0].name}`; | ||
| if (!signatures.has(name)) signatures.set(name, new Set()); | ||
| signatures.get(name)!.add(sig); | ||
| } | ||
| } | ||
| return new Set([...signatures].filter(([, sigs]) => sigs.size > 1).map(([name]) => name)); | ||
| } | ||
|
|
||
| /** The identity an overloaded function's statement groups under: `name(argtypes)`. */ | ||
| function overloadIdentity(identity: ObjectIdentity, text: string, overloaded: Set<string>): ObjectIdentity { | ||
| if (identity.kind !== 'function' || !overloaded.has(`${identity.schema ?? ''}.${identity.name}`)) return identity; | ||
| const sig = functionArgSignature(text); | ||
| return sig === null ? identity : { ...identity, name: `${identity.name}(${sig})` }; | ||
| } |
There was a problem hiding this comment.
🟡 performance · medium
Overload detection re-parses every function statement
overloadedFunctions classifies both SQL sides and then calls functionArgSignature per CreateFunctionStmt, each of which performs a fresh parseSync of that statement's text (pgpm/transform/src/semantic-diff-driver.ts:247-282). readSide then classifies the same SQL a second time and overloadIdentity re-parses each function statement again, plus one more invocation from the changeName callback. Each function statement is parsed 2-3 extra times, roughly doubling diff latency on function-heavy schemas.
📋 Prompt for AI Agents
In pgpm/transform/src/semantic-diff-driver.ts around lines 268-289: refactor overloadedFunctions to also return a Map from schema.name + statement span (or statement text) to the computed signature, computed during its single pass over classifyStatements. Change readSide (line 291) and overloadIdentity (line 285) to accept that map and look up signatures instead of calling functionArgSignature(text) (a fresh parseSync) per function statement. Also have readSide reuse the classification produced by overloadedFunctions instead of calling classifyStatements(sql) a second time on the same SQL, and read from the map in the changeName callback near line 1984.
| case 'DefineStmt': { | ||
| const spec = DEFINE_KINDS[node.kind]; | ||
| const target = qualified(strings(node.defnames)); | ||
| if (!spec || !target) return null; | ||
| if (node.kind === 'OBJECT_OPERATOR') target.name = operatorName(target.name); | ||
| return { ...target, ...spec }; | ||
| } |
There was a problem hiding this comment.
🟡 bug · medium
Operator identity ignores argument types, colliding overloads
The DefineStmt completion names operators only by the sanitized symbol; the statement's argument types (objargs) are ignored (pgpm/transform/src/facts.ts:85-91). CREATE OPERATOR + (int, int) and CREATE OPERATOR + (text, text) in the same schema both complete to identity op_plus, so readSide appends the second operator's statements into the first's unit and the diff sees one operator where the schema has two, unable to drop or alter them independently.
📋 Prompt for AI Agents
In pgpm/transform/src/facts.ts, in the DefineStmt case of completionFor (lines 85-91), for node.kind === 'OBJECT_OPERATOR' read node.objargs (a list of TypeName nodes, with A_Star items for unary/any) and append sanitized type-name components to target.name (e.g. op_plus_int4_int4) so operators overloaded by argument type get distinct identities and unit keys.
| const setting = (def: unknown, field: 'storage_name' | 'compression'): string => | ||
| ((def as Record<string, string | undefined>)[field] ?? 'default').toLowerCase(); | ||
| for (const [field, subtype] of [['storage_name', 'AT_SetStorage'], ['compression', 'AT_SetCompression']] as const) { | ||
| const was = setting(prior.canon, field); | ||
| const now = setting(col.canon, field); | ||
| if (was === now) continue; | ||
| const set = (value: string): string => alter({ subtype, name, def: { String: { sval: value } } }); | ||
| steps.push(attr(name, 'alter', set(now), set(was))); |
There was a problem hiding this comment.
🟠 bug · high
Storage inverse emits invalid SET STORAGE default
The setting helper falls back to the literal 'default' when storage_name is absent (pgpm/transform/src/semantic-diff-driver.ts:1084-1085), and that value is emitted as an AT_SetStorage command (lines 1086-1091). PostgreSQL accepts only plain/external/extended/main for SET STORAGE, so the inverse of a column that gained an explicit storage mode (its prior storage came from the type default) produces invalid SQL and the revert fails.
📋 Prompt for AI Agents
In pgpm/transform/src/semantic-diff-driver.ts around lines 1082-1092, the setting helper returns the string 'default' when storage_name is absent, and that value is passed to set(...) for the AT_SetStorage subtype — PostgreSQL rejects SET STORAGE DEFAULT (valid values: plain, external, extended, main). Change the storage branch so a missing storage setting resolves to the column type's real default storage (e.g. from the captured source column's type), and keep the 'default' fallback only for AT_SetCompression, which accepts DEFAULT.
…ch e2e case so the diff suites coexist under one PG
…ange; varchar(10)/varchar are one overload; operators overload on operand types
…er (USING allows no subquery) keeping NULLs; drop typmods from overload signatures; identify operators and aggregates by operand types; cache function signatures
…-object deparser fixes (makage deps)
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Summary
Widens
pgpm diffcoverage with 57 v1→v2 cases derived from the pgsql-parser SQL fixtures (pgpm/cli/__tests__/diff-constructs-e2e.test.ts: deploy v1 + rows → diff → deploy migration → catalog == fresh v2 → revert → catalog == v1, on PG18). The failing cases are committed first (test(diff): e2e cases …, 21/57 failed onmain); every subsequent commit fixes one finding. All 57 pass now, plus the existing diff suites (diff-e2e,diff-followups-e2e,diff-generated-roundtrip, transform unit tests).Engine fixes (
pgpm/transform/src/semantic-diff-driver.ts,facts.ts,annotations.ts)WITH (reloptions),UNLOGGED,INHERITS,SET STORAGE/COMPRESSION,DISABLE TRIGGERwere never compared → "identical", nothing emitted (or "no derivable inverse")ALTER TABLE … SET (…)/RESET (…),SET LOGGED/UNLOGGED,INHERIT/NO INHERIT,DETACH/ATTACH PARTITION … FOR VALUES,ALTER COLUMN … SET STORAGE/COMPRESSION,ENABLE/DISABLE TRIGGER, each with an exact inverseCREATE EXTENSION / COLLATION / CAST / AGGREGATE / OPERATOR / OPERATOR CLASS / STATISTICS / RULE / PUBLICATION / EVENT TRIGGER / FOREIGN …had no identity → attached to the previous statement's unit, so an unchanged schema/type was dropped and recreated, or the atom was lost entirelyfx.fwere one unit (CREATE OR REPLACE, revert never dropped the new overload)name(argtypes)—functionArgSignature/overloadIdentityALTER SEQUENCE … AS/START/CYCLE/OWNED BYafterCREATE SEQUENCE→ "dropped and recreated"foldSequenceAlterfolds the ALTER into the definition;OWNED BYis ordered around its columnDROP TYPE … CREATE TYPEfails on dependentsjsonb/textaround the drop+create (bridgeColumns), reported as data loss (values the new type rejects are lost); compositeADD/DROP ATTRIBUTEstay in place, onlyALTER ATTRIBUTE … TYPEbridgesserial→ identity: spuriousTYPE int USING id::int,ADD GENERATEDfails on the existing default, revert emitsTYPE serial(invalid)int+ owned sequence +nextvaldefault (SERIAL_TYPES);NOT NULLis ordered beforeADD GENERATED/ afterDROP IDENTITYGRANT … ON FUNCTION/TYPE/DOMAIN,GRANT … ON ALL TABLES IN SCHEMA→ grouped with the preceding statement's unit (a sequence looked "dropped and recreated")annotationTargetresolvesObjectWithArgs/name lists;absorbAnnotationacceptsACL_TARGET_ALL_IN_SCHEMA(keyed bytargtype)Catalog oracle (
catalog-check.ts)snapshotCatalogpreviously threw on aggregates and did not see persistence, reloptions, partition bounds, inheritance, storage/compression, owners, comments on non-table objects or grants on non-table objects — so "passes" for those cases proved nothing. It now captures all of them (genericpg_description/pg_describe_object, owners for schema/relation/function/type, ACLs on schemas/sequences/functions/types/domains and default privileges).Seeded generator (
diff-generated-roundtrip.test.ts)New families:
UNLOGGED,fillfactor, identityBY DEFAULT,DISABLE TRIGGER,INCLUDEindexes,security_invokerviews, columnSET STORAGE, column comments, function overloads; 16 seeds.Deliberately incomplete (named errors, not silent output)
owner-changewith no prior owner declared →--allow-incomplete; revert leaves the new owner (revertResidualin the test).composite-mid(attribute inserted mid-record):ADD ATTRIBUTEappends; reported incomplete (order differs from target).--allow-data-loss --allow-incomplete(values cannot be removed in place).Review follow-ups (Tenki findings reproduced on PG18 and fixed, test first)
ALTER COLUMN … TYPE … USINGallows no subquery, soARRAY(SELECT jsonb_populate_record …)failed outright and would have turned NULL arrays into{}; the cast-back now goes through a statement-localpg_temp.pgpm_jsonb_array(anyelement, jsonb)helper (created and dropped in the same step) that keeps NULLs. The catalog oracle ignorespg_temp_Nnamespaces.f(varchar(10))isf(varchar)), both for identity and for the replace-vs-recreate decision; signatures are cached per statement text.op_hash__int4_int4,mysum__int4.SET STORAGE DEFAULTis valid on PG16+ (verified on PG18; the revert step of the storage case exercises it) — left as is.Depends on
comments-every-object-kindandgrants-non-table-objectsdeploy SQL that goes throughpgsql-deparser, which renderedGRANT USAGE ON TYPE fx, eandCOMMENT ON TRIGGER fx.t.t_tg. Fixed in constructive-io/pgsql-parser#375; those two cases pass locally against that build and will pass in CI oncepgsql-deparseris published and bumped here vianpx -y makage@latest deps ../pgsql-parser --install. No dependency ranges were hand-edited.pgpm-projections: regenerated
diff-migration,diff-granularity,diff-dependency-order,diff-module-vs-dbwith this build — committed outputs are byte-identical, no projections PR needed.Link to Devin session: https://app.devin.ai/sessions/5cfb0f837b8849969154e1b17a38803d
Open in Devin Desktop: https://app.devin.ai/desktop/session/5cfb0f837b8849969154e1b17a38803d?variant=devin
Requested by: @pyramation