fix(diff): Tenki follow-ups — enum ADD VALUE isolation/neighbors, composite mid insert, rename on renamed table + references, sequence START, --using, -token privileges

.agents/skills/pgpm-projections/SKILL.md

@@ -117,30 +117,52 @@ pgpm diff <A> <B> --verify
   way to keep dependents intact.
 - Every emitted change has deploy, revert, and verify. Steps pgpm cannot
   derive (a DROP it does not know, an inverse that does not exist such as
-  `ALTER TYPE .. ADD VALUE`) are a hard error naming the object; only
-  `--allow-incomplete` emits them, with a `-- pgpm:` marker and a warning.
+  `ALTER TYPE .. ADD VALUE`, an enum value added and used by another step of
+  the same migration — Postgres cannot use it before the adding transaction
+  commits, so deploy with `--no-tx` or split the migration — a composite
+  attribute inserted mid-list, which `ADD ATTRIBUTE` can only append) are a
+  hard error naming the object; only `--allow-incomplete` emits them, with a
+  `-- pgpm:` marker and a warning.
 - Steps that can lose data (DROP TABLE/COLUMN/SCHEMA/SEQUENCE, any column type
   change, a type/domain/sequence that must be dropped and recreated) are a
   hard error listing each step; only `--allow-data-loss` emits them. Renames
   are never inferred from a drop/add pair: `--rename schema.table[.column]=…`
   emits `ALTER .. RENAME` with its inverse (and rebuilds dependents that name
-  the column around it); an undeclared rename is a refused column drop.
+  the column around it); an undeclared rename is a refused column drop. A
+  table rename also renames the constraints (and their indexes) Postgres named
+  after the old table (`old_pkey` → `new_pkey`), rewrites qualified references
+  in views, policies, comments, grants and `regclass` literals, and keeps a
+  changed function body (text Postgres does not rewrite) restorable by folding
+  its inverse into the rename's. A column of a table renamed in the same diff
+  is declared with either name: `--rename app.old=app.new --rename
+  app.old.col=app.new.name`. `--rename` never moves a column to another table
+  or schema.
 - Column attribute changes emit exact ALTERs with exact inverses: `SET/DROP
   DEFAULT`, `SET/DROP NOT NULL`, `ADD/DROP IDENTITY`, `ALTER COLUMN TYPE ..
   USING col::type` (or the `--using` expression; the inverse always casts back
-  with `col::oldtype`), generated-expression changes as DROP + ADD COLUMN
+  with `col::oldtype`; a `--using` value must be one expression, no top-level
+  comma), generated-expression changes as DROP + ADD COLUMN
   (derived data). Column-level `DEFERRABLE` / `INITIALLY DEFERRED` fold into
   their constraint.
 - Objects with dependents alter in place instead of DROP + CREATE: enum values
