fix(cli): repair remote db pull with pg-delta and per-connection role step-down#5895
fix(cli): repair remote db pull with pg-delta and per-connection role step-down#5895avallete wants to merge 6 commits into
Conversation
… step-down Remote `db pull --linked` with the pg-delta engine reported "No schema changes found" on every hosted project, and the migra fallback failed at the final migration-history write (CLI-1919, #5826). Three stacked causes: - pg-delta (<= alpha.31) extracted user mappings from the superuser-only `pg_catalog.pg_user_mapping` catalog, so extraction as the temp `cli_login_postgres` role failed with SQLSTATE 42501. Fixed upstream in 1.0.0-alpha.32 (world-readable `pg_user_mappings` view); bump the default pinned version in Go and TS. - The pg-delta Deno templates force the edge-runtime worker to exit by throwing on both success and failure, and both runners suppress any non-zero exit whose stderr contains "main worker has been destroyed" — so a script crash was indistinguishable from an empty diff. The catch blocks now print a PGDELTA_SCRIPT_ERROR sentinel and both runners treat it as a hard failure that surfaces the collected stderr. - alpha.32 also moved plan statements into execution-aware `units`; the diff template's `result?.plan.statements ?? []` silently yielded an empty diff. Use `flattenPlanStatements(result.plan)` instead. - The TS shell ran `SET SESSION ROLE postgres` once per session, but `PgClient.make` leaves node-postgres' default 10s idle timeout, so the pool silently replaced the stepped-down connection during the long shadow diff and the final `CREATE SCHEMA supabase_migrations` executed as the bare login role (42501). The primary connection now uses a self-managed pg.Pool (`PgClient.fromPool`) with idle reaping disabled and a pg-pool `verify` hook that re-runs the step-down on every new physical connection, matching Go's per-connection `AfterConnect`. Verified end-to-end against staging: initial and incremental `db pull --linked` produce correct migrations (including FDW server and user mapping without credential leak) and update the remote migration history as the stepped-down role. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@d72d355b4705d5b3199e12e48ff4f741ab1770dcPreview package for commit |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ffa72156e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // pg-delta >= 1.0.0-alpha.32 groups plan statements into execution-aware | ||
| // `units` (+ `sessionStatements`); the flat `plan.statements` field no longer | ||
| // exists, so reading it would silently yield an empty diff. | ||
| let statements = result ? flattenPlanStatements(result.plan) : []; |
There was a problem hiding this comment.
When pg-delta emits a plan containing non-transactional or commit-boundary units (for example ALTER TYPE ... ADD VALUE followed by a statement that uses the new value, or subscription DDL), flattening here discards the units transaction boundaries and writes one plain statement list. Generated migrations are then applied by the CLI through the migration batch path (apps/cli-go/pkg/migration/file.go / legacy-migration-apply.ts), which wraps the file transactionally unless explicit transaction control is present, so these newly-supported alpha.32 plans can still fail when users run db push/reset on the generated migration. Use the transaction-aware plan rendering (or otherwise preserve unit BEGIN/COMMIT/standalone boundaries) instead of only flattenPlanStatements for migration SQL.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4407659: the diff template now emits renderPlanFiles output (one entry per execution-aware unit, includeTransactions: false since both appliers already run each migration file in its own transaction), and db pull writes one ordered migration file per unit with every version recorded in the history. Verified against staging: ALTER TYPE ... ADD VALUE + a table defaulting to the new value pulls as two ordered files (schema_changes, after_enum_values), so the enum value commits before its first use. db diff/declarative sync join the rendered units into one script with boundary header comments.
…diff output by default Follow-up to the PR review: flattening the pg-delta plan discarded its transaction-boundary units, so plans like `ALTER TYPE ... ADD VALUE` followed by a statement using the new value produced a single migration that fails on `db push`/`reset` (both CLIs apply a migration file as one transaction). - The diff template now emits a JSON envelope of `renderPlanFiles` output (one entry per execution-aware unit, `includeTransactions: false` since the appliers provide per-file transactionality). - `db pull` writes one ordered migration per unit — a single-unit plan keeps today's `<ts>_<name>.sql` name; multi-unit plans get strictly increasing timestamps and a `_<unit>` suffix — and records every version in the migration history. - `db diff` and declarative sync join the rendered units into one script with boundary header comments (unchanged single-file behavior). - Diff/pull SQL is now formatted by default with the same settings as the declarative export (`maxWidth: 180`, uppercase keywords, formatter defaults); `[experimental.pgdelta] format_options` still merges on top, and `format_options = "null"` opts out entirely. - pg-delta routing in `DiffDatabase` goes through a stubable seam (`diffPgDeltaRefDetailed`) so the pgtest-based diff suites keep their injection point. Verified against staging: an enum `ADD VALUE` plus a table defaulting to the new value pulls as two ordered migration files (`schema_changes`, `after_enum_values`) with both versions recorded; a plain schema pull stays a single formatted file. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4407659e51
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…igrations in machine output Review follow-ups on the pg-delta db pull changes: - The primary connection's pg.Pool was created before its acquireRelease finalizer, so a failed or timed-out SELECT 1 probe (bad primary host in the multi-host fallback chain, black-holed host) leaked the pool and its in-flight dial. Pool construction is now the acquireRelease resource itself (legacyAcquireProbedPool), and each fallback attempt runs in its own scope forked from the session scope so a failed attempt's pool closes before the next host is dialed. - The structured `db pull` result now carries `schemaFiles` with every written plan-unit migration path in order; `schemaWritten` stays the first path for released consumers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70f171722b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if debugCapture != nil { | ||
| debugCapture.Stderr = result.Stderr | ||
| } | ||
| return DatabaseDiff{SQL: joinPgDeltaFiles(result.Files), Files: result.Files, Debug: debugCapture}, nil |
There was a problem hiding this comment.
When pg-delta produces multiple execution units, such as ALTER TYPE ... ADD VALUE followed by a statement that uses the new enum value, joining them here collapses the required commit boundary. Fresh evidence after the earlier fix is that db pull now consumes Files, but db diff --file still reads only DatabaseDiff.SQL in Run (lines 33-42) and writes one migration; the TypeScript mirror likewise returns result.sql in diff.handler.ts and writes one file. That file is later applied transactionally by Go's MigrationFile.ExecBatch or the TS migration batch, so db reset/db push fails on the generated migration. Materialize the units separately for --file, or render executable transaction boundaries instead of only joining their SQL.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 555063e: db diff -f (and the TS --output write) now materializes one migration file per pg-delta plan unit, reusing db pull's naming/timestamp scheme via the hoisted diff.WritePgDeltaMigrations — single-unit plans and the migra/pgadmin engines keep the byte-identical single-file write, and stdout mode keeps the joined review script with boundary comments. The TS machine payload adds a plural files field alongside the existing file. Covered by new Go TestSaveDiff (single/multi/no-change/stdout) and a TS multi-unit diff integration test.
| for (const entry of resolved) { | ||
| const content = yield* fs.readFileString(entry.migrationPath); | ||
| const statements = legacySplitAndTrim(content); | ||
| yield* session.query(UPSERT_MIGRATION_VERSION, [entry.version, entry.name, statements]); |
There was a problem hiding this comment.
When a pg-delta pull produces multiple files and a later file read or history UPSERT fails—for example because the connection drops during the second query—the earlier session.query calls have already committed independently. The command then returns an error with only part of the new migration sequence recorded remotely, while every file remains local, so the next pull reports a migration-history mismatch and requires manual repair. Go's repair.UpdateMigrationTable queues all versions in one SendBatch, which is implicitly transactional; wrap the TS UPSERT loop in one transaction and roll it back on any failure.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 555063e: the TS pull's version UPSERTs now run inside one explicit BEGIN … COMMIT with ROLLBACK on any failure (including a migration-file read that fails after BEGIN; a rollback failure can't mask the original error), mirroring Go's single implicitly-transactional pgx.Batch. legacyCreateMigrationTable stays sequenced before the transaction exactly like Go's CreateMigrationTable before the upsert batch. New integration tests assert BEGIN+N upserts+COMMIT on success and BEGIN/ROLLBACK with no COMMIT when the second upsert fails.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 797c43bf9f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| single := len(files) == 1 | ||
| written := make([]writtenMigration, 0, len(files)) | ||
| for i, file := range files { | ||
| version := utils.GetVersionTimestamp(base.Add(time.Duration(i) * time.Second)) |
There was a problem hiding this comment.
When a plan has multiple units, this assigns versions later than the current wall clock. If the user immediately runs migration new or another multi-unit db pull before those added seconds elapse, the new command can reuse one of these versions or create a version ordered before the already-written units. In the repeated-pull case, the matching path is opened with O_TRUNC below and can overwrite the prior migration before the same history version is UPSERTed, corrupting both the local migration sequence and its recorded contents. Generate versions that are unique against existing migration files without advancing beyond the clock.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in d72d355, with one honest pushback on the prescription: generated versions are now collision-checked as a FULL set against existing migration files (base bumped forward until every filename is free, bounded at 60 attempts) and files open with O_CREATE|O_EXCL instead of O_TRUNC — so the actual harms (silent overwrite, duplicate version, corrupted sequence) are gone. We deliberately did NOT stop advancing beyond the clock: second-granularity versions cannot yield N distinct values in one instant, and backdating below the wall clock could sort a new file before pre-existing migrations. The ≤N−1s future-dating is inherent and harmless once uniqueness is enforced (comment in code says the same).
| if err := utils.MkdirIfNotExistFS(fsys, utils.MigrationsDir); err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
When the optional migration name contains a path separator, such as db pull snapshots/remote --diff-engine pg-delta, new.GetMigrationPath preserves that nested path but this helper creates only the migrations root. The subsequent OpenFile therefore fails because the intermediate directory does not exist. The previous shared writer created filepath.Dir(path), and the TypeScript mirror still creates the directory derived from migrationPath, so either create each generated path's parent or reject such names consistently.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d72d355: WritePgDeltaMigrations creates filepath.Dir(path) for every generated path (matching utils.WriteFile semantics), so nested names work again for multi-unit writes. Covered by nested-name subtests in pgdelta_migrations_test.go and pgadmin_test.go. The pull-side history-repair glob still rejects nested names by design (pre-existing guard, unchanged).
…story atomically Review follow-ups on the pg-delta multi-file work: - `db diff -f` (and the TS `--output` write) now materializes one migration file per pg-delta plan unit, reusing db pull's naming and timestamp scheme, so a boundary-crossing plan saved as a migration no longer fails when `db push`/`reset` applies it as one transaction. Single-unit plans and the migra/pgadmin engines keep the byte-identical single-file write; stdout mode keeps the joined review script. The per-unit writer is hoisted from `pull` into `diff` (`WritePgDeltaMigrations`) and shared by both commands. - The TS pull's migration-history upserts now run in one explicit transaction with rollback on any failure, mirroring Go's single `pgx.Batch` (implicitly transactional): a mid-loop failure no longer leaves partial remote history that breaks the next pull's sync check. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 555063e4bf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| file = "declarative_sync" | ||
| } | ||
| if err := diff.SaveDiff(result.DiffSQL, file, fsys); err != nil { | ||
| if err := diff.SaveDiff(diff.DatabaseDiff{SQL: result.DiffSQL}, file, fsys); err != nil { |
There was a problem hiding this comment.
When a declarative delta needs multiple pg-delta units, such as adding an enum value and then using it, DiffDeclarativeToMigrations has already flattened those units into DiffSQL, and constructing DatabaseDiff with only SQL here prevents SaveDiff from using the new per-unit writer. The active smart-sync paths have the same loss (cmd/db_schema_declarative.go writes result.DiffSQL, while the TS sync handler writes result.diffSQL), so db schema declarative sync still creates and optionally applies one transactionally wrapped migration that can fail at the required commit boundary. Carry the plan files through the sync result and write/apply them as separate migrations.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This one is deliberately out of scope for this PR, as noted in the earlier replies on the flatten/units threads: declarative sync is experimental surface and carrying the plan units through DiffDeclarativeToMigrations/SyncResult (both CLIs, plus the apply step) is a self-contained change we're tracking as a follow-up rather than growing this PR further. The per-unit writer landed here (diff.WritePgDeltaMigrations) is what that follow-up will reuse.
| const migrationsDir = path.join(cliConfig.workdir, "supabase", "migrations"); | ||
| yield* legacyMakeDir(fs, migrationsDir).pipe( | ||
| Effect.mapError((cause) => new LegacyDbDiffWriteError({ message: cause.message })), |
There was a problem hiding this comment.
When db diff --file contains a path separator, such as --file snapshots/remote, legacyGetMigrationPath produces a nested path under supabase/migrations, but this now creates only the migrations root. The subsequent single- or multi-unit writeFileString therefore fails with a missing parent directory; the previous implementation explicitly created path.dirname(migrationPath). Create each generated path's parent or reject nested names consistently.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d72d355: the TS diff handler creates each written path's parent directory again (per-path legacyMakeDir(dirname) for the single-file branch; the multi-file branch goes through the new shared legacyWritePgDeltaMigrations, which does the same). Nested --file snapshots/remote covered by new single- and multi-unit integration tests.
| f, err := fsys.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0644) | ||
| if err != nil { | ||
| return nil, errors.Errorf("failed to open migration file: %w", err) |
There was a problem hiding this comment.
When writing a multi-unit plan and a later OpenFile or WriteString fails, for example because the disk fills during the second unit, this returns immediately but leaves all earlier migration files in place. For db pull, remote history is never updated after that error, so the next run sees those files as extra local migrations and refuses to continue; for db diff, the remaining files represent an incomplete plan that can be applied accidentally. Stage the complete set before publishing it, or remove every path created by this invocation on failure; the TS multi-file loops need the same cleanup.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d72d355: on any mid-loop open/write failure, both the Go writer and the shared TS writer best-effort remove every file already written by that invocation before surfacing the original error (removal failures never mask it). Covered by cleanup-on-failure tests on both sides (failing-Nth-write filesystems).
…sted names, and partial writes Review follow-ups on the per-unit migration writer: - Generated versions are collision-checked as a full set against existing migration files (base bumped forward until every filename is free, bounded), and files open with O_CREATE|O_EXCL instead of O_TRUNC, so a rapid successive pull/diff can never silently overwrite a migration or mint a duplicate version. The base only moves forward: backdating could sort a new file before pre-existing migrations, and the ≤N−1s future-dating is inherent to second-granularity versions. - The writer creates each generated path's parent directory again, fixing the nested-name regression (`db diff -f snapshots/remote`) in both Go and TS; the TS logic is hoisted into a shared `legacyWritePgDeltaMigrations` used by diff and pull. - A mid-loop open/write failure now removes every file this invocation already wrote (best-effort, never masking the original error), so a partial multi-file plan can't strand extra local migrations that break the next pull's history sync. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
Summary
Remote
db pull --linkedwith the pg-delta engine reported "No schema changes found" on every hosted project, and the migra fallback failed at the final migration-history write withpermission denied for database postgres(Slack report, #5826). Three stacked causes, all addressed here:1. pg-delta required superuser to extract (fixed upstream, version bump here). Up to alpha.31, extraction read
pg_catalog.pg_user_mapping(superuser-only), so extracting as the tempcli_login_postgresrole failed with SQLSTATE 42501 on every hosted project. It only worked locally because local connections usesupabase_admin.@supabase/pg-delta@1.0.0-alpha.32reads the world-readablepg_user_mappingsview instead; this PR bumps the default pinned version (GoDefaultPgDeltaNpmVersion+ TSLEGACY_DEFAULT_PG_DELTA_NPM_VERSION).2. pg-delta script crashes were swallowed as an empty diff. The Deno templates force the edge-runtime worker to exit by throwing on both the success and failure paths, and both runners (Go
RunEdgeRuntimeScript, TSlegacy-edge-runtime-script.layer.ts) suppress any non-zero exit whose stderr contains "main worker has been destroyed" — making a crash indistinguishable from a genuinely empty diff. The template catch blocks now print aPGDELTA_SCRIPT_ERRORsentinel to stderr, and both runners treat its presence as a hard failure that surfaces the collected stderr (so users see the real error instead of "No schema changes found").3. alpha.32 changed the plan API. Plan statements moved into execution-aware
units(+sessionStatements); the diff template'sresult?.plan.statements ?? []silently produced an empty diff. The template now usesflattenPlanStatements(result.plan). TS template embeds regenerated from the Go sources (byte-equality test unchanged).4. The TS role step-down was lost mid-command (the migra-path failure). The legacy shell ran
SET SESSION ROLE postgresonce per session, butPgClient.makeleaves node-postgres' default 10s idle timeout — during the minutes-long shadow diff the pool silently reaped and redialed the stepped-down connection, so the finalCREATE SCHEMA IF NOT EXISTS supabase_migrationsexecuted as the bare login role (42501). The primary connection now uses a self-managedpg.PoolviaPgClient.fromPoolwith idle reaping disabled (idleTimeoutMillis: 0,max: 1) and a pg-poolverifyhook that re-runs the step-down on every new physical connection — matching Go's per-connectionAfterConnect(connect.go:337-362).verifyruns before the checkout resolves, so it cannot race the caller's first query (apool.on("connect")client.query()hits node-postgres' concurrent-query deprecation, removed in pg@9).Verification against staging
link(login-role path, noSUPABASE_DB_PASSWORD) →db pull --linked("engine":"pg-delta"): migration contains the table with PK and grants, andRepaired migration history: [...] => appliedsucceeds — exercising the step-down after a long-idle diff, exactly where 2.109.1 failed.CREATE USER MAPPINGcarries no password option (CLI-1467 handling intact).Linked issue
Closes #5826
open-for-contributionlabel (or I'm a Supabase maintainer).Checklist
fix(cli): …).pnpm check:allandpnpm testpass for the workspace(s) I touched.🤖 Generated with Claude Code