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 3 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@70f171722b07dfdbd431252454f76d38118aadd4Preview 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 👍 / 👎.
| 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 👍 / 👎.
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