-  append (`ALTER TYPE .. ADD VALUE`), domains (`ALTER DOMAIN` constraints /
-  default / NOT NULL), sequences (`ALTER SEQUENCE`), composite types (`ADD /
-  DROP / ALTER ATTRIBUTE`). A function whose signature uses a dropped or
+  append (`ALTER TYPE .. ADD VALUE`, `BEFORE`/`AFTER` only a value that already
+  exists), domains (`ALTER DOMAIN` constraints / default / NOT NULL), sequences
+  (`ALTER SEQUENCE`, including the `START` Postgres derives from a dropped
+  `MINVALUE`/`MAXVALUE`), composite types (`ADD / DROP / ALTER ATTRIBUTE`). A function whose signature uses a dropped or
   altered type is dropped and recreated around it (its body is validated
   against the type's current shape in both directions).
 - COMMENT ON, GRANT/REVOKE, OWNER TO and ALTER DEFAULT PRIVILEGES are keyed
   state on their object: the diff emits the exact `COMMENT` / `GRANT` /
   `REVOKE` / `ALTER DEFAULT PRIVILEGES` with the previous state as inverse,
-  never a rebuild of the object.
+  never a rebuild of the object. An explicit `REVOKE .. FROM PUBLIC` is state
+  too: dropping it re-grants only what PUBLIC holds by default (EXECUTE on
+  functions, USAGE on types, CONNECT/TEMPORARY on databases; nothing on
+  tables, sequences or schemas), never `GRANT ALL`, so a table's or schema's
+  dropped revoke emits no GRANT at all; the inverse of an added revoke
+  likewise restores only that default (a comment when there is none). A table rename rewrites
+  `old.col` references only where `old` names the table itself, never another
+  relation's alias; a view/policy rebuilt around a renamed table is dropped
+  under the old name and recreated on revert from its text as written.
 - A module compares equal to the live database it deployed: type spellings
   (`integer`/`int4`, `timestamp with time zone`/`timestamptz`, …), catalog
   default casts (`'a'::app.status`, `nextval('s'::regclass)`, `'x'::text`),

pgpm/cli/__tests__/diff-followups-e2e.test.ts

@@ -0,0 +1,274 @@
+/**
+ * e2e for the `pgpm diff` review follow-ups against live Postgres: enum values
+ * inserted at the head of the list, a composite attribute inserted mid-list,
+ * a removed MINVALUE (implied START), a removed `REVOKE ALL FROM PUBLIC`, a
+ * column rename on a table renamed in the same diff, and references to a
+ * renamed table from comments, views, policies and functions. Each migration
+ * deploys on v1 to the catalog of v2 and reverts back to v1.
+ *
+ * PREREQUISITES: a running PostgreSQL instance via standard PG* env vars.
+ */
+import { diffCatalogSnapshots, loadModule, snapshotCatalog, withoutColumnOrder } from '@pgpmjs/transform';
+import * as fs from 'fs';
+import * as path from 'path';
+import { teardownPgPools } from 'pg-cache';
+
+import { CLIDeployTestFixture } from '../test-utils';
+
+jest.setTimeout(240000);
+
+afterAll(async () => {
+  await teardownPgPools();
+});
+
+const WS = 'fu-ws';
+
+const writeModule = (moduleDir: string, name: string, sql: string) => {
+  fs.mkdirSync(path.join(moduleDir, 'deploy'), { recursive: true });
+  fs.mkdirSync(path.join(moduleDir, 'revert'), { recursive: true });
+  fs.mkdirSync(path.join(moduleDir, 'verify'), { recursive: true });
+  fs.writeFileSync(
+    path.join(moduleDir, 'pgpm.plan'),
+    `%syntax-version=1.0.0\n%project=${name}\n%uri=https://github.com/test/${name}\n\ninit 2024-01-01T00:00:00Z test <test@example.com> # add init\n`
+  );
+  fs.writeFileSync(
+    path.join(moduleDir, `${name}.control`),
+    `comment = 'Diff follow-ups e2e module'\ndefault_version = '0.0.1'\nrelocatable = false\nsuperuser = false\n`
+  );
+  fs.writeFileSync(path.join(moduleDir, 'deploy', 'init.sql'), `-- Deploy init to pg\n\nBEGIN;\n\n${sql}\n\nCOMMIT;\n`);
+  fs.writeFileSync(path.join(moduleDir, 'revert', 'init.sql'), `-- Revert init from pg\n\nBEGIN;\n\nDROP SCHEMA fu CASCADE;\n\nCOMMIT;\n`);
+  fs.writeFileSync(path.join(moduleDir, 'verify', 'init.sql'), `-- Verify init on pg\n\nBEGIN;\n\nROLLBACK;\n`);
+};
+
+interface Case {
+  name: string;
+  v1: string;
+  v2: string;
+  flags: string;
+  rows?: string;
+  deployed: RegExp[];
+  notDeployed?: RegExp[];
+  /** Catalog differences that remain after the revert (Postgres cannot remove enum values). */
+  revertResidual?: RegExp[];
+  /** Catalog differences that remain after the forward migration (documented, reported as incomplete). */
+  forwardResidual?: RegExp[];
+}
+
+const CASES: Case[] = [
+  {
+    name: 'enum-head',
+    v1: `CREATE SCHEMA fu; CREATE TYPE fu.status AS ENUM ('a'); CREATE TABLE fu.t (id int PRIMARY KEY, s fu.status);`,
+    v2: `CREATE SCHEMA fu; CREATE TYPE fu.status AS ENUM ('x', 'y', 'a'); CREATE TABLE fu.t (id int PRIMARY KEY, s fu.status);`,
+    flags: '--allow-incomplete',
+    deployed: [/ADD VALUE 'x' BEFORE 'a'/, /ADD VALUE 'y' AFTER 'x'/],
+    revertResidual: [/types: fu\.status differs/]
+  },
+  {
+    name: 'composite-mid',
+    v1: `CREATE SCHEMA fu; CREATE TYPE fu.pt AS (a int, b int); CREATE TABLE fu.t (id int PRIMARY KEY, p fu.pt);`,
+    v2: `CREATE SCHEMA fu; CREATE TYPE fu.pt AS (a int, c int, b int); CREATE TABLE fu.t (id int PRIMARY KEY, p fu.pt);`,
+    flags: '--allow-incomplete',
+    deployed: [/ALTER TYPE fu\.pt ADD ATTRIBUTE c/],
+    // ADD ATTRIBUTE appends: the attribute order differs from v2 and pgpm says so (incomplete).
+    forwardResidual: [/types: fu\.pt differs/]
+  },
+  {
+    name: 'sequence-minvalue',
+    v1: `CREATE SCHEMA fu; CREATE SEQUENCE fu.s MINVALUE 5 MAXVALUE 1000 CACHE 10;`,
+    v2: `CREATE SCHEMA fu; CREATE SEQUENCE fu.s;`,
+    flags: '',
+    deployed: [/NO MINVALUE/, /START (WITH )?1\b/]
+  },
+  {
+    name: 'revoke-public',
+    v1: [
+      'CREATE SCHEMA fu; CREATE TABLE fu.t (id int PRIMARY KEY); REVOKE ALL ON TABLE fu.t FROM PUBLIC;',
+      'CREATE FUNCTION fu.f() RETURNS int LANGUAGE sql AS $$ SELECT 1 $$; REVOKE ALL ON FUNCTION fu.f() FROM PUBLIC;'
+    ].join('\n'),
+    v2: `CREATE SCHEMA fu; CREATE TABLE fu.t (id int PRIMARY KEY); CREATE FUNCTION fu.f() RETURNS int LANGUAGE sql AS $$ SELECT 1 $$;`,
+    flags: '',
+    deployed: [/GRANT EXECUTE ON FUNCTION fu\.f\(\) TO PUBLIC/],
+    notDeployed: [/GRANT ALL/]
+  },
+  {
+    name: 'rename-table-and-column',
+    v1: `CREATE SCHEMA fu; CREATE TABLE fu.old (id int PRIMARY KEY, col text);`,
+    v2: `CREATE SCHEMA fu; CREATE TABLE fu.new (id int PRIMARY KEY, name text);`,
+    flags: '--rename fu.old=fu.new --rename fu.old.col=fu.new.name',
+    rows: `INSERT INTO fu.old VALUES (1, 'kept')`,
+    deployed: [/ALTER TABLE fu\.old RENAME TO new/, /RENAME (COLUMN )?col TO name/],
+    notDeployed: [/DROP COLUMN/, /DROP TABLE/]
+  },
+  {
+    name: 'rename-references',
+    v1: [
+      'CREATE SCHEMA fu; CREATE TABLE fu.old (id int PRIMARY KEY, col text);',
+      "COMMENT ON TABLE fu.old IS 'the table'; COMMENT ON COLUMN fu.old.col IS 'the col';",
+      'GRANT SELECT ON TABLE fu.old TO PUBLIC;',
+      'ALTER TABLE fu.old ENABLE ROW LEVEL SECURITY; CREATE POLICY p ON fu.old FOR SELECT USING (true);',
+      'CREATE VIEW fu.v AS SELECT old.id, old.col FROM fu.old;',
+      'CREATE FUNCTION fu.f() RETURNS bigint LANGUAGE sql AS $$ SELECT count(*) FROM fu.old $$;'
+    ].join('\n'),
+    v2: [
+      'CREATE SCHEMA fu; CREATE TABLE fu.new (id int PRIMARY KEY, col text);',
+      "COMMENT ON TABLE fu.new IS 'the table'; COMMENT ON COLUMN fu.new.col IS 'the col';",
+      'GRANT SELECT ON TABLE fu.new TO PUBLIC;',
+      'ALTER TABLE fu.new ENABLE ROW LEVEL SECURITY; CREATE POLICY p ON fu.new FOR SELECT USING (true);',
+      'CREATE VIEW fu.v AS SELECT new.id, new.col FROM fu.new;',
+      'CREATE FUNCTION fu.f() RETURNS bigint LANGUAGE sql AS $$ SELECT count(*) FROM fu.new $$;'
+    ].join('\n'),
+    flags: '--rename fu.old=fu.new',
+    deployed: [/ALTER TABLE fu\.old RENAME TO new/, /CREATE OR REPLACE FUNCTION fu\.f\(\)/],
+    notDeployed: [/COMMENT ON/, /DROP VIEW/, /DROP POLICY/, /REVOKE/]
+  },
+  {
+    name: 'rename-rebuild',
+    v1: 'CREATE SCHEMA fu; CREATE TABLE fu.old (id int PRIMARY KEY, col text); CREATE VIEW fu.v AS SELECT old.id, old.col FROM fu.old;',
+    v2: 'CREATE SCHEMA fu; CREATE TABLE fu.new (id int PRIMARY KEY, col int); CREATE VIEW fu.v AS SELECT new.id, new.col FROM fu.new;',
+    flags: '--rename fu.old=fu.new --allow-data-loss',
+    deployed: [/ALTER TABLE fu\.old RENAME TO new/, /DROP VIEW fu\.v/, /ALTER COLUMN col TYPE int/, /CREATE VIEW fu\.v/]
+  },
+  {
+    name: 'rename-alias',
+    v1: 'CREATE SCHEMA fu; CREATE TABLE fu.old (id int PRIMARY KEY); CREATE TABLE fu.other (id int PRIMARY KEY, n int); CREATE VIEW fu.v AS SELECT old.id, old.n FROM fu.other AS old;',
+    v2: 'CREATE SCHEMA fu; CREATE TABLE fu.new (id int PRIMARY KEY); CREATE TABLE fu.other (id int PRIMARY KEY, n int); CREATE VIEW fu.v AS SELECT old.id, old.n FROM fu.other AS old;',
+    flags: '--rename fu.old=fu.new',
+    deployed: [/ALTER TABLE fu\.old RENAME TO new/],
+    notDeployed: [/VIEW/]
+  }
+];
+
+describe('pgpm diff e2e: review follow-ups', () => {
+  let fixture: CLIDeployTestFixture;
+  let wsDir: string;
+
+  beforeAll(async () => {
+    await loadModule();
+    fixture = new CLIDeployTestFixture();
+    wsDir = path.join(fixture.tempFixtureDir, WS);
+    fs.mkdirSync(wsDir, { recursive: true });
+    fs.writeFileSync(path.join(wsDir, 'pgpm.json'), '{\n    "packages": [\n        "*"\n    ]\n}');
+  });
+
+  afterAll(async () => {
+    await fixture.cleanup();
+  });
+
+  for (const c of CASES) {
+    it(`${c.name}: migrates v1 to v2 and reverts to v1`, async () => {
+      const v1 = `${c.name}-v1`;
+      const v2 = `${c.name}-v2`;
+      const mig = `${c.name}-mig`;
+      writeModule(path.join(wsDir, v1), v1, c.v1);
+      writeModule(path.join(wsDir, v2), v2, c.v2);
+      const v2Db = await fixture.setupTestDatabase();
+      const testDb = await fixture.setupTestDatabase();
+      for (const [db, pkg] of [[v2Db, v2], [testDb, v1]] as const) {
+        await fixture.runTerminalCommands(`
+          cd ${WS}/${pkg}
+          pgpm deploy --database ${db.name} --package ${pkg} --yes
+        `, { database: db.name });
+      }
+      const v1Catalog = withoutColumnOrder(await snapshotCatalog(testDb));
+      if (c.rows) await testDb.query(c.rows);
+
+      await fixture.runTerminalCommands(`
+        cd ${WS}
+        pgpm diff ${v1} ${v2} ${c.flags} --emit-migration . --pkg ${mig}
+      `, {});
+      const migrationDir = path.join(wsDir, mig);
+      const deployed = fs.readdirSync(path.join(migrationDir, 'deploy'), { recursive: true })
+        .map(String).filter(f => f.endsWith('.sql'))
+        .map(f => fs.readFileSync(path.join(migrationDir, 'deploy', f), 'utf-8')).join('\n');
+      for (const re of c.deployed) expect(deployed).toMatch(re);
+      for (const re of c.notDeployed ?? []) expect(deployed).not.toMatch(re);
+
+      await fixture.runTerminalCommands(`
+        cd ${WS}/${mig}
+        pgpm deploy --database ${testDb.name} --package ${mig} --yes
+      `, { database: testDb.name });
+      const forward = diffCatalogSnapshots(withoutColumnOrder(await snapshotCatalog(testDb)), withoutColumnOrder(await snapshotCatalog(v2Db)));
+      expect(forward).toEqual((c.forwardResidual ?? []).map(re => expect.stringMatching(re)));
+      if (c.rows) {
+        const rows = await testDb.query('SELECT id, name FROM fu.new ORDER BY id');
+        expect(rows.rows).toEqual([{ id: 1, name: 'kept' }]);
+      }
+
+      await fixture.runTerminalCommands(`
+        cd ${WS}/${mig}
+        pgpm revert --database ${testDb.name} --package ${mig} --yes --no-recursive
+      `, { database: testDb.name });
+      const back = diffCatalogSnapshots(withoutColumnOrder(await snapshotCatalog(testDb)), v1Catalog);
+      expect(back).toEqual((c.revertResidual ?? []).map(re => expect.stringMatching(re)));
+    });
+  }
+
+  it('an added REVOKE .. FROM PUBLIC reverts to the privileges PUBLIC actually held (none on a schema, EXECUTE on a function)', async () => {
+    const v1 = 'revoke-added-v1';
+    const v2 = 'revoke-added-v2';
+    const base = 'CREATE SCHEMA fu; CREATE FUNCTION fu.f() RETURNS int LANGUAGE sql AS $$ SELECT 1 $$;';
+    writeModule(path.join(wsDir, v1), v1, base);
+    writeModule(path.join(wsDir, v2), v2, `${base} REVOKE ALL ON SCHEMA fu FROM PUBLIC; REVOKE ALL ON FUNCTION fu.f() FROM PUBLIC;`);
+    const testDb = await fixture.setupTestDatabase();
+    await fixture.runTerminalCommands(`
+      cd ${WS}/${v1}
+      pgpm deploy --database ${testDb.name} --package ${v1} --yes
+    `, { database: testDb.name });
+    // Effective privileges: a NULL acl is the owner's default, spelled out.
+    const acls = async (): Promise<unknown[]> => (await testDb.query(`
+      SELECT 'schema' AS obj, a.grantee::regrole::text AS grantee, a.privilege_type
+        FROM pg_namespace n, aclexplode(coalesce(n.nspacl, acldefault('n', n.nspowner))) a WHERE n.nspname = 'fu'
+      UNION ALL
+      SELECT 'function', a.grantee::regrole::text, a.privilege_type
+        FROM pg_proc p, aclexplode(coalesce(p.proacl, acldefault('f', p.proowner))) a
+        WHERE p.proname = 'f' AND p.pronamespace = 'fu'::regnamespace
+      ORDER BY 1, 2, 3`)).rows;
+    const before = await acls();
+    await fixture.runTerminalCommands(`
+      cd ${WS}
+      pgpm diff ${v1} ${v2} --emit-migration . --pkg revoke-added-mig
+    `, {});
+    await fixture.runTerminalCommands(`
+      cd ${WS}/revoke-added-mig
+      pgpm deploy --database ${testDb.name} --package revoke-added-mig --yes
+    `, { database: testDb.name });
+    expect(await acls()).not.toEqual(before);
+    await fixture.runTerminalCommands(`
+      cd ${WS}/revoke-added-mig
+      pgpm revert --database ${testDb.name} --package revoke-added-mig --yes --no-recursive
+    `, { database: testDb.name });
+    expect(await acls()).toEqual(before);
+  });
+
+  it('an enum value added and used in the same migration is named: Postgres cannot use it before the transaction commits', async () => {
+    const v1 = 'enum-used-v1';
+    const v2 = 'enum-used-v2';
+    writeModule(path.join(wsDir, v1), v1, `CREATE SCHEMA fu; CREATE TYPE fu.status AS ENUM ('a', 'b'); CREATE TABLE fu.t (id int PRIMARY KEY, s fu.status NOT NULL DEFAULT 'a');`);
+    writeModule(path.join(wsDir, v2), v2, `CREATE SCHEMA fu; CREATE TYPE fu.status AS ENUM ('a', 'b', 'c'); CREATE TABLE fu.t (id int PRIMARY KEY, s fu.status NOT NULL DEFAULT 'c');`);
+    await fixture.runTerminalCommands(`
+      cd ${WS}
+      pgpm diff ${v1} ${v2} --allow-incomplete --emit-migration . --pkg enum-used-mig
+    `, {});
+    const deployed = fs.readdirSync(path.join(wsDir, 'enum-used-mig', 'deploy'), { recursive: true })
+      .map(String).filter(f => f.endsWith('.sql'))
+      .map(f => fs.readFileSync(path.join(wsDir, 'enum-used-mig', 'deploy', f), 'utf-8')).join('\n');
+    expect(deployed).toMatch(/--no-tx/);
+    // Without a transaction per deploy, the migration applies and reaches v2.
+    const v2Db = await fixture.setupTestDatabase();
+    const testDb = await fixture.setupTestDatabase();
+    await fixture.runTerminalCommands(`
+      cd ${WS}/${v2}
+      pgpm deploy --database ${v2Db.name} --package ${v2} --yes
+    `, { database: v2Db.name });
+    await fixture.runTerminalCommands(`
+      cd ${WS}/${v1}
+      pgpm deploy --database ${testDb.name} --package ${v1} --yes
+    `, { database: testDb.name });
+    await fixture.runTerminalCommands(`
+      cd ${WS}/enum-used-mig
+      pgpm deploy --database ${testDb.name} --package enum-used-mig --yes --no-tx
+    `, { database: testDb.name });
+    const forward = diffCatalogSnapshots(withoutColumnOrder(await snapshotCatalog(testDb)), withoutColumnOrder(await snapshotCatalog(v2Db)));
+    expect(forward).toEqual([]);
+  });
+});

pgpm/cli/test-utils/CLIDeployTestFixture.ts

@@ -223,7 +223,9 @@ export class CLIDeployTestFixture extends TestFixture {
           argv[baseKey] = false;
           i += 1;
         } else if (i + 1 < tokens.length && !tokens[i + 1].startsWith('--')) {
-          argv[key] = tokens[i + 1];
+          // Repeated flags accumulate, as minimist does (`--rename a=b --rename c=d`).
+          const prior = argv[key];
+          argv[key] = prior === undefined ? tokens[i + 1] : [...(Array.isArray(prior) ? prior : [prior]), tokens[i + 1]];
           i += 2;
         } else {
           argv[key] = true;