Skip to content

Commit 08103c0

Browse files
authored
fix(cli): skip provisioned ledger ddl (CLI-2275) (#6422)
## TL;DR stop sending the migration ledger DDL to remotes that already have the ledger, which is the traffic the Supavisor session pooler kills... ## what's biting? migration repair dies with a connection error over the session pooler while migration list works on the same connection. every history command runs the seven statement ledger setup transaction on every invocation, even when the ledger already exists, and that DDL is what the pooler drops. list only sends a SELECT. ## now fixed by: a read-only probe with the same wire shape as the list SELECT. ledger fully there: no DDL is sent. anything less, or any odd probe answer: the full unchanged DDL runs, so ledger upgrades behave exactly as before. covers the history and seed ledgers... ## ref: - closes: #6393
1 parent 6b85fba commit 08103c0

6 files changed

Lines changed: 183 additions & 41 deletions

File tree

apps/cli/src/legacy/commands/db/push/SIDE_EFFECTS.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,10 +30,10 @@ before migrations unless `--skip-vault` is set.
3030
| Statement | When |
3131
| ------------------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
3232
| `RESET ALL` + migration statements + `INSERT INTO supabase_migrations.schema_migrations(version, name, statements)` | per pending migration (after confirmation); compatible statements use an implicit extended-protocol batch with one final `Sync`, while pipeline-incompatible statements run standalone — see Notes |
33-
| `CREATE SCHEMA/TABLE … supabase_migrations.schema_migrations`, `ALTER TABLE … ADD COLUMN …` | once before applying migrations (idempotent) |
33+
| `CREATE SCHEMA/TABLE … supabase_migrations.schema_migrations`, `ALTER TABLE … ADD COLUMN …` | once before applying migrations, when a read-only probe finds the ledger not yet provisioned (idempotent; supabase/cli#6393) |
3434
| `roles.sql` statements (no history row) | per `--include-roles` globals file (after confirmation); statements use an implicit extended-protocol batch with one final `Sync` |
3535
| `SELECT id, name FROM vault.secrets …`, `SELECT vault.update_secret(...)`, `SELECT vault.create_secret(...)` | when `[db.vault]` has syncable secrets, migrations are applied, and `--skip-vault` is not set |
36-
| `CREATE TABLE … supabase_migrations.seed_files`, seed statements, `INSERT … seed_files(path, hash) … ON CONFLICT …` | per pending seed file with `--include-seed` (after confirmation); a dirty seed only refreshes the hash |
36+
| `CREATE TABLE … supabase_migrations.seed_files`, seed statements, `INSERT … seed_files(path, hash) … ON CONFLICT …` | per pending seed file with `--include-seed` (after confirmation; the `seed_files` DDL only when a read-only probe finds that ledger not yet provisioned); a dirty seed only refreshes the hash |
3737
| `SET SESSION ROLE postgres` | stepped-down sessions only (`cli_login_*`/`supabase_admin`): after each top-level role-reverting statement, at the end of each migration/globals/seed file, and before the history insert and the `seed_files` upsert (CLI-2205, #6236) |
3838

3939
## API Routes

apps/cli/src/legacy/commands/migration/repair/SIDE_EFFECTS.md

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -42,9 +42,11 @@
4242
When repairing specific versions, prints `Repaired migration history: [<versions>]
4343
=> <status>` to stderr, then `Finished supabase migration repair.` to stdout and
4444
the suggestion `Run supabase migration list to show the updated migration history.`
45-
to stderr. The DB mutation is one transaction: create the history table, then (for
46-
repair-all) `TRUNCATE`, plus `applied` → per-version `UPSERT` from the local file,
47-
`reverted``DELETE ... WHERE version = ANY($1)`.
45+
to stderr. The DB work is the history-table provisioning (its own transaction,
46+
skipped entirely when a read-only probe finds the ledger already provisioned, so a
47+
provisioned remote runs no provisioning DDL — supabase/cli#6393) followed by one
48+
repair transaction: (for repair-all) `TRUNCATE`, plus `applied` → per-version
49+
`UPSERT` from the local file, `reverted``DELETE ... WHERE version = ANY($1)`.
4850

4951
> **Atomicity note:** the old Go CLI ran the TRUNCATE/UPSERT/DELETE via a batched
5052
> pipeline (not an explicit transaction), so a partial failure mid-batch (e.g.

apps/cli/src/legacy/shared/legacy-migration-apply.unit.test.ts

Lines changed: 21 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -391,7 +391,9 @@ describe("legacyApplyMigrationFile", () => {
391391
expect(action).toBeGreaterThan(set);
392392
expect(cleanup).toBeGreaterThan(action);
393393

394-
const history = calls.filter((call) => call.kind === "query");
394+
const history = calls.filter(
395+
(call) => call.kind === "query" && call.params !== undefined,
396+
);
395397
expect(history).toHaveLength(1);
396398
expect(history[0]?.params).toEqual([
397399
"20240101120000",
@@ -422,7 +424,9 @@ describe("legacyApplyMigrationFile", () => {
422424
expect(Exit.isFailure(exit)).toBe(true);
423425
const execs = calls.filter((call) => call.kind === "exec").map((call) => call.sql);
424426
expect(execs.at(-1)).toBe("RESET ALL");
425-
expect(calls.some((call) => call.kind === "query")).toBe(false);
427+
expect(calls.some((call) => call.kind === "query" && call.params !== undefined)).toBe(
428+
false,
429+
);
426430
if (Exit.isFailure(exit)) {
427431
expect(JSON.stringify(exit.cause)).toContain("At statement: 1");
428432
}
@@ -474,7 +478,9 @@ describe("legacyApplyMigrationFile", () => {
474478
expect(execs.filter((sql) => sql === "BEGIN")).toHaveLength(2);
475479
expect(execs.filter((sql) => sql === "COMMIT")).toHaveLength(2);
476480
expect(execs).toContain("SET LOCAL check_function_bodies = off");
477-
const history = calls.filter((call) => call.kind === "query");
481+
const history = calls.filter(
482+
(call) => call.kind === "query" && call.params !== undefined,
483+
);
478484
expect(history).toHaveLength(1);
479485
expect(history[0]?.params?.[0]).toBe("20240101120000");
480486
rmSync(dir, { recursive: true, force: true });
@@ -510,7 +516,9 @@ describe("legacyApplyMigrationFile", () => {
510516
expect.stringContaining("supabase_migrations.schema_migrations"),
511517
]);
512518
// No standalone history insert: it rides the same batch as the statements.
513-
expect(calls.filter((call) => call.kind === "query")).toHaveLength(0);
519+
expect(
520+
calls.filter((call) => call.kind === "query" && call.params !== undefined),
521+
).toHaveLength(0);
514522
rmSync(dir, { recursive: true, force: true });
515523
}),
516524
),
@@ -527,7 +535,9 @@ describe("legacyApplyMigrationFile", () => {
527535
Effect.tap((exit) =>
528536
Effect.sync(() => {
529537
expect(Exit.isFailure(exit)).toBe(true);
530-
expect(calls.some((call) => call.kind === "query")).toBe(false);
538+
expect(calls.some((call) => call.kind === "query" && call.params !== undefined)).toBe(
539+
false,
540+
);
531541
expect(calls.some((call) => call.kind === "exec" && call.sql === "ROLLBACK")).toBe(true);
532542
rmSync(dir, { recursive: true, force: true });
533543
}),
@@ -601,7 +611,9 @@ describe("legacyApplyMigrationFile", () => {
601611
const authoredCommit = calls.findLastIndex(
602612
(call) => call.kind === "exec" && call.sql === "COMMIT",
603613
);
604-
const history = calls.findIndex((call) => call.kind === "query");
614+
const history = calls.findIndex(
615+
(call) => call.kind === "query" && call.params !== undefined,
616+
);
605617
expect(lastRestore).toBeGreaterThan(authoredCommit);
606618
expect(history).toBeGreaterThan(lastRestore);
607619
rmSync(dir, { recursive: true, force: true });
@@ -801,7 +813,9 @@ describe("legacyApplyMigrationFile", () => {
801813
const execs = calls.filter((call) => call.kind === "exec").map((call) => call.sql);
802814
expect(execs.filter((sql) => sql === "SET SESSION ROLE postgres")).toHaveLength(1);
803815
expect(execs[execs.indexOf("reset role") + 1]).toBe("SET SESSION ROLE postgres");
804-
expect(calls.filter((call) => call.kind === "query")).toHaveLength(1);
816+
expect(
817+
calls.filter((call) => call.kind === "query" && call.params !== undefined),
818+
).toHaveLength(1);
805819
rmSync(dir, { recursive: true, force: true });
806820
}),
807821
),

apps/cli/src/legacy/shared/legacy-migration-history.ts

Lines changed: 63 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -75,36 +75,77 @@ const SELECT_SEED_TABLE = "SELECT path, hash FROM supabase_migrations.seed_files
7575
/** `pkg/migration/file.go` — `<digits>_<name>.sql`. */
7676
export const MIGRATE_FILE_PATTERN = /^([0-9]+)_(.*)\.sql$/u;
7777

78+
/**
79+
* Read-only probe: `true` when the relation is an ordinary or partitioned table
80+
* carrying every live column its DDL creates, i.e. when each setup statement
81+
* below is a guaranteed no-op. Sent with no bind parameters, matching the wire
82+
* shape of the `migration list` SELECT that demonstrably survives the poolers
83+
* this setup DDL dies on (supabase/cli#6393). Any unexpected answer falls
84+
* through to the DDL path, but a probe FAILURE deliberately aborts instead: a
85+
* connection that cannot serve this SELECT will not serve the setup transaction
86+
* either, and failing loudly surfaces the real error rather than masking it
87+
* behind a DDL failure. Interpolates its arguments verbatim: callers pass
88+
* compile-time literals only.
89+
*/
90+
const legacyProvisionedProbe = (relation: string, columns: ReadonlyArray<string>) =>
91+
`SELECT count(*) = ${columns.length} AS provisioned FROM pg_catalog.pg_attribute a JOIN pg_catalog.pg_class c ON c.oid = a.attrelid WHERE a.attrelid = pg_catalog.to_regclass('${relation}') AND c.relkind IN ('r', 'p') AND NOT a.attisdropped AND a.attname IN (${columns.map((column) => `'${column}'`).join(", ")})`;
92+
93+
const SELECT_VERSION_TABLE_PROVISIONED = legacyProvisionedProbe(
94+
"supabase_migrations.schema_migrations",
95+
["version", "name", "statements"],
96+
);
97+
const SELECT_SEED_TABLE_PROVISIONED = legacyProvisionedProbe("supabase_migrations.seed_files", [
98+
"path",
99+
"hash",
100+
]);
101+
102+
const legacyIsTableProvisioned = (session: LegacyDbSession, probe: string) =>
103+
session.query(probe).pipe(Effect.map((rows) => rows[0]?.["provisioned"] === true));
104+
78105
/**
79106
* Creates the migration-history schema/table (idempotent). `CreateMigrationTable`.
80-
* The setup runs in one transaction so `SET LOCAL lock_timeout` is scoped to it and
81-
* reverts on `COMMIT`, matching Go's implicit `pgconn.ExecBatch` transaction; the GUC
82-
* never leaks into the caller's subsequent work. A failed statement rolls back.
107+
* Skipped entirely when the provisioning probe finds the ledger already current, so
108+
* an already-provisioned remote runs no DDL (supabase/cli#6393); an older or partial
109+
* ledger still gets the full setup. The setup runs in one transaction so
110+
* `SET LOCAL lock_timeout` is scoped to it and reverts on `COMMIT`, matching Go's
111+
* implicit `pgconn.ExecBatch` transaction; the GUC never leaks into the caller's
112+
* subsequent work. A failed statement rolls back.
83113
*/
84114
export const legacyCreateMigrationTable = (session: LegacyDbSession) =>
85-
Effect.gen(function* () {
86-
yield* session.exec("BEGIN");
87-
yield* session.exec(SET_LOCAL_LOCK_TIMEOUT);
88-
yield* session.exec(CREATE_VERSION_SCHEMA);
89-
yield* session.exec(CREATE_VERSION_TABLE);
90-
yield* session.exec(ADD_STATEMENTS_COLUMN);
91-
yield* session.exec(ADD_NAME_COLUMN);
92-
yield* session.exec("COMMIT");
93-
}).pipe(Effect.tapError(() => session.exec("ROLLBACK").pipe(Effect.ignore)));
115+
Effect.flatMap(
116+
legacyIsTableProvisioned(session, SELECT_VERSION_TABLE_PROVISIONED),
117+
(provisioned) =>
118+
provisioned
119+
? Effect.void
120+
: Effect.gen(function* () {
121+
yield* session.exec("BEGIN");
122+
yield* session.exec(SET_LOCAL_LOCK_TIMEOUT);
123+
yield* session.exec(CREATE_VERSION_SCHEMA);
124+
yield* session.exec(CREATE_VERSION_TABLE);
125+
yield* session.exec(ADD_STATEMENTS_COLUMN);
126+
yield* session.exec(ADD_NAME_COLUMN);
127+
yield* session.exec("COMMIT");
128+
}).pipe(Effect.tapError(() => session.exec("ROLLBACK").pipe(Effect.ignore))),
129+
);
94130

95131
/**
96-
* Creates the `seed_files` schema/table (idempotent). `CreateSeedTable`. Same
97-
* transaction-scoped `SET LOCAL lock_timeout` as `legacyCreateMigrationTable` so the
98-
* timeout reverts on `COMMIT` and never leaks into the seed SQL the caller runs next.
132+
* Creates the `seed_files` schema/table (idempotent). `CreateSeedTable`. Probed and
133+
* skipped when already provisioned; otherwise the same transaction-scoped
134+
* `SET LOCAL lock_timeout` as `legacyCreateMigrationTable` so the timeout reverts on
135+
* `COMMIT` and never leaks into the seed SQL the caller runs next.
99136
*/
100137
export const legacyCreateSeedTable = (session: LegacyDbSession) =>
101-
Effect.gen(function* () {
102-
yield* session.exec("BEGIN");
103-
yield* session.exec(SET_LOCAL_LOCK_TIMEOUT);
104-
yield* session.exec(CREATE_VERSION_SCHEMA);
105-
yield* session.exec(CREATE_SEED_TABLE);
106-
yield* session.exec("COMMIT");
107-
}).pipe(Effect.tapError(() => session.exec("ROLLBACK").pipe(Effect.ignore)));
138+
Effect.flatMap(legacyIsTableProvisioned(session, SELECT_SEED_TABLE_PROVISIONED), (provisioned) =>
139+
provisioned
140+
? Effect.void
141+
: Effect.gen(function* () {
142+
yield* session.exec("BEGIN");
143+
yield* session.exec(SET_LOCAL_LOCK_TIMEOUT);
144+
yield* session.exec(CREATE_VERSION_SCHEMA);
145+
yield* session.exec(CREATE_SEED_TABLE);
146+
yield* session.exec("COMMIT");
147+
}).pipe(Effect.tapError(() => session.exec("ROLLBACK").pipe(Effect.ignore))),
148+
);
108149

109150
/** A recorded seed file's path + content hash. `migration.SeedFile`. */
110151
export interface LegacySeedRow {

apps/cli/src/legacy/shared/legacy-migration-history.unit.test.ts

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@ import { stripAnsi } from "../../../tests/helpers/ansi.ts";
55
import { LegacyDbExecError } from "./legacy-db-connection.errors.ts";
66
import type { LegacyDbSession } from "./legacy-db-connection.service.ts";
77
import {
8+
legacyCreateMigrationTable,
9+
legacyCreateSeedTable,
810
legacyFindPendingMigrations,
911
legacyListRemoteMigrations,
1012
legacyReconcileMigrations,
@@ -239,3 +241,81 @@ describe("legacyResolveMigrationFile (byte-ordered match, Go's sort.Strings via
239241
);
240242
});
241243
});
244+
245+
describe("legacyCreateMigrationTable / legacyCreateSeedTable (provisioning probe, #6393)", () => {
246+
const HISTORY_DDL = [
247+
"BEGIN",
248+
"SET LOCAL lock_timeout = '4s'",
249+
"CREATE SCHEMA IF NOT EXISTS supabase_migrations",
250+
"CREATE TABLE IF NOT EXISTS supabase_migrations.schema_migrations (version text NOT NULL PRIMARY KEY)",
251+
"ALTER TABLE supabase_migrations.schema_migrations ADD COLUMN IF NOT EXISTS statements text[]",
252+
"ALTER TABLE supabase_migrations.schema_migrations ADD COLUMN IF NOT EXISTS name text",
253+
"COMMIT",
254+
];
255+
256+
const probedSession = (probeRows: ReadonlyArray<Record<string, unknown>>) => {
257+
const execs: Array<string> = [];
258+
const queries: Array<{ sql: string; params?: ReadonlyArray<unknown> }> = [];
259+
const session: LegacyDbSession = {
260+
exec: (sql) => {
261+
execs.push(sql);
262+
return Effect.void;
263+
},
264+
execBatch: () => Effect.die("unused"),
265+
query: (sql, params) => {
266+
queries.push({ sql, ...(params === undefined ? {} : { params }) });
267+
return Effect.succeed(probeRows);
268+
},
269+
extensionExists: () => Effect.die("unused"),
270+
copyToCsv: () => Effect.die("unused"),
271+
queryRaw: () => Effect.die("unused"),
272+
};
273+
return { session, execs, queries };
274+
};
275+
276+
it("puts no DDL on the wire when a ledger is already provisioned", async () => {
277+
const { session, execs, queries } = probedSession([{ provisioned: true }]);
278+
await Effect.runPromise(legacyCreateMigrationTable(session));
279+
await Effect.runPromise(legacyCreateSeedTable(session));
280+
expect(execs).toEqual([]);
281+
expect(queries).toHaveLength(2);
282+
expect(queries[0]?.sql).toContain("'supabase_migrations.schema_migrations'");
283+
expect(queries[1]?.sql).toContain("'supabase_migrations.seed_files'");
284+
});
285+
286+
it("keeps the probes on the simple query protocol: no bind parameters", async () => {
287+
const { session, queries } = probedSession([{ provisioned: true }]);
288+
await Effect.runPromise(legacyCreateMigrationTable(session));
289+
await Effect.runPromise(legacyCreateSeedTable(session));
290+
expect(queries).toHaveLength(2);
291+
expect(queries.filter((q) => q.params !== undefined)).toEqual([]);
292+
expect(queries.filter((q) => q.sql.includes("$"))).toEqual([]);
293+
});
294+
295+
for (const [shape, rows] of [
296+
["an absent ledger (no rows)", []],
297+
["a partial ledger (provisioned: false)", [{ provisioned: false }]],
298+
["an unexpected result shape", [{ wat: 1 }]],
299+
] as const) {
300+
it(`runs the full provisioning DDL in order against ${shape}`, async () => {
301+
const { session, execs } = probedSession([...rows]);
302+
await Effect.runPromise(legacyCreateMigrationTable(session));
303+
expect(execs).toEqual(HISTORY_DDL);
304+
});
305+
}
306+
307+
it("propagates a probe failure without opening a transaction to roll back", async () => {
308+
const error = new LegacyDbExecError({ message: "effect/sql/SqlError: Connection error" });
309+
const execs: Array<string> = [];
310+
const session: LegacyDbSession = {
311+
...failingSession(error),
312+
exec: (sql) => {
313+
execs.push(sql);
314+
return Effect.void;
315+
},
316+
};
317+
expect(await Effect.runPromise(Effect.flip(legacyCreateMigrationTable(session)))).toBe(error);
318+
expect(await Effect.runPromise(Effect.flip(legacyCreateSeedTable(session)))).toBe(error);
319+
expect(execs).toEqual([]);
320+
});
321+
});

0 commit comments

Comments
 (0)