fix(pg-delta): preserve REPLICA IDENTITY USING INDEX on tables (#235)

Co-authored-by: Claude <noreply@anthropic.com>

Author: avallete

.changeset/fix-replica-identity-using-index.md

@@ -0,0 +1,19 @@
+---
+"@supabase/pg-delta": patch
+---
+
+fix(pg-delta): preserve `REPLICA IDENTITY USING INDEX` on tables instead of silently reverting to `DEFAULT` on declarative sync.
+
+The table extractor only stored `replica_identity` as a single character (`'d' | 'n' | 'f' | 'i'`) and discarded the index name when the mode was `'i'`. The diff path then explicitly skipped mode `'i'` ("handled by index changes" — but no such handler existed), and `AlterTableSetReplicaIdentity.serialize()` fell back to `REPLICA IDENTITY DEFAULT` for that mode. Compounding this, `Index.is_replica_identity` participated in equality and was marked non-alterable, so toggling the flag on the index triggered a spurious `DROP INDEX` + `CREATE INDEX` — and Postgres reverts the table to `REPLICA IDENTITY DEFAULT` whenever the configured replica-identity index is dropped.
+
+End result: a table configured with `ALTER TABLE foo REPLICA IDENTITY USING INDEX foo_idx` would extract as `replica_identity = 'i'` but produce no setter on diff. The next `declarative sync` would generate a migration that dropped the user's index, reset the table to `DEFAULT`, and recreated the index — never converging (reported as supabase/cli#5141).
+
+The fix:
+
+- `Table.replica_identity_index` is extracted via `pg_index.indisreplident` and included in `dataFields`, so the index name participates in equality.
+- `AlterTableSetReplicaIdentity` now serializes `REPLICA IDENTITY USING INDEX <name>` for mode `'i'` and declares the index as a `requires` dependency so it is created first.
+- The table diff emits the change for all modes (including `'i'`) on both `CREATE` and `ALTER`, and re-emits when the configured index name changes while staying in `'i'` mode.
+- `Index.is_replica_identity` is no longer in `dataFields` / `NON_ALTERABLE_FIELDS`; the table side is the source of truth, set via `ALTER TABLE`. This stops the spurious `DROP INDEX` + `CREATE INDEX` cycle.
+- A new `restoreReplicaIdentityAfterIndexReplace` pass in `post-diff-normalization.ts` re-emits `ALTER TABLE ... REPLICA IDENTITY USING INDEX <name>` after any `DropIndex(idx) + CreateIndex(idx)` pair where `idx` is the replica-identity index of a branch table. This covers the second flavor of the bug: when both main and branch already point at the same replica-identity index, but that index's *definition* changes (e.g. a column added to its key), the index is replaced, Postgres silently flips `relreplident` to `'d'`, and the table-level diff alone cannot see the cross-object interaction. The pass is idempotent — if `diffTables()` already emitted the same setter (because the table is also flipping mode or pointing to a different index), no duplicate is added.
+
+The post-diff layer file `src/core/post-diff-cycle-breaking.ts` is renamed to `post-diff-normalization.ts` and `normalizePostDiffCycles` to `normalizePostDiffChanges` — the file already contained dedup and replacement-superseded pruning that aren't strictly cycle-breaking, and actual cycle breaking moved to the lazy sort-phase dispatcher in a previous release. The rename brings the file in line with the "post-diff normalization" terminology already used in the package's `CLAUDE.md` rule of thumb.

.github/agents/pg-toolbelt.md

@@ -243,6 +243,69 @@ pg-delta has 45+ integration test files across 3 PG versions, sharded across 15
 
 - Run `bun run test:pg-delta` (full suite) only after all changes are complete and targeted tests pass
 
+### Running integration tests in a sandbox (no systemd, no Docker daemon)
+
+Cloud sandboxes (e.g. Claude Code on the web) typically ship with the Docker
+client installed but no running daemon and no registry credentials. If
+`docker info` reports `Cannot connect to the Docker daemon`, set it up
+yourself instead of giving up and skipping integration coverage:
+
+1. **Start `dockerd` directly** (no systemd in these sandboxes — `systemctl`
+   and `/etc/init.d/docker` will both fail with `Operation not permitted`):
+
+   ```bash
+   sudo dockerd > /tmp/dockerd.log 2>&1 &
+   sleep 5
+   docker info | grep "Server Version"   # confirm the daemon is up
+   ```
+
+2. **Configure a registry mirror before pulling.** Anonymous Docker Hub pulls
+   are rate-limited per source IP and the limit is reached almost immediately
+   on shared CI/sandbox egress. `mirror.gcr.io` is a Google-hosted pull-through
+   cache for Docker Hub `library/*` and other public images and works without
+   credentials:
+
+   ```bash
+   sudo mkdir -p /etc/docker
+   echo '{"registry-mirrors": ["https://mirror.gcr.io"]}' | sudo tee /etc/docker/daemon.json
+   sudo pkill dockerd; sleep 2
+   sudo dockerd > /tmp/dockerd.log 2>&1 &
+   sleep 5
+   docker info | grep -A1 "Registry Mirrors"   # confirm
+   ```
+
+3. **Pre-pull only the images you need.** `tests/global-setup.ts` pulls *all*
+   Alpine + Supabase tags listed in `tests/constants.ts` at startup. Always
+   limit the matrix with `PGDELTA_TEST_POSTGRES_VERSIONS=17` (or `15`) so the
+   preload only fetches the tags relevant to your run:
+
+   ```bash
+   docker pull postgres:17.6-alpine
+   docker pull supabase/postgres:17.6.1.107   # only if your test uses withDbSupabase*
+   ```
+
+4. **Run integration tests as usual** — the global-setup will reuse the cached
+   images:
+
+   ```bash
+   cd packages/pg-delta
+   PGDELTA_TEST_POSTGRES_VERSIONS=17 bun run test tests/integration/<file>.test.ts
+   ```
+
+If you cannot get Docker running (e.g. the sandbox blocks `dockerd`'s
+networking even with the mirror), say so explicitly in your final report —
+do not silently skip the integration step. For unit-only iteration, you can
+bypass the bunfig `preload` (which loads `tests/global-setup.ts` and tries to
+contact the Docker daemon) by invoking `bun test` from outside the package
+directory:
+
+```bash
+cd /tmp && bun test /home/user/pg-toolbelt/packages/pg-delta/src/...
+```
+
+This is a workaround for fast unit-test feedback only; integration tests
+still need a working Docker daemon.
+
 ### Upgrading Supabase test images
 
 When changing `packages/pg-delta/tests/constants.ts`, especially

packages/pg-delta/CLAUDE.md

@@ -88,7 +88,7 @@ for (const pgVersion of POSTGRES_VERSIONS) {
 Keep cycle handling split by the scope of information it needs:
 
 - **Object-local PostgreSQL semantics stay in `diff*`**. If a single object diff can prove a statement is redundant or invalid on its own, fix it there. Example: `src/core/objects/sequence/sequence.diff.ts` skips `DROP SEQUENCE` when `OWNED BY` means PostgreSQL will already cascade-drop it with the owning table/column.
-- **Whole-plan interactions belong in post-diff normalization**. If the fix only becomes obvious after multiple emitted changes are combined, implement it in `src/core/post-diff-cycle-breaking.ts`, wired from `src/core/catalog.diff.ts` after raw diffs and `expandReplaceDependencies()`. Current examples: mutual dropped-table FK cycles and pruning same-table `AlterTableDropColumn` / `AlterTableDropConstraint` changes that are superseded by an expansion-added `DropTable+CreateTable` pair.
+- **Whole-plan interactions belong in post-diff normalization**. If the fix only becomes obvious after multiple emitted changes are combined, implement it in `src/core/post-diff-normalization.ts` (`normalizePostDiffChanges`), wired from `src/core/catalog.diff.ts` after raw diffs and `expandReplaceDependencies()`. The pass is the single chokepoint that observes the final `Change[]`, so it catches cross-object effects regardless of whether the relevant change pair was emitted by an object's `diff*` (e.g. `index.diff` for a definition-changed index) or by `expandReplaceDependencies()` (dependency-closure replacement). Current examples: pruning same-table `AlterTableDropColumn` / `AlterTableDropConstraint` changes superseded by an expansion-added `DropTable+CreateTable` pair, deduplicating constraint Add/Validate/Comment on replaced tables, and re-emitting `ALTER TABLE … REPLICA IDENTITY USING INDEX` after a replica-identity index is dropped+recreated.
 - **`expandReplaceDependencies()` only computes replacement closure**. It may report metadata such as which tables were promoted to replacement pairs, but it should not own unrelated cycle-pruning policy.
 - **`src/core/sort/dependency-filter.ts` is a narrow last resort**. Use it only for safe edge filtering where the emitted statements are already valid and only the graph edge is artificial. Do not extend sort-phase filtering to paper over plans that would still fail at apply time.
 
@@ -108,6 +108,20 @@ Used to track objects across databases (OIDs differ per environment):
 - Sub-entities: `type:schema.parent.name` (e.g. `column:public.users.email`).
 - Metadata: `scope:target` (e.g. `comment:public.users`).
 
+**Always build stable identifiers through the `stableId.*` helpers in
+`src/core/objects/utils.ts` (or the `<Object>.stableId` getter on a model
+instance) — never inline the prefix as a template literal.** Inline strings
+like `` `index:${schema}.${table}.${name}` `` drift from the helper if
+prefixes or escaping rules change, scatter the format across the codebase,
+and were caught in review on this exact pattern. If you need a stable id
+for an object type that does not have a helper yet, add the helper to
+`stableId` first, then use it everywhere — including new code paths,
+post-diff passes, and dependency wiring inside change classes.
+
+When asserting stable ids in tests, the literal form is fine as the
+expected value (it documents the on-the-wire format), but the **production
+side** of the comparison should still call the helper.
+
 ### Integration DSL
 
 - **Filter**: JSON pattern to include/exclude changes (e.g. `{ "not": { "schema": ["pg_catalog"] } }`).

packages/pg-delta/src/core/catalog.diff.ts

@@ -1,7 +1,7 @@
 import debug from "debug";
 import type { Catalog } from "./catalog.model.ts";
 import { expandReplaceDependencies } from "./expand-replace-dependencies.ts";
-import { normalizePostDiffCycles } from "./post-diff-cycle-breaking.ts";
+import { normalizePostDiffChanges } from "./post-diff-normalization.ts";
 
 const debugCatalog = debug("pg-delta:catalog");
 
@@ -238,9 +238,10 @@ export function diffCatalogs(
     mainCatalog: main,
     branchCatalog: branch,
   });
-  filteredChanges = normalizePostDiffCycles({
+  filteredChanges = normalizePostDiffChanges({
     changes: expandedDependencies.changes,
     replacedTableIds: expandedDependencies.replacedTableIds,
+    branchTables: branch.tables,
   });
 
   debugCatalog(

packages/pg-delta/src/core/objects/index/index.diff.ts

@@ -104,7 +104,6 @@ export function diffIndexes(
       "nulls_not_distinct",
       "immediate",
       "is_clustered",
-      "is_replica_identity",
       "column_collations",
       "operator_classes",
       "column_options",

packages/pg-delta/src/core/objects/index/index.model.ts

@@ -163,7 +163,11 @@ export class Index extends BasePgModel {
       nulls_not_distinct: this.nulls_not_distinct,
       immediate: this.immediate,
       is_clustered: this.is_clustered,
-      is_replica_identity: this.is_replica_identity,
+      // is_replica_identity excluded: the table's `replica_identity` /
+      // `replica_identity_index` is the source of truth, set via
+      // ALTER TABLE ... REPLICA IDENTITY USING INDEX. Including this flag here
+      // would trigger spurious DROP+CREATE of the index whenever the table's
+      // replica identity changes.
       // key_columns excluded: contains attribute numbers that can differ between databases
       // even when indexes are logically identical. The definition field already captures
       // the logical structure using column names, so we compare by definition instead.

packages/pg-delta/src/core/objects/table/changes/table.alter.test.ts

@@ -343,7 +343,7 @@ describe.concurrent("table", () => {
       ).toBe("ALTER TABLE public.test_table REPLICA IDENTITY FULL");
     });
 
-    test("replica identity DEFAULT and INDEX fallback", async () => {
+    test("replica identity DEFAULT and USING INDEX", async () => {
       const baseProps: Omit<
         TableProps,
         "owner" | "options" | "replica_identity"
@@ -372,31 +372,23 @@ describe.concurrent("table", () => {
         options: null,
         replica_identity: "n",
       });
-      const toDefault = new Table({
-        ...baseProps,
-        owner: "o1",
-        options: null,
-        replica_identity: "d",
-      });
-      const toIndex = new Table({
-        ...baseProps,
-        owner: "o1",
-        options: null,
-        replica_identity: "i",
-      });
       expect(
         new AlterTableSetReplicaIdentity({
           table,
-          mode: toDefault.replica_identity,
-        }).serialize(),
-      ).toBe("ALTER TABLE public.test_table REPLICA IDENTITY DEFAULT");
-      // AlterTableSetReplicaIdentity of type "i" will not be emitted in diff, it is handled by index changes, we fallback to DEFAULT here
-      expect(
-        new AlterTableSetReplicaIdentity({
-          table,
-          mode: toIndex.replica_identity,
+          mode: "d",
         }).serialize(),
       ).toBe("ALTER TABLE public.test_table REPLICA IDENTITY DEFAULT");
+      const usingIndex = new AlterTableSetReplicaIdentity({
+        table,
+        mode: "i",
+        indexName: "test_table_pkey",
+      });
+      expect(usingIndex.serialize()).toBe(
+        "ALTER TABLE public.test_table REPLICA IDENTITY USING INDEX test_table_pkey",
+      );
+      expect(usingIndex.requires).toContain(
+        "index:public.test_table.test_table_pkey",
+      );
     });
 
     test("columns add/drop/alter", async () => {

packages/pg-delta/src/core/objects/table/changes/table.alter.ts

@@ -462,20 +462,46 @@ export class AlterTableValidateConstraint extends AlterTableChange {
 
 /**
  * ALTER TABLE ... REPLICA IDENTITY ...
+ *
+ * When `mode === "i"` (USING INDEX), `indexName` is the name of the index to
+ * use. The extractor populates `Table.replica_identity_index` from
+ * `pg_index.indisreplident` whenever `Table.replica_identity` is `'i'`, so
+ * callers that source their props from a `Table` instance can rely on the
+ * pair being consistent. The non-null assertions in `requires` / `serialize`
+ * below are justified by that data invariant — the same pattern the FK
+ * branch of `AlterTableAddConstraint` uses for `foreign_key_columns!` /
+ * `foreign_key_table!` / `foreign_key_schema!`.
  */
 export class AlterTableSetReplicaIdentity extends AlterTableChange {
   public readonly table: Table;
   public readonly mode: "d" | "n" | "f" | "i";
+  public readonly indexName: string | null;
   public readonly scope = "object" as const;
 
-  constructor(props: { table: Table; mode: "d" | "n" | "f" | "i" }) {
+  constructor(props: {
+    table: Table;
+    mode: "d" | "n" | "f" | "i";
+    indexName?: string | null;
+  }) {
     super();
     this.table = props.table;
     this.mode = props.mode;
+    this.indexName = props.indexName ?? null;
   }
 
   get requires() {
-    return [this.table.stableId];
+    const reqs: string[] = [this.table.stableId];
+    if (this.mode === "i") {
+      reqs.push(
+        stableId.index(
+          this.table.schema,
+          this.table.name,
+          // biome-ignore lint/style/noNonNullAssertion: mode 'i' implies the extractor populated replica_identity_index
+          this.indexName!,
+        ),
+      );
+    }
+    return reqs;
   }
 
   serialize(_options?: SerializeOptions): string {
@@ -486,7 +512,8 @@ export class AlterTableSetReplicaIdentity extends AlterTableChange {
           ? "NOTHING"
           : this.mode === "f"
             ? "FULL"
-            : "DEFAULT"; // 'i' (USING INDEX) is handled via index changes; fallback to DEFAULT
+            : // biome-ignore lint/style/noNonNullAssertion: mode 'i' implies the extractor populated replica_identity_index
+              `USING INDEX ${this.indexName!}`;
     return [
       "ALTER TABLE",
       `${this.table.schema}.${this.table.name}`,

packages/pg-delta/src/core/objects/table/table.diff.ts

@@ -245,15 +245,13 @@ export function diffTables(
 
     // REPLICA IDENTITY: If non-default, emit ALTER TABLE ... REPLICA IDENTITY
     if (branchTable.replica_identity !== "d") {
-      // Skip 'i' (USING INDEX) — handled by index changes
-      if (branchTable.replica_identity !== "i") {
-        changes.push(
-          new AlterTableSetReplicaIdentity({
-            table: branchTable,
-            mode: branchTable.replica_identity,
-          }),
-        );
-      }
+      changes.push(
+        new AlterTableSetReplicaIdentity({
+          table: branchTable,
+          mode: branchTable.replica_identity,
+          indexName: branchTable.replica_identity_index,
+        }),
+      );
     }
 
     changes.push(
@@ -404,16 +402,23 @@ export function diffTables(
     }
 
     // REPLICA IDENTITY
-    if (mainTable.replica_identity !== branchTable.replica_identity) {
-      // Skip when target is 'i' (USING INDEX) — handled by index changes
-      if (branchTable.replica_identity !== "i") {
-        changes.push(
-          new AlterTableSetReplicaIdentity({
-            table: mainTable,
-            mode: branchTable.replica_identity,
-          }),
-        );
-      }
+    // Re-emit when the mode changes, or when staying in 'i' mode but pointing
+    // at a different index. The index named on the branch must already exist
+    // before this ALTER runs; AlterTableSetReplicaIdentity declares that
+    // dependency in its `requires`.
+    const replicaIdentityChanged =
+      mainTable.replica_identity !== branchTable.replica_identity ||
+      (branchTable.replica_identity === "i" &&
+        mainTable.replica_identity_index !==
+          branchTable.replica_identity_index);
+    if (replicaIdentityChanged) {
+      changes.push(
+        new AlterTableSetReplicaIdentity({
+          table: mainTable,
+          mode: branchTable.replica_identity,
+          indexName: branchTable.replica_identity_index,
+        }),
+      );
     }
 
     // OWNER