Repository navigation
fix(diff): Tenki follow-ups — enum ADD VALUE isolation/neighbors, composite mid insert, rename on renamed table + references, sequence START, --using, -token privileges - #1968
Conversation
…/same-tx use, composite mid insert, sequence MINVALUE reset, --using comma, -token privileges, rename on renamed table, rename references)
… mid-list insert, sequence START after MINVALUE/MAXVALUE drop, --using single expression, -token privilege state, column rename on a renamed table, rename references and implicit constraint names - alterEnum: BEFORE/AFTER only a value that already exists; a value added and used by another step of the same migration is incomplete (Postgres 55P04) - alterComposite: a mid-list ADD ATTRIBUTE is incomplete (it appends) - alterSequence: resets the START Postgres derives from a dropped MINVALUE/MAXVALUE - parseExpression: --using must be exactly one expression (no top-level comma) - diffPrivileges: -token state revokes once; dropping it re-grants only PUBLIC defaults - --rename app.old.col=app.new.name declares a column rename on a renamed table - table rename rewrites qualified refs in views/policies/comments/grants/regclass, renames Postgres-derived constraint names (old_pkey -> new_pkey), and folds a replaced function's inverse into the rename's inverse so revert works - CLIDeployTestFixture: repeated flags accumulate like minimist
…d of a third database per case (connection pressure in the parallel cli lane)
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
|
Review complete. 🟠 1 high · 🟡 3 medium 💬 Inline comments (4)
🧹 Nitpicks (2) — 🟢 2 low
This PR extends the pgpm transform engine so that a schema diff between two declared states can emit ALTER steps for renames and privilege revocations instead of treating every difference as drop-and-recreate. The driver now detects table/column renames from annotations, emits RENAME steps with folded inverse actions, folds same-migration enum-use hazards into advisory comments, and re-grants PUBLIC default privileges when an explicit revoke token disappears.
Reviewed commit: 9143e53 |
![tenki-reviewer[bot]](https://avatars.githubusercontent.com/in/2900641?s=60&v=4){kind=small}
There was a problem hiding this comment.
Adds rename-aware ALTER generation and GRANT/REVOKE re-granting to the pgpm semantic-diff pipeline, with hardening tests and an e2e CLI fixture, but the new rename inverse-folding can drop dependent views on revert.
Key findings
- 🟠 Rename inverse folding drops rebuilt dependents on revert — semantic-diff-driver.ts:1705
- 🟡 PUBLIC default privileges omit schema/database entries — annotations.ts:185
- 🟡 ColumnRef rename rewrites table aliases named like the old table — semantic-diff-driver.ts:1115
- 🟡 Enum add-before-use gets a comment but no ordering edge — semantic-diff-driver.ts:1675
| const PUBLIC_DEFAULT_PRIVILEGES: Record<string, string[]> = { | ||
| OBJECT_FUNCTION: ['execute'], | ||
| OBJECT_PROCEDURE: ['execute'], | ||
| OBJECT_ROUTINE: ['execute'], | ||
| OBJECT_TYPE: ['usage'], | ||
| OBJECT_DOMAIN: ['usage'], | ||
| OBJECT_LANGUAGE: ['usage'] | ||
| }; |
There was a problem hiding this comment.
🟡 bug · medium
PUBLIC default privileges omit schema/database entries
PUBLIC_DEFAULT_PRIVILEGES (pgpm/transform/src/annotations.ts:185-192) lists only functions, procedures, routines, types, domains, and languages, but Postgres also grants PUBLIC default privileges on schemas (USAGE) and databases (CONNECT, TEMP). When a diff drops an explicit REVOKE ALL ON SCHEMA ... FROM PUBLIC (a -all token in the from-state), unrevoked at line 221 evaluates to empty because byDefault is empty, so no re-grant is emitted. The migration cannot reach the target state and deploy verification reports a residual privilege difference.
📋 Prompt for AI Agents
In pgpm/transform/src/annotations.ts lines 185-192, extend PUBLIC_DEFAULT_PRIVILEGES so dropping an explicit REVOKE ... FROM PUBLIC re-grants everything PUBLIC holds by default: add OBJECT_SCHEMA: ['usage'] and OBJECT_DATABASE: ['connect', 'temporary'] (plus foreign-data-wrapper/server USAGE if those object types are handled). Currently only functions/procedures/routines/types/domains/languages are listed, so a diff that removes REVOKE ALL ON SCHEMA ... FROM PUBLIC emits no GRANT and the migration cannot reach the target state. When adding these, also make the all re-grant emit the individual byDefault privileges (see the GRANT ALL finding) so GRANT ALL ON SCHEMA does not hand PUBLIC CREATE.
| if (key === 'ColumnRef') { | ||
| const cr = value as { fields?: unknown[] }; | ||
| if (cr.fields && cr.fields.length >= 2) { | ||
| out[key] = { ...cr, fields: renameNames(cr.fields, 1) }; | ||
| continue; | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 bug · medium
ColumnRef rename rewrites table aliases named like the old table
The ColumnRef branch of renameRelation rewrites any 2-field reference whose first field equals the old table name, with no check that it is actually a relation reference (pgpm/transform/src/semantic-diff-driver.ts:1115-1120). A view or policy whose SQL aliases another table with the old table's name — SELECT old.id FROM app.other AS old — is rewritten to reference new.id on a relation that was not renamed, producing a broken CREATE OR REPLACE VIEW (or a spurious replacement of a view that did not need one). Quoted identifiers ("Old") are also missed by the same branch.
📋 Prompt for AI Agents
In pgpm/transform/src/semantic-diff-driver.ts around lines 1098-1121, tighten the ColumnRef rewriting in renameRelation: before rewriting a 2-field reference old.col, verify that old is actually the renamed relation — collect the relname and alias of every RangeVar/DeleteStmt/UpdateStmt/InsertStmt relation encountered in the same statement and only rewrite when the head field matches the relation name or an explicit alias OF that relation. Table aliases of other tables (e.g. FROM app.other AS old) must be left untouched, since Postgres stores view/policy definitions by OID and never rewrites them on rename.
| for (const renameAtom of atoms) { | ||
| if (!renameAtom.rename || renameAtom.sub?.kind !== 'table') continue; | ||
| for (const atom of atoms) { | ||
| if (atom.phase !== 'create' || atom.added || !atom.inverse || !atom.deps.has(renameAtom.id)) continue; | ||
| renameAtom.inverse = `${renameAtom.inverse}\n${atom.inverse}`; | ||
| atom.inverse = ''; | ||
| } | ||
| } |
There was a problem hiding this comment.
🟠 bug · high
Rename inverse folding drops rebuilt dependents on revert
The new inverse-folding loop moves the inverse of every create-phase atom that depends on a table rename into the rename atom's inverse (pgpm/transform/src/semantic-diff-driver.ts:1705-1711), but it does not distinguish replace-in-place atoms (whose inverse restores the old definition) from drop-and-recreate atoms (whose inverse is only a DROP). A view rebuilt around a breaking table change under a declared rename has its DROP VIEW inverse folded into the rename inverse; on revert the rebuilt view is recreated first (via its drop atom's inverse) and the rename change's revert then runs RENAME ... back followed by the folded DROP VIEW, permanently dropping the object the revert was supposed to restore. Reverting such a migration leaves the catalog missing the dependent object.
📋 Prompt for AI Agents
In pgpm/transform/src/semantic-diff-driver.ts around lines 1705-1712, tighten the inverse-folding loop so it only folds atoms whose inverse restores the object's old definition (replace-in-place cases, e.g. CREATE OR REPLACE FUNCTION inverses). Detect this by checking that the atom's inverse re-creates the same object identity (or that the atom has no sibling drop atom in the same unit) instead of accepting any create-phase atom with a non-empty inverse that deps the rename atom. Why: a rebuilt view/policy/trigger under a declared table rename currently gets its DROP inverse folded into the rename inverse; on revert the object is recreated by its drop atom first and then dropped by the folded statement, so the reverted database loses the object.
| for (const atom of atoms) { | ||
| const added = /^ALTER TYPE (\S+) ADD VALUE ('(?:[^']|'')*')/.exec(atom.sql); | ||
| if (!added) continue; | ||
| const users = atoms.filter(x => x !== atom && x.phase !== 'drop' && !/^ALTER TYPE \S+ ADD VALUE /.test(x.sql) && x.sql.includes(added[2])); | ||
| if (users.length === 0) continue; | ||
| const note = `enum value ${added[2]} of ${added[1]} is added and used in the same migration; Postgres cannot use a new enum value before the transaction that added it commits (deploy with --no-tx, or split the migration)`; | ||
| incomplete.push(`${atom.name}: ${note}`); | ||
| for (const user of users) user.sql = `-- pgpm: ${note}\n${user.sql}`; | ||
| } |
There was a problem hiding this comment.
🟡 bug · medium
Enum add-before-use gets a comment but no ordering edge
The 55P04 detection loop only prepends an advisory -- pgpm: comment to consuming atoms and pushes an incomplete warning; it never adds a dependency edge from the consuming atom to the ALTER TYPE .. ADD VALUE atom (pgpm/transform/src/semantic-diff-driver.ts:1675-1683). Ordering between same-phase (alter) atoms of different units falls back to rank alone, so a table's SET DEFAULT 'c' or constraint step can still be emitted before the ADD VALUE it consumes. Even when the operator obeys the note and deploys with --no-tx, the migration can then fail with invalid input value for enum because the value does not exist yet when the consuming statement runs.
📋 Prompt for AI Agents
In pgpm/transform/src/semantic-diff-driver.ts around lines 1675-1683, after computing users for a detected ALTER TYPE .. ADD VALUE atom, add a dependency edge from each user atom to the ADD VALUE atom (for (const user of users) user.deps.add(atom.id);) alongside the comment annotation. Why: without the edge the topological sort orders same-phase atoms by rank only, so a consuming ALTER TABLE step can be emitted before the ADD VALUE; with --no-tx the deploy then fails with invalid input value for enum even though the tool detected the hazard.
… renamed table reverts out of order with the rewritten text, alias named like the renamed table is rewritten, enum consumer has no edge to its ADD VALUE)
… and from its text as written; rewrite old.col only where old names the table, not an alias; a consumer of a new enum value depends on its ADD VALUE
…ke (inverse of an added REVOKE grants ALL back; database CONNECT/TEMPORARY not restored; GRANT ALL where PUBLIC only held EXECUTE)
…efault privileges in both directions (EXECUTE, USAGE, CONNECT/TEMPORARY on databases; nothing on schemas/tables), never GRANT ALL
… owner's default) in the added-revoke e2e case
Summary
Follow-up to #1967: fixes the five Tenki findings and the three items that were left unverified there. Failing tests first (f8025c9), then the fix (abb4284).
What was broken → what works now
55P04 unsafe use of new value(Postgres cannot use a value before the adding transaction commits;pgpm deployruns in one transaction)--no-tx, or split the migration);--allow-incompleteemits it with a-- pgpm:marker on the consumerADD VALUE 'x' BEFORE 'y'withynot yet addedBEFORE/AFTERonly a value that already existsADD ATTRIBUTEappends →(a,b,c)instead of(a,c,b), silently--allow-incomplete) — Postgres can only append--rename app.old.col=app.new.name(column on a table renamed in the same diff)renameRelationrewrote onlyRangeVar/pktable/typevarColumnRefs,COMMENT/SECURITY LABELtargets, grants, policies,regclassliterals; renames constraints Postgres named after the old table (old_pkey→new_pkey, with their indexes); a changed SQL-function body (text Postgres does not rewrite) is replaced forward and its inverse folded into the rename's inverse so revert worksNO MINVALUE/NO MAXVALUEMINVALUE 5leftSTART WITH 5(catalog mismatch vs fresh v2)ALTER SEQUENCE .. STARTreset to what Postgres derives--using "a, b"--usingdiffPrivileges-tokenstateREVOKE ALL .. FROM PUBLICon a table emittedGRANT ALL ON TABLE .. TO PUBLIC; a revoke of a held privilege could emit twiceReview round (failing tests 2a0a8cb → fix 23da4cb):
ALTER COLUMN .. TYPEwith the view still in place (its drop had been folded into the rename's inverse) and recreated the view from the rename-rewritten text (FROM fu.new) after the table was namedoldagainObjectUnit.inverseTexts)SELECT old.id FROM app.other AS oldwhenoldis the renamed tableold.idrewritten tonew.id→ spurious view rebuild whose revert fails (missing FROM-clause entry)old.colis rewritten only in statements that read relationolditself and never useoldas an aliasADD VALUEADD VALUEchange (and the diagnostic is whitespace-safe)REVOKE ALL ON SCHEMA/TABLE .. FROM PUBLICwasGRANT ALL .. TO PUBLIC(privileges nobody held, granted on revert); a droppedREVOKE .. ON DATABASErestored nothing; a dropped function revoke emittedGRANT ALLChecked against the PG18 GRANT docs for that review's finding 4: PUBLIC has no default privilege on schemas (only the
publicschema's USAGE, which no module declares), soPUBLIC_DEFAULT_PRIVILEGESdeliberately has no schema entry; the database entry was missing and is added.Also:
CLIDeployTestFixturenow accumulates repeated flags (--rename a=b --rename c=d) like minimist does; it used to keep only the last.Decisions made
pgpm deploywraps the whole run in one transaction, so splitting changes would not help; the message says to deploy with--no-txor split the migration.<table>_<col>_not_null; a table rename leavesold_id_not_null(pg_dump-visible, cosmetic). Not renamed: PG<18 has no such constraint, andpgpm diffhas no server to ask.Verification
pgpm/transform: 173 tests (13 new insemantic-diff-hardening.test.ts).pgpm/cli/__tests__/diff-followups-e2e.test.ts(new, live PG18): each case deploys v1, deploys the diff, compares the catalog with a fresh v2, reverts, compares with v1 — enum head insert, composite mid insert, sequence MINVALUE/MAXVALUE drop, REVOKE FROM PUBLIC drop, table+column rename with rows, rename references (comment/grant/policy/view/function), view rebuilt around a renamed+retyped table, alias named like the renamed table, enum add+use, added REVOKE FROM PUBLIC reverting to the effective privileges PUBLIC held.Link to Devin session: https://app.devin.ai/sessions/88795a13d3834106a2ac3a6782348b40
Open in Devin Desktop: https://app.devin.ai/desktop/session/88795a13d3834106a2ac3a6782348b40?variant=devin
Requested by: @pyramation