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 2 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@4407659e51c3e210d15ec2227285e9e131fd9b0cPreview 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".
| // manual `formatSqlStatements` pass. | ||
| const files = result | ||
| ? renderPlanFiles(result.plan, { | ||
| includeTransactions: false, |
There was a problem hiding this comment.
When pg-delta emits a transactionMode: "none" unit (for example CREATE INDEX CONCURRENTLY; the new pull tests include this case), setting includeTransactions: false writes only the raw statement. The generated migration is later applied by Go's MigrationFile.ExecBatch implicit transaction and by the TS applier's normal BEGIN/COMMIT path, so the standalone unit still runs inside a transaction and fails with SQLSTATE 25001. Fresh evidence in this revision is that the unit's transactionMode is serialized only as metadata and is never used when writing or applying the file; keep transaction-aware rendering or otherwise mark non-transactional units so the applier runs them standalone.
Useful? React with 👍 / 👎.
| }); | ||
| ) => { | ||
| const acquire = Effect.gen(function* () { | ||
| const pool = new Pg.Pool( |
There was a problem hiding this comment.
When a dial attempt fails or times out before pool.query("SELECT 1") succeeds (for example a bad primary host before trying fallbacks, or a black-holed host), the acquireRelease finalizer is never installed because the Pg.Pool was created outside the acquired effect. That leaves the failed pool open while the fallback chain continues or the command returns, which can keep sockets/timers alive and hang the CLI or leak connection attempts. Ensure pool.end() also runs on probe failure/timeout, not only after a successful probe.
Useful? React with 👍 / 👎.
| yield* output.success("Schema pulled.", { | ||
| declarative: false, | ||
| schemaWritten: migrationPath, | ||
| schemaWritten: writtenMigrations[0]?.path ?? migrationPath, |
There was a problem hiding this comment.
When db pull --diff-engine pg-delta --output-format json writes multiple plan-unit files, the structured result reports only the first path even though writtenMigrations contains every file and the history repair records all versions. Automation consuming the JSON payload cannot discover or review the additional migrations that were created, so the machine-readable output becomes inaccurate exactly for the new multi-file workflow. Return the full list (or otherwise expose all paths) instead of truncating to index 0.
Useful? React with 👍 / 👎.
1 participant
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