diff --git a/packages/cli/README.md b/packages/cli/README.md index 9f5934e..8870125 100644 --- a/packages/cli/README.md +++ b/packages/cli/README.md @@ -125,6 +125,7 @@ Useful `diff` flags: | `--allow-destructive` | Also apply drops and deletes | | `--apply-posture` | Also apply the target-only checks' fixes | | `--no-transaction` | Apply fix by fix, keeping what works | +| `--no-snapshot` | Read each query on its own. By default each database is read as of one instant, so a change made while the scan runs is seen by all of it or none; as with `pg_dump`, what it reads stays locked until the scan ends | | `--json` / `--ci` | Machine-readable output; CI annotations and exit codes | ## Applying fixes safely @@ -197,7 +198,9 @@ the output. - **Targets.** Restore runs as one transaction and expects an empty database. `--force` clears the snapshot's own schemas first (never Supabase's) and puts back the triggers and policies outside them that depended on what it - cleared. + cleared. A restore or apply that looks likely to need more locks than the + target allows in one transaction says so before it starts; + `--no-transaction` commits as it goes instead. - **Plain PostgreSQL.** Restoring into plain PostgreSQL works. What can't exist there is skipped and named: a foreign key to `auth.users`, a webhook, an extension the server doesn't ship, and everything that needs it. diff --git a/packages/cli/src/checks/extensions.ts b/packages/cli/src/checks/extensions.ts index 4c791a4..0af2448 100644 --- a/packages/cli/src/checks/extensions.ts +++ b/packages/cli/src/checks/extensions.ts @@ -18,12 +18,25 @@ export class ExtensionsCheck extends Check { } async scan(ctx: CheckContext): Promise { - const [source, target] = await Promise.all([ + const [source, target, reachable] = await Promise.all([ this.fetchExtensions(ctx.source.dbUrl), this.fetchExtensions(ctx.target.dbUrl), + this.fetchUpdates(ctx.target.dbUrl), ]) const ignored = new Set(ctx.config.ignoreExtensions ?? []) - return diffExtensions(source, target.filter(e => !ignored.has(e.name) || source.some(s => s.name === e.name))) + return diffExtensions(source, target.filter(e => !ignored.has(e.name) || source.some(s => s.name === e.name)), reachable) + } + + /** The versions each installed extension can be updated to on this server. */ + private async fetchUpdates(dbUrl: string): Promise> | undefined> { + try { + const rows = await this.queryFn(dbUrl, UPDATES_SQL) as unknown as Array<{ name: string; version: string }> + const out = new Map>() + for (const r of rows) out.set(r.name, (out.get(r.name) ?? new Set()).add(r.version)) + return out + } catch { + return undefined + } } private async fetchExtensions(dbUrl: string): Promise { @@ -40,7 +53,20 @@ const EXTENSIONS_SQL = ` ORDER BY e.extname ` -export function diffExtensions(source: PgExtension[], target: PgExtension[]): DriftIssue[] { +const UPDATES_SQL = ` + SELECT e.extname AS name, p.target AS version + FROM pg_extension e + CROSS JOIN LATERAL pg_extension_update_paths(e.extname) p + WHERE p.source = e.extversion AND p.path IS NOT NULL +` + +/** + * `reachable`: the versions each of the target's extensions can be updated + * to there. A version mismatch gets its ALTER EXTENSION only when the + * source's is one of them: PostgreSQL has no way down a version, and a + * server may not ship the one asked for, and either failed the whole apply. + */ +export function diffExtensions(source: PgExtension[], target: PgExtension[], reachable?: Map>): DriftIssue[] { const issues: DriftIssue[] = [] const sourceMap = new Map(source.map(e => [e.name, e])) const targetMap = new Map(target.map(e => [e.name, e])) @@ -56,7 +82,15 @@ export function diffExtensions(source: PgExtension[], target: PgExtension[]): Dr description: `Extension "${name}" (v${ext.version}) is enabled in source (schema: ${ext.schema}) but not in target.`, sourceValue: ext, sql: { - up: `CREATE EXTENSION IF NOT EXISTS ${quoteName(name)}${ext.schema !== 'public' ? ` SCHEMA ${quoteName(ext.schema)}` : ''};`, + // Into the schema it is in on the source, which plain PostgreSQL + // may not have — Supabase's `extensions`. Made first; if the server + // does not ship the extension, the fix is left out with it. A + // system schema (pg_cron's pg_catalog) is always there, and cannot + // be made. + up: ext.schema === 'public' + ? `CREATE EXTENSION IF NOT EXISTS ${quoteName(name)};` + : `${ext.schema.startsWith('pg_') ? '' : `CREATE SCHEMA IF NOT EXISTS ${quoteName(ext.schema)};\n`}` + + `CREATE EXTENSION IF NOT EXISTS ${quoteName(name)} SCHEMA ${quoteName(ext.schema)};`, down: `DROP EXTENSION IF EXISTS ${quoteName(name)};`, }, }) @@ -83,6 +117,7 @@ export function diffExtensions(source: PgExtension[], target: PgExtension[]): Dr if (!tgtExt) continue if (srcExt.version !== tgtExt.version) { + const updatable = !reachable || reachable.get(name)?.has(srcExt.version) issues.push({ id: `ext-version-${name}`, check: 'extensions', @@ -91,10 +126,14 @@ export function diffExtensions(source: PgExtension[], target: PgExtension[]): Dr description: `Extension "${name}" is v${srcExt.version} in source but v${tgtExt.version} in target.`, sourceValue: srcExt, targetValue: tgtExt, - sql: { - up: `ALTER EXTENSION ${quoteName(name)} UPDATE TO ${quoteLiteral(srcExt.version)};`, - down: `ALTER EXTENSION ${quoteName(name)} UPDATE TO ${quoteLiteral(tgtExt.version)};`, - }, + ...(updatable + ? { + sql: { + up: `ALTER EXTENSION ${quoteName(name)} UPDATE TO ${quoteLiteral(srcExt.version)};`, + down: `ALTER EXTENSION ${quoteName(name)} UPDATE TO ${quoteLiteral(tgtExt.version)};`, + }, + } + : { manualOnly: `the target cannot go from v${tgtExt.version} to v${srcExt.version}: its server has no update path there (an extension cannot be moved down a version)` }), }) } } diff --git a/packages/cli/src/checks/roles.ts b/packages/cli/src/checks/roles.ts index b6de8a1..36148a4 100644 --- a/packages/cli/src/checks/roles.ts +++ b/packages/cli/src/checks/roles.ts @@ -23,12 +23,12 @@ interface PgRole { * syntax; a sequence and a routine each need their own keyword, and a * routine its argument types, since overloads share a name. */ -type ObjectKind = 'table' | 'sequence' | 'function' | 'procedure' +type ObjectKind = 'table' | 'sequence' | 'function' | 'procedure' | 'schema' interface RoleGrant { grantee: string table_schema: string - /** The object's name; for a routine, without its arguments. */ + /** The object's name; for a routine, without its arguments; empty for a schema. */ table_name: string privilege_type: string is_grantable: boolean @@ -135,6 +135,34 @@ const grantsSql = (ignoreSchemas: string[]) => ` ORDER BY 1, 2, 3, 4 ` +/** + * Grants on schemas, read from `nspacl`: `USAGE` on a schema of its own is + * what lets `authenticated` reach the tables in it through the Data API at + * all. A schema an extension made is the extension's. + */ +const schemaGrantsSql = (ignoreSchemas: string[]) => ` + SELECT CASE WHEN a.grantee = 0 THEN 'PUBLIC' ELSE pg_get_userbyid(a.grantee) END AS grantee, + n.nspname AS table_schema, '' AS table_name, + a.privilege_type, a.is_grantable, 'schema' AS object_kind + FROM pg_namespace n + CROSS JOIN LATERAL aclexplode(n.nspacl) a + WHERE n.nspacl IS NOT NULL + AND a.grantee <> n.nspowner + AND n.nspname ${grantSchemaFilter(ignoreSchemas)} + AND n.nspname NOT LIKE 'pg\\_%' AND n.nspname <> 'information_schema' + AND NOT EXISTS (SELECT 1 FROM pg_depend d WHERE d.classid = 'pg_namespace'::regclass AND d.objid = n.oid AND d.deptype = 'e') + AND (a.grantee = 0 OR pg_get_userbyid(a.grantee) NOT IN (${quoted(PLATFORM_ROLES)})) + AND (a.grantee = 0 OR pg_get_userbyid(a.grantee) NOT LIKE 'pg_%') + ORDER BY 1, 2, 4 +` + +/** Schemas, as objects whose grants go with them. */ +const schemasSql = (ignoreSchemas: string[]) => ` + SELECT nspname AS table_schema, '' AS table_name, 'schema' AS object_kind + FROM pg_namespace + WHERE nspname ${grantSchemaFilter(ignoreSchemas)} +` + /** * Column-level grants, read from `attacl`. * @@ -241,6 +269,7 @@ type DefaultGrant = { /** The object a grant or relation names, with its kind and a routine's arguments. */ function objectKey(o: { table_schema: string; table_name: string; object_kind?: ObjectKind; args?: string | null }): string { + if (o.object_kind === 'schema') return `schema:${o.table_schema}` const routine = o.object_kind === 'function' || o.object_kind === 'procedure' return `${routine ? 'routine' : o.object_kind ?? 'table'}:${o.table_schema}.${o.table_name}${routine ? `(${o.args ?? ''})` : ''}` } @@ -351,6 +380,7 @@ export class RolesCheck extends Check { sourceRoles, targetRoles, sourceGrants, targetGrants, sourceColumns, targetColumns, sourceVersion, targetVersion, sourceRelations, targetRelations, targetDefaults, sourceRoutineGrants, targetRoutineGrants, sourceRoutines, targetRoutines, + sourceSchemaGrants, targetSchemaGrants, sourceSchemas, ] = await Promise.all([ this.queryFn(ctx.source.dbUrl, ROLES_SQL) as unknown as Promise, this.queryFn(ctx.target.dbUrl, ROLES_SQL) as unknown as Promise, @@ -367,16 +397,19 @@ export class RolesCheck extends Check { this.queryFn(ctx.target.dbUrl, functionGrantsSql(ignore)) as unknown as Promise, this.queryFn(ctx.source.dbUrl, routinesSql(ignore)) as unknown as Promise, this.queryFn(ctx.target.dbUrl, routinesSql(ignore)) as unknown as Promise, + this.queryFn(ctx.source.dbUrl, schemaGrantsSql(ignore)) as unknown as Promise, + this.queryFn(ctx.target.dbUrl, schemaGrantsSql(ignore)) as unknown as Promise, + this.queryFn(ctx.source.dbUrl, schemasSql(ignore)) as unknown as Promise, ]) - const sourceObjects = [...(sourceRelations ?? []), ...(sourceRoutines ?? [])] + const sourceObjects = [...(sourceRelations ?? []), ...(sourceRoutines ?? []), ...(sourceSchemas ?? [])] const targetObjects = [...(targetRelations ?? []), ...(targetRoutines ?? [])] - const sourceAll = [...(sourceGrants ?? []), ...(sourceRoutineGrants ?? [])] - // A routine or sequence only the target has is the schema check's finding, - // and its grants go with it. Every routine grants EXECUTE to PUBLIC unless + const sourceAll = [...(sourceGrants ?? []), ...(sourceRoutineGrants ?? []), ...(sourceSchemaGrants ?? [])] + // A routine, sequence or schema only the target has is the schema check's + // finding, and its grants go with it. Every routine grants EXECUTE to PUBLIC unless // told otherwise, so each one present on the target alone was reported // again as an extra grant. const inSource = new Set(sourceObjects.map(objectKey)) - const targetAll = [...(targetGrants ?? []), ...(targetRoutineGrants ?? [])] + const targetAll = [...(targetGrants ?? []), ...(targetRoutineGrants ?? []), ...(targetSchemaGrants ?? [])] .filter(g => !g.object_kind || g.object_kind === 'table' || inSource.has(objectKey(g))) const comparable = comparablePrivileges(sourceVersion, targetVersion) const byDefault = grantedByDefault(targetObjects, targetDefaults ?? []) @@ -500,6 +533,7 @@ function grantKey(g: RoleGrant): string { function objectSql(g: RoleGrant): string { const name = `${quoteName(g.table_schema)}.${quoteName(g.table_name)}` switch (g.object_kind) { + case 'schema': return `SCHEMA ${quoteName(g.table_schema)}` case 'sequence': return `SEQUENCE ${name}` case 'function': return `FUNCTION ${name}(${g.args ?? ''})` case 'procedure': return `PROCEDURE ${name}(${g.args ?? ''})` @@ -511,6 +545,7 @@ function objectSql(g: RoleGrant): string { function describeObject(g: RoleGrant): string { const name = `${g.table_schema}.${g.table_name}` switch (g.object_kind) { + case 'schema': return `schema ${g.table_schema}` case 'sequence': return `sequence ${name}` case 'function': return `function ${name}(${g.args ?? ''})` case 'procedure': return `procedure ${name}(${g.args ?? ''})` @@ -548,7 +583,12 @@ const revokeSql = (g: RoleGrant) => `REVOKE ${grantTarget(g)} FROM ${granteeSql( */ export function comparablePrivileges(sourceVersion: number, targetVersion: number): (g: RoleGrant) => boolean { const both = Math.min(sourceVersion || Infinity, targetVersion || Infinity) - return g => g.privilege_type !== 'MAINTAIN' || both >= 170000 + // PostgreSQL 15 stopped granting CREATE on `public` to everyone. Across that + // line the difference is each version's default, and copying the old one + // into a newer server would undo its hardening. + const acrossFifteen = Boolean(sourceVersion && targetVersion) && (sourceVersion < 150000) !== (targetVersion < 150000) + return g => (g.privilege_type !== 'MAINTAIN' || both >= 170000) + && !(acrossFifteen && g.object_kind === 'schema' && g.table_schema === 'public' && g.grantee === 'PUBLIC' && g.privilege_type === 'CREATE') } /** Grants that differ the same way, on one table (or column) to one role, as one finding. */ diff --git a/packages/cli/src/commands/diff.ts b/packages/cli/src/commands/diff.ts index 06e9410..048f602 100644 --- a/packages/cli/src/commands/diff.ts +++ b/packages/cli/src/commands/diff.ts @@ -176,6 +176,10 @@ export default class Diff extends BaseCommand { description: 'With --apply, print the fixes in the order they would run without executing any of them', default: false, }), + 'no-snapshot': Flags.boolean({ + description: 'Read each query on its own, rather than each database as of one instant (which holds what it reads locked until the scan ends, as pg_dump does)', + default: false, + }), 'no-transaction': Flags.boolean({ description: 'With --apply, run each fix on its own instead of rolling the whole set back on the first failure', aliases: ['continue-on-error'], @@ -241,12 +245,13 @@ export default class Diff extends BaseCommand { */ private renderApplyResult(result: PromoteResult, dryRun: boolean): void { const verb = dryRun ? 'Would apply' : 'Applied' + for (const warning of result.warnings ?? []) this.log(`${warn(warning)}\n`) if (result.applied.length > 0) { const heading = `${verb} ${result.applied.length} fix(es)${dryRun ? ', in this order' : ''}:` this.log(dryRun ? bold(heading) : ok(heading)) result.applied.forEach((stmt, i) => { const glyph = dryRun ? dim(`${i + 1}.`) : ok('✓') - this.log(` ${glyph} ${dim(`[${stmt.check}]`)} ${stmt.issueId}`) + this.log(` ${glyph} ${dim(`[${stmt.check}]`)} ${stmt.issueId}${stmt.note ? dim(` — ${stmt.note}`) : ''}`) // The SQL itself only in a dry run: it is the thing being reviewed, // and printing it after the fact would just repeat --detail. if (dryRun && stmt.sql) this.log(` ${dim(stmt.sql.replace(/\s*\n\s*/g, ' '))}`) @@ -395,7 +400,7 @@ export default class Diff extends BaseCommand { // the flag that performs one (issue #71). if (flags.apply || flags['dry-run']) { const onProgress = makeProgress() - const scanResult = await scan(registry, { config, checks, skip, tableFilter, onProgress, onDetail: makeDetail() }) + const scanResult = await scan(registry, { config, checks, skip, tableFilter, onProgress, onDetail: makeDetail(), snapshot: !flags['no-snapshot'] }) this.setCheckSummaries(scanResult.checks.map(c => ({ check: c.check, status: c.status, @@ -522,6 +527,7 @@ export default class Diff extends BaseCommand { const result = await promote({ dbUrl: targetEnv.dbUrl, + sourceUrl: config.environments[config.source!]?.dbUrl, scanResult, checks, dryRun, @@ -558,7 +564,7 @@ export default class Diff extends BaseCommand { // ── Scan mode (summary, detail, CI, or JSON) ──────────────────────────── const onProgress = makeProgress() - const result = await scan(registry, { config, checks, skip, tableFilter, onProgress, onDetail: makeDetail() }) + const result = await scan(registry, { config, checks, skip, tableFilter, onProgress, onDetail: makeDetail(), snapshot: !flags['no-snapshot'] }) this.setCheckSummaries(result.checks.map(c => ({ check: c.check, status: c.status, diff --git a/packages/cli/src/commands/restore.ts b/packages/cli/src/commands/restore.ts index 1b93638..4082118 100644 --- a/packages/cli/src/commands/restore.ts +++ b/packages/cli/src/commands/restore.ts @@ -252,6 +252,8 @@ export default class Restore extends BaseCommand { return } + for (const warning of result.warnings ?? []) this.log(`⚠ ${warning}\n`) + if (result.applied.length > 0) { this.log(`✅ Applied ${result.applied.length} operation(s):`) for (const op of result.applied.slice(0, 20)) { diff --git a/packages/cli/src/db.ts b/packages/cli/src/db.ts index 4dd6160..9c8fe1d 100644 --- a/packages/cli/src/db.ts +++ b/packages/cli/src/db.ts @@ -103,9 +103,95 @@ function poolFor(dbUrl: string): pg.Pool { return pool } +/** + * A scan reads each database as of one instant. + * + * The checks run many queries, concurrently, over seconds or minutes; a + * change made meanwhile — a migration, a policy someone adds — was seen by + * some and not others, and the report described a database that never + * existed. While a database is held (holdSnapshot), one connection keeps a + * REPEATABLE READ, READ ONLY transaction open that exports its snapshot, and + * each pooled connection joins it once, in a REPEATABLE READ transaction of + * its own — not read only, so a definition can still be rendered with a + * temporary object, inside a savepoint. A query that fails ends that + * connection's transaction; it is rolled back and joins again next time. + * + * As with pg_dump, what the reads lock stays locked until the scan ends, so + * releaseSnapshots() runs before anything is applied. + */ +const snapshots = new Map() +const joined = new WeakMap() + +/** Read `dbUrl` as of one instant until releaseSnapshots(). False, and nothing held, when the server will not. */ +export async function holdSnapshot(dbUrl: string): Promise { + if (snapshots.has(dbUrl)) return true + const anchor = newPgClient(dbUrl) + try { + await anchor.connect() + await anchor.query('BEGIN ISOLATION LEVEL REPEATABLE READ, READ ONLY') + const { rows: [row] } = await anchor.query<{ id: string }>('SELECT pg_export_snapshot() AS id') + snapshots.set(dbUrl, { id: row.id, anchor }) + return true + } catch { + await anchor.end().catch(() => undefined) + return false + } +} + +/** The snapshot `dbUrl` is read as of, while it is held. */ +export function heldSnapshot(dbUrl: string): string | undefined { + return snapshots.get(dbUrl)?.id +} + +/** + * Let every held database go. The pools reading them are closed with them: + * their connections sit in the snapshot's transactions. + */ +export async function releaseSnapshots(): Promise { + const held = [...snapshots.entries()] + snapshots.clear() + await Promise.all(held.map(async ([dbUrl, { anchor }]) => { + const pool = pools.get(dbUrl) + pools.delete(dbUrl) + await pool?.end().catch(() => undefined) + await anchor.end().catch(() => undefined) + })) +} + +/** A pooled connection, in the held snapshot's transaction when `dbUrl` is held. */ +async function connectionFor(dbUrl: string): Promise { + const client = await poolFor(dbUrl).connect() + const id = snapshots.get(dbUrl)?.id + if (id && joined.get(client) !== id) { + try { + await client.query(`BEGIN ISOLATION LEVEL REPEATABLE READ; SET TRANSACTION SNAPSHOT '${id.replace(/'/g, "''")}'`) + joined.set(client, id) + } catch (err) { + await client.query('ROLLBACK').catch(() => undefined) + client.release() + throw err + } + } + return client +} + export const pgQuery: QueryFn = async (dbUrl, sql, params) => { - const { rows } = await poolFor(dbUrl).query(sql, params) - return rows + if (!snapshots.has(dbUrl)) { + const { rows } = await poolFor(dbUrl).query(sql, params) + return rows + } + const client = await connectionFor(dbUrl) + try { + const { rows } = await client.query(sql, params) + return rows + } catch (err) { + // The transaction is over for this connection; it joins again next time. + await client.query('ROLLBACK').catch(() => undefined) + joined.delete(client) + throw err + } finally { + client.release() + } } /** @@ -117,13 +203,18 @@ export async function inRolledBackTransaction( dbUrl: string, fn: (client: pg.PoolClient) => Promise, ): Promise { - const client = await poolFor(dbUrl).connect() + const client = await connectionFor(dbUrl) + // Inside the held snapshot's transaction, a savepoint does what a + // transaction of its own would. + const [begin, undo] = joined.has(client) + ? ['SAVEPOINT sf_scratch', 'ROLLBACK TO SAVEPOINT sf_scratch'] + : ['BEGIN', 'ROLLBACK'] try { - await client.query('BEGIN') + await client.query(begin) try { return await fn(client) } finally { - await client.query('ROLLBACK').catch(() => undefined) + await client.query(undo).catch(() => undefined) } } finally { client.release() diff --git a/packages/cli/src/dbdiff.ts b/packages/cli/src/dbdiff.ts index fd24f8e..a14b1b6 100644 --- a/packages/cli/src/dbdiff.ts +++ b/packages/cli/src/dbdiff.ts @@ -8,6 +8,7 @@ import type { DriftIssue } from './types/drift' import { errMsg, friendlyDbError, DiagnosticError } from './utils/error' import { DBDIFF_EXEC_TIMEOUT_MS, DBDIFF_MAX_BUFFER } from './constants' import { schemaDiffIgnores } from './defaults' +import { PLATFORM_ROLES } from './checks/roles' import { parseStatements, names, relationName, type Statement } from './sql-ast.js' import { sqlSkeleton, policiesRemoved } from './sql-deps' import { escapeRegex } from './utils/strings' @@ -767,6 +768,20 @@ function unfoldedIssues( * " modified"; anything else by its leading statement, which for a * table change is its first ALTER TABLE rather than a view it stands aside. */ +/** + * Default privileges for a role the platform owns — supabase_admin's, the + * auth and storage admins' — are Supabase's, like their grants: they differ + * between a Supabase project and plain PostgreSQL or between Supabase + * versions, and `postgres` cannot change them, so offering to failed the + * whole apply. `postgres`'s own are the project's. + */ +function platformDefaultPrivileges(sql: string): boolean { + const role = /^\s*ALTER\s+DEFAULT\s+PRIVILEGES\s+FOR\s+ROLE\s+("(?:[^"]|"")+"|[\w$]+)/i.exec(sql)?.[1] + if (!role) return false + const name = role.startsWith('"') ? role.slice(1, -1).replace(/""/g, '"') : role.toLowerCase() + return name !== 'postgres' && PLATFORM_ROLES.includes(name) +} + function issuesFromUnits( upUnits: DbDiffUnit[], downUnits: DbDiffUnit[], @@ -775,7 +790,8 @@ function issuesFromUnits( ignoredSchemaTables?: Set, ): DriftIssue[] { const pairs = pairUnits(upUnits, downUnits).filter(({ up, down }) => - !(ignoreSchemas?.length && isCrossSchemaFkUnit(up, down, ignoreSchemas, ignoredSchemaTables))) + !(ignoreSchemas?.length && isCrossSchemaFkUnit(up, down, ignoreSchemas, ignoredSchemaTables)) + && !platformDefaultPrivileges(up.sql)) return pairs.map(({ up, down }, i) => { const upSql = up.sql diff --git a/packages/cli/src/defaults.ts b/packages/cli/src/defaults.ts index 581982a..d47bb5f 100644 --- a/packages/cli/src/defaults.ts +++ b/packages/cli/src/defaults.ts @@ -43,8 +43,10 @@ export const SUPABASE_PLATFORM_SCHEMAS = [ * schema is compared. */ export function schemaDiffIgnores(ignoreSchemas: readonly string[] = DEFAULT_IGNORE_SCHEMAS): string[] { + // pgmq's tables are its queues, which the queues check compares through + // pgmq itself. return [...new Set([...ignoreSchemas, ...SUPABASE_PLATFORM_SCHEMAS.filter(s => !DEFAULT_IGNORE_SCHEMAS.includes(s)), - '_realtime', '_analytics', 'pgsodium_masks'])] + '_realtime', '_analytics', 'pgsodium_masks', 'pgmq'])] } /** Hint text shown during `supaforge init` to guide users through the Supabase UI. */ diff --git a/packages/cli/src/lock-budget.ts b/packages/cli/src/lock-budget.ts new file mode 100644 index 0000000..9b9e696 --- /dev/null +++ b/packages/cli/src/lock-budget.ts @@ -0,0 +1,98 @@ +/** + * Whether one transaction can hold the locks a restore or an apply takes. + * + * Every relation a transaction creates, alters, drops or reads stays locked + * until it commits, and PostgreSQL keeps those locks in one table shared by + * the whole server: `max_locks_per_transaction` for each of its backend + * slots — connections, autovacuum and background workers, WAL senders — and + * prepared transactions, less what other sessions hold. A large + * restore in one transaction can run out part way, after minutes of work, + * and roll everything back with "out of shared memory". + * + * So the statements are counted first, and the run warns before it starts + * when they look likely to need more than the server has. It only warns: the + * table can grow into spare shared memory, so a count over the nominal size + * may still fit. + */ +import type pg from 'pg' +import { parseStatements } from './sql-ast.js' + +/** Constraints that build an index of their own. */ +const INDEXED = new Set(['CONSTR_PRIMARY', 'CONSTR_UNIQUE', 'CONSTR_EXCLUSION']) + +/** + * About how many lock-table slots running `statements` in one transaction + * holds: one per distinct relation named, one per index or identity + * sequence made along the way, and two per new table for its TOAST table + * and index. Generous on purpose; a statement that does not parse counts one. + */ +export function estimateLocks(statements: string[]): number { + const relations = new Set() + let made = 0 + for (const sql of statements) { + const parsed = parseStatements(sql) + if (!parsed) { + made += 1 + continue + } + for (const { kind, node } of parsed) { + if (kind === 'CreateStmt') made += 2 + if (kind === 'IndexStmt') made += 1 + walk(node, (key, value) => { + // A relation: wrapped as `RangeVar` in a list, bare in a field of its own. + if (typeof value.relname === 'string') relations.add(`${value.schemaname ?? ''}.${value.relname}`.toLowerCase()) + if (key === 'Constraint' && (INDEXED.has(value.contype) || value.contype === 'CONSTR_IDENTITY')) made += 1 + }) + } + } + return relations.size + made +} + +function walk(node: unknown, visit: (key: string, value: any) => void): void { + if (Array.isArray(node)) { + for (const item of node) walk(item, visit) + } else if (node && typeof node === 'object') { + for (const [key, value] of Object.entries(node)) { + if (value && typeof value === 'object' && !Array.isArray(value)) visit(key, value) + walk(value, visit) + } + } +} + +export interface LockBudget { + /** The server's lock table, in slots. */ + capacity: number + /** Slots other sessions hold now. */ + inUse: number +} + +/** The target's lock table, and how much of it is taken. Undefined when it cannot be read. */ +export async function readLockBudget(client: Pick): Promise { + try { + const { rows: [row] } = await client.query<{ capacity: string; in_use: string }>(` + SELECT current_setting('max_locks_per_transaction')::bigint + * (current_setting('max_connections')::bigint + + coalesce(current_setting('autovacuum_worker_slots', true), current_setting('autovacuum_max_workers'))::bigint + 1 + + current_setting('max_worker_processes')::bigint + + current_setting('max_wal_senders')::bigint + + current_setting('max_prepared_transactions')::bigint) AS capacity, + (SELECT count(*) FROM pg_locks WHERE NOT fastpath) AS in_use`) + return { capacity: Number(row.capacity), inUse: Number(row.in_use) } + } catch { + return undefined + } +} + +/** + * A warning when `needed` slots look like more than the target has free, + * keeping a tenth of the table for everyone else; undefined when they fit. + */ +export function lockBudgetWarning(needed: number, budget: LockBudget | undefined, what: string): string | undefined { + if (!budget) return undefined + const free = budget.capacity - budget.inUse - Math.ceil(budget.capacity / 10) + if (needed <= free) return undefined + return `${what} takes about ${needed} locks in one transaction, and the target has room for about ${Math.max(free, 0)} ` + + `(max_locks_per_transaction for each connection slot, less what other sessions hold). It may fail with ` + + `"out of shared memory" and roll back. Raising max_locks_per_transaction on the target (a restart) makes room; ` + + `--no-transaction commits as it goes instead.` +} diff --git a/packages/cli/src/pg-errors.ts b/packages/cli/src/pg-errors.ts index 0b1fb69..ec30d7e 100644 --- a/packages/cli/src/pg-errors.ts +++ b/packages/cli/src/pg-errors.ts @@ -15,3 +15,11 @@ export function sqlState(err: unknown): string | undefined { const code = (err as { code?: unknown } | null)?.code return typeof code === 'string' ? code : undefined } + +/** + * How an older server refuses what a newer one wrote: a syntax it does not + * know (`UNIQUE NULLS NOT DISTINCT` before 15, a `VIRTUAL` column before 18), + * a feature it lacks, a parameter it does not have (`security_invoker` before + * 15). Taken as the target lacking it only when the target is older. + */ +export const OLDER_SERVER: ReadonlySet = new Set(['42601', '0A000', '22023']) diff --git a/packages/cli/src/promote.ts b/packages/cli/src/promote.ts index 8200327..d3c10d8 100644 --- a/packages/cli/src/promote.ts +++ b/packages/cli/src/promote.ts @@ -2,9 +2,10 @@ import pg from 'pg' import { newPgClient } from './db.js' import type { ScanResult, SyncAction } from './types/drift' import { errMsg } from './utils/error' -import { ABSENT_ON_TARGET, EXTENSION_UNAVAILABLE, UNDEFINED_COLUMN, sqlState } from './pg-errors.js' +import { ABSENT_ON_TARGET, EXTENSION_UNAVAILABLE, OLDER_SERVER, UNDEFINED_COLUMN, sqlState } from './pg-errors.js' import { destructiveReason } from './dbdiff' import { parseStatements } from './sql-ast' +import { estimateLocks, lockBudgetWarning, readLockBudget } from './lock-budget.js' import { orderStatements, referencedTables, createdPolicies, createsOnlyPolicies, @@ -21,6 +22,12 @@ export type FetchFn = (url: string, init?: RequestInit) => Promise export interface PromoteOptions { /** Target database connection string */ dbUrl: string + /** + * The source's connection string. Its PostgreSQL version tells a fix an + * older target cannot parse — a feature it does not have — from any other + * failure: that fix is left out, as one needing a missing extension is. + */ + sourceUrl?: string /** The scan result with SQL fixes to apply */ scanResult: ScanResult /** Only promote specific checks */ @@ -75,7 +82,8 @@ type PlannedAction = FixOrigin & { action: SyncAction } type PlannedSkip = FixOrigin & { reason: string } export interface PromoteResult { - applied: (FixOrigin & { sql?: string; action?: string })[] + /** `note`: why a fix that did nothing still counts — see revokesFromAbsent. */ + applied: (FixOrigin & { sql?: string; action?: string; note?: string })[] skipped: PlannedSkip[] errors: (FixOrigin & { error: string })[] /** @@ -84,6 +92,8 @@ export interface PromoteResult { * lists what is actually in the target now. */ rolledBack?: (FixOrigin & { sql?: string })[] + /** Said before the fixes ran: they may need more locks than the target has — see lock-budget. */ + warnings?: string[] } interface PlannedWork { @@ -468,13 +478,15 @@ async function executeSql( statements: PlannedSql[], result: PromoteResult, transactional: boolean, + sourceMajor?: number, ): Promise { if (statements.length === 0) return const client = newPgClient(dbUrl) await client.connect() try { - if (transactional) await runInTransaction(client, statements, result) + const olderTarget = sourceMajor !== undefined && (await majorOf(client)) < sourceMajor + if (transactional) await runInTransaction(client, statements, result, olderTarget) else await runIndependently(client, statements, result) } finally { await client.end() @@ -488,12 +500,30 @@ async function executeSql( * this way, so it rehearses what the apply will do rather than a stricter * script that stopped at the first fix the server could not hold. */ -export async function runLikeApply(dbUrl: string, statements: PlannedSql[]): Promise { +export async function runLikeApply(dbUrl: string, statements: PlannedSql[], sourceMajor?: number): Promise { const result: PromoteResult = { applied: [], skipped: [], errors: [] } - await executeSql(dbUrl, statements, result, true) + await executeSql(dbUrl, statements, result, true, sourceMajor) return result } +/** A server's major version, from a client or a connection string. */ +async function majorOf(client: Pick): Promise { + const { rows: [row] } = await client.query<{ v: string }>(`SELECT current_setting('server_version_num') AS v`) + return Math.floor(Number(row?.v ?? 0) / 10000) +} + +export async function serverMajor(dbUrl: string): Promise { + const client = newPgClient(dbUrl) + try { + await client.connect() + return await majorOf(client) + } catch { + return undefined + } finally { + await client.end().catch(() => {}) + } +} + /** One statement at a time: a failure is recorded and the rest still run. */ async function runIndependently( client: pg.Client, @@ -552,12 +582,16 @@ async function runInTransaction( client: pg.Client, statements: PlannedSql[], result: PromoteResult, + olderTarget = false, ): Promise { const additions = statements.filter(s => isEnumValueAddition(s.sql)) const rest = statements.filter(s => !isEnumValueAddition(s.sql)) - if (additions.length > 0 && !(await runBatch(client, additions, result))) return - await runBatch(client, rest, result) + const warning = lockBudgetWarning(estimateLocks(rest.map(s => s.sql)), await readLockBudget(client), 'This apply') + if (warning) result.warnings = [...(result.warnings ?? []), warning] + + if (additions.length > 0 && !(await runBatch(client, additions, result, olderTarget))) return + await runBatch(client, rest, result, olderTarget) } /** @@ -567,10 +601,14 @@ async function runInTransaction( * Only "does not exist" failures qualify, and for `CREATE EXTENSION`, an * extension the server does not ship. */ -export function unmetDependency(sql: string, err: unknown): string | undefined { +export function unmetDependency(sql: string, err: unknown, olderTarget = false): string | undefined { const code = sqlState(err) if (!code) return undefined - if (/^\s*CREATE\s+EXTENSION\b/i.test(sqlSkeleton(sql))) { + if (olderTarget && OLDER_SERVER.has(code)) { + return "the target's PostgreSQL is older than the source's and does not support it" + } + // Anywhere in the fix: one into a schema of its own makes the schema first. + if (/(?:^|;)\s*CREATE\s+EXTENSION\b/i.test(sqlSkeleton(sql))) { return EXTENSION_UNAVAILABLE.has(code) ? 'this server does not ship it' : undefined } if (ABSENT_ON_TARGET.has(code) || code === UNDEFINED_COLUMN) { @@ -579,11 +617,33 @@ export function unmetDependency(sql: string, err: unknown): string | undefined { return undefined } +/** + * Whether `sql` only revokes privileges on one object, so that object being + * absent leaves nothing to do. Asked only of a fix that failed because + * something does not exist, once retrying it after the others no longer + * helps — a view dropped and created again by the same apply takes its + * revoke after the creation. + */ +export function revokesFromAbsent(sql: string): boolean { + const statements = parseStatements(sql) + if (!statements?.length) return false + const targets = new Set() + for (const { kind, node } of statements) { + if (kind !== 'GrantStmt' || node.is_grant || node.targtype !== 'ACL_TARGET_OBJECT') return false + if ((node.objects ?? []).length !== 1) return false + // Where in the text each was written is not which object it is. + targets.add(JSON.stringify([node.objtype, node.objects[0]], (key, value) => key === 'location' ? undefined : value)) + } + return targets.size === 1 +} + + /** One transaction; true when it committed. */ async function runBatch( client: pg.Client, statements: PlannedSql[], result: PromoteResult, + olderTarget = false, ): Promise { if (statements.length === 0) return true const done: PromoteResult['applied'] = [] @@ -603,7 +663,7 @@ async function runBatch( await client.query('RELEASE SAVEPOINT sf_fix') done.push({ check: stmt.check, issueId: stmt.issueId, sql: stmt.sql }) } catch (err) { - const why = unmetDependency(stmt.sql, err) + const why = unmetDependency(stmt.sql, err, olderTarget) if (why && await client.query('ROLLBACK TO SAVEPOINT sf_fix').then(() => true, () => false)) { unmet.push({ stmt, err, why }) continue @@ -618,6 +678,14 @@ async function runBatch( pending = unmet.map(u => u.stmt) } + // A revoke on an object the target no longer has — another fix in this + // apply dropped it — has nothing left to take away: done, not left out. + const gone = unmet.filter(u => revokesFromAbsent(u.stmt.sql)) + unmet = unmet.filter(u => !gone.includes(u)) + for (const { stmt } of gone) { + done.push({ check: stmt.check, issueId: stmt.issueId, sql: stmt.sql, note: 'nothing to revoke: the object is not on the target' }) + } + await client.query('COMMIT') result.applied.push(...done) for (const { stmt, err, why } of unmet) { @@ -697,7 +765,8 @@ export async function promote(options: PromoteOptions): Promise { return result } - await executeSql(dbUrl, sqlStatements, result, transactional) + await executeSql(dbUrl, sqlStatements, result, transactional, + options.sourceUrl ? await serverMajor(options.sourceUrl) : undefined) // A rolled-back batch leaves the target untouched, so the API calls that // would have gone with it must not fire either — they are not transactional diff --git a/packages/cli/src/prove.ts b/packages/cli/src/prove.ts index 842d10b..dad7256 100644 --- a/packages/cli/src/prove.ts +++ b/packages/cli/src/prove.ts @@ -292,7 +292,7 @@ export async function proveConvergence(opts: { // The migration under test. let notApplied: ProofResult['notApplied'] if (opts.fixes) { - const replayed = await runLikeApply(cloneUrl, opts.fixes) + const replayed = await runLikeApply(cloneUrl, opts.fixes, sourceMajor) const unmet = replayed.errors.filter(e => / — not applied: /.test(e.error)) const failed = replayed.errors.filter(e => !unmet.includes(e)) if (failed.length > 0) { diff --git a/packages/cli/src/restore.ts b/packages/cli/src/restore.ts index cbb48eb..1db0962 100644 --- a/packages/cli/src/restore.ts +++ b/packages/cli/src/restore.ts @@ -3,6 +3,7 @@ import { constants } from 'node:fs' import { join } from 'node:path' import pg from 'pg' import { newPgClient } from './db.js' +import { estimateLocks, lockBudgetWarning, readLockBudget } from './lock-budget.js' import type { MigrationFile, SnapshotManifest } from './types/config' import { loadMigrations } from './migration' import { MIGRATIONS_TABLE } from './constants.js' @@ -16,7 +17,7 @@ import { dropUnsupportedSetStatements, knownParameters } from './prove' import { splitSqlStatements, isCommentOnly, isPsqlMetaCommand, stripPsqlMetaCommands } from './utils/sql-split' import { quoteIdent, quoteLiteral, quoteName } from './utils/sql.js' import { sqlSkeleton, statementSubject, rolesNamedBy } from './sql-deps.js' -import { ABSENT_ON_TARGET, EXTENSION_UNAVAILABLE } from './pg-errors.js' +import { ABSENT_ON_TARGET, EXTENSION_UNAVAILABLE, OLDER_SERVER } from './pg-errors.js' import { dataTablesInOrder, restoreBuckets, restoreTableRows } from './restore-data.js' import { replaceableSchemas, findExternalDependents, dropSchemaContents, recreateExternalDependents, @@ -83,6 +84,32 @@ export interface RestoreResult { * most misleading thing this command could tell anyone (issue #95). */ rolledBack?: { type: 'sql' | 'api'; label: string }[] + /** Said before the restore started: it may need more locks than the target has — see lock-budget. */ + warnings?: string[] +} + +/** Whether the snapshot's server, as pg_dump's header names it, is a newer major version than the target's. */ +async function snapshotIsNewer(snapshotDir: string, client: Pick): Promise { + const head = (await readFile(join(snapshotDir, 'schema.sql'), 'utf-8').catch(() => '')).slice(0, 2000) + const dumped = /Dumped from database version (\d+)/.exec(head)?.[1] + if (!dumped) return false + const { rows: [row] } = await client.query<{ v: string }>(`SELECT current_setting('server_version_num') AS v`) + return Number(dumped) > Math.floor(Number(row?.v ?? 0) / 10000) +} + +/** About how many locks replaying the snapshot's SQL layers in one transaction holds. */ +async function locksForSnapshot( + snapshotDir: string, + layers: readonly string[], + manifest: Awaited>, +): Promise { + const statements: string[] = [] + for (const layer of layers) { + if (!manifest.layers[layer === 'storage-policies' ? 'storage' : layer]?.captured) continue + const raw = await readFile(join(snapshotDir, layerRestoreFile(layer)), 'utf-8').catch(() => '') + statements.push(...extractExecutableStatements(raw)) + } + return estimateLocks(statements) } // ─── Safety Check ──────────────────────────────────────────────────────────── @@ -131,8 +158,20 @@ export async function restoreFromSnapshot(options: RestoreOptions): Promise() @@ -260,7 +299,7 @@ export async function restoreFromSnapshot(options: RestoreOptions): Promise, missing: ReadonlySet = new Set(), leftOut: ReadonlyMap = new Map(), + olderTarget = false, ): string | undefined { const code = (err as { code?: string } | null)?.code if (!code) return undefined const skeleton = sqlSkeleton(sql) + // A snapshot of a newer server holds what an older one has no syntax or + // feature for: a VIRTUAL generated column before 18, UNIQUE NULLS NOT + // DISTINCT before 15. Left out and named, as a missing extension is. + if (olderTarget && OLDER_SERVER.has(code)) { + return `this server's PostgreSQL is older than the snapshot's and does not support it: ${errMsg(err)}` + } + if (/^\s*CREATE\s+EXTENSION\b/i.test(skeleton)) { return EXTENSION_UNAVAILABLE.has(code) ? `extension not available on this server: ${errMsg(err)}` @@ -873,6 +920,7 @@ async function applyStatement( result: RestoreResult, missing: Set, leftOut: Map = new Map(), + olderTarget = false, ): Promise { if (transactional) await client.query('SAVEPOINT sf_restore_statement') try { @@ -894,7 +942,7 @@ async function applyStatement( if (transactional) await client.query('ROLLBACK TO SAVEPOINT sf_restore_statement') const { rows } = await client.query<{ nspname: string }>('SELECT nspname FROM pg_namespace') const targetSchemas = new Set(rows.map(r => r.nspname)) - const tolerated = tolerableFailure(sql, err, targetSchemas, missing, leftOut) + const tolerated = tolerableFailure(sql, err, targetSchemas, missing, leftOut, olderTarget) if (tolerated) { result.skipped.push({ type: 'sql', label: summarizeStatement(sql), reason: tolerated }) // What this would have created, and why it is not there, for whatever diff --git a/packages/cli/src/scanner.ts b/packages/cli/src/scanner.ts index b85bf36..ea29ede 100644 --- a/packages/cli/src/scanner.ts +++ b/packages/cli/src/scanner.ts @@ -1,3 +1,4 @@ +import { holdSnapshot, releaseSnapshots } from './db.js' import { policyOnlyKeys, sqlSkeleton } from './sql-deps.js' import { splitSqlStatements } from './utils/sql-split.js' import type { HookBus } from './hooks' @@ -32,6 +33,11 @@ export interface ScanOptions { * Merged with the config's own `checks.tables` / `checks.excludeTables`. */ tableFilter?: TableFilter + /** + * Read each database as of one instant (see holdSnapshot). On by default; + * what the scan reads stays locked until it ends, as with pg_dump. + */ + snapshot?: boolean } /** @@ -203,9 +209,17 @@ export async function scan( } } - await Promise.all( - Array.from({ length: Math.min(checkConcurrency(), total) }, () => worker()), - ) + // Each side read as of one instant, and let go before anything is applied. + if (options.snapshot !== false) { + await Promise.all([source?.dbUrl, target?.dbUrl].filter(Boolean).map(url => holdSnapshot(url as string))) + } + try { + await Promise.all( + Array.from({ length: Math.min(checkConcurrency(), total) }, () => worker()), + ) + } finally { + if (options.snapshot !== false) await releaseSnapshots() + } foldDuplicatePolicyFindings(results) foldDuplicateExtensionFindings(results) diff --git a/packages/cli/src/snapshot.ts b/packages/cli/src/snapshot.ts index cee6fdb..2023a51 100644 --- a/packages/cli/src/snapshot.ts +++ b/packages/cli/src/snapshot.ts @@ -700,6 +700,20 @@ async function captureRoleGrants( AND (a.grantee = 0 OR pg_get_userbyid(a.grantee) NOT LIKE 'pg\\_%') AND n.nspname NOT IN (${excluded}) UNION ALL + -- Schemas: USAGE on a schema of its own is what lets a role reach its tables. + SELECT CASE WHEN a.grantee = 0 THEN 'PUBLIC' ELSE pg_get_userbyid(a.grantee) END, + n.nspname, '', NULL::text, a.privilege_type, a.is_grantable, 'SCHEMA', NULL::text + FROM pg_namespace n + CROSS JOIN LATERAL aclexplode(n.nspacl) a + WHERE n.nspacl IS NOT NULL AND a.grantee <> n.nspowner + AND n.nspname NOT IN (${excluded}) AND n.nspname NOT LIKE 'pg\\_%' AND n.nspname <> 'information_schema' + AND NOT EXISTS (SELECT 1 FROM pg_depend d WHERE d.classid = 'pg_namespace'::regclass AND d.objid = n.oid AND d.deptype = 'e') + AND (a.grantee = 0 OR pg_get_userbyid(a.grantee) NOT IN ( + 'postgres','supabase_admin','authenticator','supabase_auth_admin', + 'supabase_storage_admin','dashboard_user','pgbouncer','supavisor' + )) + AND (a.grantee = 0 OR pg_get_userbyid(a.grantee) NOT LIKE 'pg\\_%') + UNION ALL SELECT CASE WHEN a.grantee = 0 THEN 'PUBLIC' ELSE pg_get_userbyid(a.grantee) END, n.nspname, c.relname, att.attname, a.privilege_type, a.is_grantable, '', NULL::text FROM pg_attribute att @@ -713,7 +727,7 @@ async function captureRoleGrants( `) as unknown as Array<{ grantee: string; table_schema: string; table_name: string column_name: string | null; privilege_type: string; is_grantable: boolean - /** SEQUENCE, FUNCTION or PROCEDURE; empty for a table or view. */ + /** SCHEMA, SEQUENCE, FUNCTION or PROCEDURE; empty for a table or view. */ keyword: string /** A routine's identity arguments. */ args: string | null @@ -732,23 +746,84 @@ async function captureRoleGrants( } const statements = [...grouped.values()].map(([row, ...rest]) => `GRANT ${[row, ...rest].map(r => r.privilege_type + (r.column_name ? ` (${quoteName(r.column_name)})` : '')).join(', ')}` - + ` ON ${row.keyword ? `${row.keyword} ` : ''}${quoteName(row.table_schema)}.${quoteName(row.table_name)}` - + (row.args !== null && row.keyword !== 'SEQUENCE' && row.keyword ? `(${row.args})` : '') + + ` ON ${row.keyword === 'SCHEMA' ? `SCHEMA ${quoteName(row.table_schema)}` + : `${row.keyword ? `${row.keyword} ` : ''}${quoteName(row.table_schema)}.${quoteName(row.table_name)}`}` + + (row.args !== null && row.keyword !== 'SEQUENCE' && row.keyword !== 'SCHEMA' && row.keyword ? `(${row.args})` : '') // PUBLIC is a keyword; quoted, it names a role that does not exist. + ` TO ${row.grantee === 'PUBLIC' ? 'PUBLIC' : quoteName(row.grantee)}` + (row.is_grantable ? ' WITH GRANT OPTION' : '') + ';') - const output = statements.length > 0 - ? `-- SupaForge Role Grants Snapshot\n-- ${rows.length} grant(s)\n\n${statements.join('\n')}\n` + const defaults = await defaultPrivilegeStatements(dbUrl, queryFn, excluded) + const all = [...statements, ...defaults] + const output = all.length > 0 + ? `-- SupaForge Role Grants Snapshot\n-- ${rows.length} grant(s), ${defaults.length} default privilege statement(s)\n\n${all.join('\n')}\n` : '-- No role grants found\n' await writeFile(join(dir, file), output) - return { captured: true, file, itemCount: rows.length } + return { captured: true, file, itemCount: rows.length + defaults.length } } catch (err) { return { captured: false, file, itemCount: 0, error: errMsg(err) } } } +/** What `ALTER DEFAULT PRIVILEGES` calls each kind pg_default_acl records. */ +const DEFAULT_ACL_KINDS: Record = { r: 'TABLES', S: 'SEQUENCES', f: 'FUNCTIONS', T: 'TYPES', n: 'SCHEMAS' } + +/** + * The `ALTER DEFAULT PRIVILEGES` that make each role's defaults what they + * are: what its new tables, sequences, functions, types or schemas are + * granted. One for a single schema adds to the global one; the global one + * replaces PostgreSQL's built-in default for the kind, so it is the GRANTs + * and REVOKEs between the two. A platform role's defaults are the platform's. + */ +async function defaultPrivilegeStatements(dbUrl: string, queryFn: QueryFn, excludedSchemas: string): Promise { + const platform = `'postgres','supabase_admin','authenticator','supabase_auth_admin','supabase_storage_admin','dashboard_user','pgbouncer','supavisor'` + const rows = await queryFn(dbUrl, ` + WITH d AS ( + SELECT d.*, n.nspname FROM pg_default_acl d LEFT JOIN pg_namespace n ON n.oid = d.defaclnamespace + WHERE pg_get_userbyid(d.defaclrole) NOT IN ('supabase_admin','supabase_auth_admin','supabase_storage_admin','dashboard_user') + AND (n.nspname IS NULL OR n.nspname NOT IN (${excludedSchemas})) + ), have AS ( + SELECT d.oid, a.* FROM d CROSS JOIN LATERAL aclexplode(d.defaclacl) a + ), builtin AS ( + SELECT d.oid, b.* FROM d CROSS JOIN LATERAL aclexplode(acldefault(d.defaclobjtype, d.defaclrole)) b + WHERE d.defaclnamespace = 0 + ) + SELECT pg_get_userbyid(d.defaclrole) AS role, d.nspname AS schema, d.defaclobjtype::text AS kind, x.action, + CASE WHEN x.grantee = 0 THEN 'PUBLIC' ELSE pg_get_userbyid(x.grantee) END AS grantee, + x.privilege_type, x.is_grantable + FROM d JOIN ( + SELECT h.oid, 'GRANT' AS action, h.grantee, h.privilege_type, h.is_grantable FROM have h + WHERE NOT EXISTS (SELECT 1 FROM builtin b WHERE b.oid = h.oid AND b.grantee = h.grantee + AND b.privilege_type = h.privilege_type AND b.is_grantable = h.is_grantable) + UNION ALL + SELECT b.oid, 'REVOKE', b.grantee, b.privilege_type, b.is_grantable FROM builtin b + WHERE NOT EXISTS (SELECT 1 FROM have h WHERE h.oid = b.oid AND h.grantee = b.grantee + AND h.privilege_type = b.privilege_type AND h.is_grantable = b.is_grantable) + ) x ON x.oid = d.oid + WHERE (x.grantee = 0 OR pg_get_userbyid(x.grantee) NOT IN (${platform}) OR x.grantee = d.defaclrole) + AND (x.grantee = 0 OR pg_get_userbyid(x.grantee) NOT LIKE 'pg\\_%') + ORDER BY 1, 2 NULLS FIRST, 3, 4, 5, 6 + `) as unknown as Array<{ role: string; schema: string | null; kind: string; action: 'GRANT' | 'REVOKE' + grantee: string; privilege_type: string; is_grantable: boolean }> + + const grouped = new Map() + for (const row of rows) { + if (!DEFAULT_ACL_KINDS[row.kind]) continue + const key = JSON.stringify([row.role, row.schema, row.kind, row.action, row.grantee, row.action === 'GRANT' && row.is_grantable]) + grouped.set(key, [...(grouped.get(key) ?? []), row]) + } + const grantee = (g: string) => g === 'PUBLIC' ? 'PUBLIC' : quoteName(g) + return [...grouped.values()].map(([row, ...rest]) => { + const head = `ALTER DEFAULT PRIVILEGES FOR ROLE ${quoteName(row.role)}${row.schema ? ` IN SCHEMA ${quoteName(row.schema)}` : ''}` + const privileges = [row, ...rest].map(r => r.privilege_type).join(', ') + const on = DEFAULT_ACL_KINDS[row.kind] + return row.action === 'GRANT' + ? `${head} GRANT ${privileges} ON ${on} TO ${grantee(row.grantee)}${row.is_grantable ? ' WITH GRANT OPTION' : ''};` + : `${head} REVOKE ${privileges} ON ${on} FROM ${grantee(row.grantee)};` + }) +} + async function captureExtensions( dir: string, dbUrl: string, diff --git a/packages/cli/src/sql-deps.ts b/packages/cli/src/sql-deps.ts index 9e59e90..168b6e4 100644 --- a/packages/cli/src/sql-deps.ts +++ b/packages/cli/src/sql-deps.ts @@ -657,7 +657,13 @@ function rolesOf({ kind, node }: Statement): string[] { (kind === 'GrantStmt' && node.is_grant) ? node.grantees ?? [] : (kind === 'GrantRoleStmt' && node.is_grant) ? node.grantee_roles ?? [] : (kind === 'CreatePolicyStmt' || kind === 'AlterPolicyStmt') ? node.roles ?? [] - : [] + // The roles whose new objects it is about (FOR ROLE), and those it grants to. + : kind === 'AlterDefaultPrivilegesStmt' ? [ + ...((node.options ?? []) as Array>) + .filter(o => o.DefElem?.defname === 'roles').flatMap(o => o.DefElem.arg?.List?.items ?? []), + ...(node.action?.is_grant ? node.action.grantees ?? [] : []), + ] + : [] return specs .map(spec => spec.RoleSpec) // PUBLIC, CURRENT_USER and the like are not role names. diff --git a/packages/cli/src/state-diff.ts b/packages/cli/src/state-diff.ts index 9cbdca5..ccb4832 100644 --- a/packages/cli/src/state-diff.ts +++ b/packages/cli/src/state-diff.ts @@ -100,8 +100,11 @@ function compareTable(out: string[], subject: string, a: StateTable, b: StateTab compareKeyed(out, 'column', a.columns, b.columns, c => `${prefix}.${c.name}`, (o, s, x, y) => compareColumn(o, s, x, y)) + // A NOT NULL, a constraint from PostgreSQL 18, by the column it holds for: + // its name is a table's or column's old one after a rename, or one the + // server picked, and an older source has none to compare. compareKeyed(out, `constraint on ${prefix}`, a.constraints, b.constraints, - c => c.name, + c => c.type === 'not_null' ? c.definition : c.name, (o, s, x, y) => { compareField(o, s, 'definition', x.definition, y.definition) compareField(o, s, 'validated', x.validated, y.validated) diff --git a/packages/cli/test/checks/extensions.test.ts b/packages/cli/test/checks/extensions.test.ts index a6016a9..f6af96b 100644 --- a/packages/cli/test/checks/extensions.test.ts +++ b/packages/cli/test/checks/extensions.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect } from 'vitest' -import { ExtensionsCheck } from '../../src/checks/extensions.js' +import { ExtensionsCheck, diffExtensions } from '../../src/checks/extensions.js' import type { CheckContext } from '../../src/checks/base.js' import type { QueryFn } from '../../src/db.js' @@ -54,7 +54,8 @@ describe('ExtensionsCheck', () => { }) it('detects version mismatch', async () => { - const queryFn: QueryFn = async (dbUrl) => { + const queryFn: QueryFn = async (dbUrl, sql) => { + if (sql.includes('pg_extension_update_paths')) return [{ name: makeExt().name, version: '1.4' }] if (dbUrl.includes('source')) return [makeExt({ version: '1.4' })] return [makeExt({ version: '1.3' })] } @@ -103,3 +104,43 @@ describe('ExtensionsCheck', () => { expect(issues[0].sql?.up).not.toContain('SCHEMA') }) }) + +// The target's own server says which versions it can reach. PostgreSQL has +// no way down a version: ALTER EXTENSION pg_cron UPDATE TO '1.6' against a +// target on 1.6.4 failed, and with it the whole apply. +describe('ExtensionsCheck: versions the target cannot reach', () => { + const scan = (paths: Array<{ name: string; version: string }>) => new ExtensionsCheck(async (dbUrl, sql) => { + if (sql.includes('pg_extension_update_paths')) return paths as never + return [makeExt({ version: dbUrl.includes('source') ? '1.6' : '1.6.4' })] + }).scan(mockContext()) + + it('reports the mismatch with no fix, and why', async () => { + const [issue] = await scan([]) + expect(issue.title).toContain('version mismatch') + expect(issue.sql).toBeUndefined() + expect(issue.manualOnly).toContain('cannot go from v1.6.4 to v1.6') + }) + + it('still offers the update when the target can make it', async () => { + const [issue] = await scan([{ name: makeExt().name, version: '1.6' }]) + expect(issue.sql?.up).toContain("UPDATE TO '1.6'") + expect(issue.manualOnly).toBeUndefined() + }) +}) + +describe('diffExtensions: an extension in a schema of its own', () => { + it('makes the schema first, which plain PostgreSQL may not have', () => { + const [issue] = diffExtensions([{ name: 'pgjwt', version: '0.2.0', schema: 'extensions' }], []) + expect(issue.sql?.up).toBe('CREATE SCHEMA IF NOT EXISTS "extensions";\nCREATE EXTENSION IF NOT EXISTS "pgjwt" SCHEMA "extensions";') + }) + + it('makes no system schema, which is always there and cannot be made', () => { + const [issue] = diffExtensions([{ name: 'pg_cron', version: '1.6', schema: 'pg_catalog' }], []) + expect(issue.sql?.up).toBe('CREATE EXTENSION IF NOT EXISTS "pg_cron" SCHEMA "pg_catalog";') + }) + + it('needs no schema made in public', () => { + const [issue] = diffExtensions([{ name: 'pgcrypto', version: '1.3', schema: 'public' }], []) + expect(issue.sql?.up).toBe('CREATE EXTENSION IF NOT EXISTS "pgcrypto";') + }) +}) diff --git a/packages/cli/test/checks/roles.test.ts b/packages/cli/test/checks/roles.test.ts index 1e31cf9..de95c3f 100644 --- a/packages/cli/test/checks/roles.test.ts +++ b/packages/cli/test/checks/roles.test.ts @@ -136,7 +136,7 @@ describe('RolesCheck', () => { expect(grantIssue!.sql?.up).toContain('REVOKE INSERT') }) - it('runs all 15 queries (roles, grants, column grants, routine grants, version, tables and routines per side; the target\'s default grants)', async () => { + it('runs all 18 queries (roles, grants, column grants, routine grants, schema grants, version, tables and routines per side; the source\'s schemas; the target\'s default grants)', async () => { const calls: string[] = [] const queryFn: QueryFn = async (_dbUrl, sql) => { calls.push(sql) @@ -144,7 +144,8 @@ describe('RolesCheck', () => { } const check = new RolesCheck(queryFn) await check.scan(mockContext()) - expect(calls).toHaveLength(15) + expect(calls).toHaveLength(18) + expect(calls.filter(s => s.includes('n.nspacl'))).toHaveLength(2) expect(calls.filter(s => s.includes('proacl'))).toHaveLength(2) expect(calls.filter(s => s.includes('server_version_num'))).toHaveLength(2) const roleQueries = calls.filter(s => s.includes('rolsuper')) @@ -404,7 +405,7 @@ describe('a new table keeps the source\'s grants, not the target\'s defaults', ( return source ? [] : ['anon', 'authenticated'].flatMap(grantee => ALL.map(privilege_type => ({ table_schema: 'public', grantee, privilege_type, is_grantable: false }))) } - if (sql.includes('attacl') || sql.includes('rolsuper') || sql.includes('pg_proc')) return [] + if (sql.includes('attacl') || sql.includes('rolsuper') || sql.includes('pg_proc') || sql.includes("'schema' AS object_kind")) return [] if (sql.includes('server_version_num')) return [{ v: '150008' }] if (sql.includes('relacl')) { return source ? [ @@ -528,3 +529,55 @@ describe('grants on a routine only the target has', () => { expect(issues).toEqual([]) }) }) + +// USAGE on a schema of its own is what lets a role reach the tables in it at +// all; it was not compared, so a target without it read as matching. +describe('RolesCheck: grants on schemas', () => { + const schemaGrant = (overrides: Record = {}) => + makeGrant({ table_schema: 'app', table_name: '', privilege_type: 'USAGE', object_kind: 'schema', ...overrides }) + const scanWith = (source: unknown[], target: unknown[], schemas = ['app']) => new RolesCheck(async (dbUrl, sql) => { + if (sql.includes('pg_roles')) return [makeRole()] + if (sql.includes('n.nspacl')) return (dbUrl.includes('source') ? source : target) as never + if (sql.includes("'schema' AS object_kind") && dbUrl.includes('source')) { + return schemas.map(table_schema => ({ table_schema, table_name: '', object_kind: 'schema' })) as never + } + return [] + }).scan(mockContext()) + + it('reports a schema grant the target lacks, with the GRANT that makes it', async () => { + const issues = await scanWith([schemaGrant()], []) + const missing = issues.find(i => i.id.startsWith('roles-grant-missing')) + expect(missing?.title).toContain('USAGE ON schema app TO app_readonly') + expect(missing?.sql?.up).toBe('GRANT USAGE ON SCHEMA "app" TO "app_readonly";') + expect(missing?.sql?.down).toBe('REVOKE USAGE ON SCHEMA "app" FROM "app_readonly";') + }) + + it('reports one only the target has, and revokes it', async () => { + const issues = await scanWith([], [schemaGrant({ privilege_type: 'CREATE' })]) + const extra = issues.find(i => i.id.startsWith('roles-grant-extra')) + expect(extra?.sql?.up).toBe('REVOKE CREATE ON SCHEMA "app" FROM "app_readonly";') + }) + + it('leaves the grants on a schema only the target has to the schema check', async () => { + expect(await scanWith([], [schemaGrant({ table_schema: 'old' })])).toEqual([]) + }) + + it('reports nothing when both match', async () => { + expect(await scanWith([schemaGrant()], [schemaGrant()])).toEqual([]) + }) +}) + +describe('comparablePrivileges: CREATE on public across PostgreSQL 15', () => { + const g = { grantee: 'PUBLIC', table_schema: 'public', table_name: '', privilege_type: 'CREATE', is_grantable: false, object_kind: 'schema' as const } + + it('leaves out the default that changed, in either direction', () => { + expect(comparablePrivileges(140010, 170000)(g)).toBe(false) + expect(comparablePrivileges(170000, 140010)(g)).toBe(false) + }) + + it('still compares it between servers on the same side of the change', () => { + expect(comparablePrivileges(140010, 140012)(g)).toBe(true) + expect(comparablePrivileges(150008, 180000)(g)).toBe(true) + expect(comparablePrivileges(140010, 170000)({ ...g, table_schema: 'app' })).toBe(true) + }) +}) diff --git a/packages/cli/test/dbdiff-units.test.ts b/packages/cli/test/dbdiff-units.test.ts index a846e74..380a696 100644 --- a/packages/cli/test/dbdiff-units.test.ts +++ b/packages/cli/test/dbdiff-units.test.ts @@ -325,3 +325,21 @@ describe('dbdiffNotices', () => { .toEqual(['Operator objects are not compared (source 1, target 0: ===) — check them by hand']) }) }) + +// supabase_admin's default privileges are Supabase's, and postgres cannot +// change them: offered as a fix, they failed the whole apply. +describe('default privileges a platform role holds', () => { + const defaults = (role: string) => units(['AlterDefaultPrivileges', `${role}.public.tables`, + `ALTER DEFAULT PRIVILEGES FOR ROLE "${role}" IN SCHEMA "public" REVOKE SELECT ON TABLES FROM "anon";`]) + + it('are left out', () => { + expect(sqlToIssues({ up: defaults('supabase_admin'), down: defaults('supabase_admin') }, 'schema')).toEqual([]) + }) + + it('but postgres\'s, and any other role\'s, are the project\'s', () => { + for (const role of ['postgres', 'app_owner']) { + expect(sqlToIssues({ up: defaults(role), down: defaults(role) }, 'schema').map(i => i.title)) + .toEqual([`Default privileges differ: ${role} in public on tables`]) + } + }) +}) diff --git a/packages/cli/test/e2e/snapshot-reads.test.ts b/packages/cli/test/e2e/snapshot-reads.test.ts new file mode 100644 index 0000000..335f268 --- /dev/null +++ b/packages/cli/test/e2e/snapshot-reads.test.ts @@ -0,0 +1,62 @@ +/** + * A scan reads each database as of one instant, against a real server. + * + * A table made while the database is held is not seen by any read until it + * is let go; a read that fails, and a definition rendered with a temporary + * object, leave the reads after them working. + */ +import { describe, it, expect, beforeAll, afterAll, afterEach } from 'vitest' +import { PgHarness } from '../harness/PgHarness.js' +import { describeWithContainers } from '../harness/containers.js' +import { holdSnapshot, heldSnapshot, inRolledBackTransaction, pgQuery, releaseSnapshots } from '../../src/db.js' + +const describeE2E = describeWithContainers() + +describeE2E('e2e: reads as of one instant', () => { + let h: PgHarness + + beforeAll(async () => { + h = new PgHarness({ verbose: !!process.env.E2E_VERBOSE }) + await h.up() + }, 300_000) + + afterEach(async () => { await releaseSnapshots() }) + afterAll(async () => { await h?.down() }, 120_000) + + const count = async (url: string, name: string) => + Number((await pgQuery(url, 'SELECT count(*) AS n FROM pg_class WHERE relname = $1', [name]))[0].n) + + it('does not see a table made while the database is held, until it is let go', async () => { + const url = h.connectionString('source') + expect(await holdSnapshot(url)).toBe(true) + expect(heldSnapshot(url)).toBeTruthy() + await count(url, 'made_meanwhile') + + await h.applySql('source', 'CREATE TABLE public.made_meanwhile (id int);') + + // Several reads, on several pooled connections, all as of the instant held. + expect(await Promise.all([1, 2, 3, 4, 5, 6].map(() => count(url, 'made_meanwhile')))).toEqual([0, 0, 0, 0, 0, 0]) + + await releaseSnapshots() + expect(heldSnapshot(url)).toBeUndefined() + expect(await count(url, 'made_meanwhile')).toBe(1) + }, 120_000) + + it('keeps reading after a read that fails and a rendering with a temporary object', async () => { + const url = h.connectionString('target') + await h.applySql('target', 'CREATE TABLE public.kept (id int);') + expect(await holdSnapshot(url)).toBe(true) + await h.applySql('target', 'CREATE TABLE public.later (id int);') + + await expect(pgQuery(url, 'SELECT * FROM no_such_table')).rejects.toThrow(/no_such_table/) + const rendered = await inRolledBackTransaction(url, async client => { + await client.query('CREATE TEMP TABLE sf_probe (x int CHECK (x > 0))') + return (await client.query(`SELECT pg_get_constraintdef(oid) AS def FROM pg_constraint WHERE conrelid = 'sf_probe'::regclass`)).rows[0].def + }) + expect(rendered).toBe('CHECK ((x > 0))') + + expect(await count(url, 'kept')).toBe(1) + expect(await count(url, 'later')).toBe(0) + expect(await count(url, 'sf_probe')).toBe(0) + }, 120_000) +}) diff --git a/packages/cli/test/lock-budget.test.ts b/packages/cli/test/lock-budget.test.ts new file mode 100644 index 0000000..c0418a3 --- /dev/null +++ b/packages/cli/test/lock-budget.test.ts @@ -0,0 +1,63 @@ +import { describe, it, expect } from 'vitest' +import { estimateLocks, lockBudgetWarning, readLockBudget } from '../src/lock-budget.js' + +describe('estimateLocks', () => { + it('counts each relation once, however often it is named', () => { + expect(estimateLocks([ + 'ALTER TABLE public.a ADD COLUMN x int;', + 'ALTER TABLE public.a ADD COLUMN y int;', + 'COMMENT ON TABLE public.a IS \'one\';', + ])).toBe(1) + }) + + it('adds what a new table brings: its TOAST table and index, and an index per key', () => { + // a, its TOAST table and index, its primary key's index, its identity sequence + expect(estimateLocks(['CREATE TABLE public.a (id int GENERATED ALWAYS AS IDENTITY PRIMARY KEY, body text);'])).toBe(5) + // b and c, b's TOAST pair, its unique key's index; c is only referenced + expect(estimateLocks(['CREATE TABLE public.b (id int UNIQUE REFERENCES public.c (id));'])).toBe(5) + }) + + it('counts a key added later, an index, and the tables they are on', () => { + expect(estimateLocks([ + 'ALTER TABLE ONLY public.a ADD CONSTRAINT a_pkey PRIMARY KEY (id);', + 'CREATE INDEX a_body ON public.a (body);', + ])).toBe(3) + }) + + it('grows with the schema', () => { + const tables = Array.from({ length: 200 }, (_, i) => `CREATE TABLE public.t${i} (id int PRIMARY KEY);`) + expect(estimateLocks(tables)).toBe(800) + }) + + it('counts a statement it cannot parse as one', () => { + expect(estimateLocks(['\\connect other', 'not sql at all'])).toBe(2) + }) +}) + +describe('lockBudgetWarning', () => { + const budget = { capacity: 1000, inUse: 100 } + + it('says nothing when the locks fit, keeping a tenth for everyone else', () => { + expect(lockBudgetWarning(800, budget, 'This restore')).toBeUndefined() + expect(lockBudgetWarning(800, undefined, 'This restore')).toBeUndefined() + }) + + it('warns, with both numbers and the ways round it, when they do not', () => { + const warning = lockBudgetWarning(801, budget, 'This restore') + expect(warning).toMatch(/^This restore takes about 801 locks in one transaction, and the target has room for about 800/) + expect(warning).toContain('max_locks_per_transaction') + expect(warning).toContain('--no-transaction') + }) +}) + +describe('readLockBudget', () => { + it('reads the lock table and what is held', async () => { + const client = { query: async () => ({ rows: [{ capacity: '6400', in_use: '12' }] }) } + expect(await readLockBudget(client as never)).toEqual({ capacity: 6400, inUse: 12 }) + }) + + it('is undefined when the server will not say', async () => { + const client = { query: async () => { throw new Error('permission denied') } } + expect(await readLockBudget(client as never)).toBeUndefined() + }) +}) diff --git a/packages/cli/test/promote-savepoints.test.ts b/packages/cli/test/promote-savepoints.test.ts index c1f3460..ce3e6a6 100644 --- a/packages/cli/test/promote-savepoints.test.ts +++ b/packages/cli/test/promote-savepoints.test.ts @@ -27,7 +27,14 @@ vi.mock('pg', () => ({ else if (sql === 'ROLLBACK') pending = new Set(committed) else if (sql.startsWith('SAVEPOINT')) atSavepoint = new Set(pending) else if (sql.startsWith('ROLLBACK TO SAVEPOINT')) pending = new Set(atSavepoint) - else if (sql.startsWith('RELEASE SAVEPOINT')) { /* kept */ } else { + else if (sql.startsWith('RELEASE SAVEPOINT')) { /* kept */ } + // A revoke on `public.` needs to exist. + else if (/^REVOKE\b/i.test(sql)) { + const on = /\bON\s+(?:TABLE\s+)?public\.(\w+)/i.exec(sql)?.[1] + if (on && !pending.has(on)) { + throw Object.assign(new Error(`relation "public.${on}" does not exist`), { code: '42P01' }) + } + } else { const code = /code:(\w+)/.exec(sql)?.[1] if (code) throw Object.assign(new Error(`failed with ${code}`), { code }) for (const need of /needs:(\w+)/.exec(sql)?.[1].split(',') ?? []) { @@ -45,7 +52,7 @@ vi.mock('pg', () => ({ }, })) -const { promote, unmetDependency } = await import('../src/promote.js') +const { promote, unmetDependency, revokesFromAbsent } = await import('../src/promote.js') function scan(fixes: Array<[id: string, spec: string]>): ScanResult { const issues = fixes.map(([id, spec]) => ({ @@ -134,14 +141,109 @@ describe('unmetDependency', () => { it('accepts an extension the server does not ship, only for CREATE EXTENSION', () => { expect(unmetDependency('CREATE EXTENSION IF NOT EXISTS vector', err('0A000'))).toContain('does not ship') + // Into a schema of its own, which the fix makes first. + expect(unmetDependency('CREATE SCHEMA IF NOT EXISTS "graphql";\nCREATE EXTENSION IF NOT EXISTS "pg_graphql" SCHEMA "graphql";', err('0A000'))) + .toContain('does not ship') expect(unmetDependency('CREATE EXTENSION vector', err('58P01'))).toContain('does not ship') // 0A000 is "feature not supported" in general: anywhere else it is a real failure. expect(unmetDependency('ALTER TABLE t ALTER COLUMN c TYPE int', err('0A000'))).toBeUndefined() }) + // A newer source writes what an older target cannot parse: UNIQUE NULLS NOT + // DISTINCT before 15, a VIRTUAL column before 18, security_invoker before + // 15. On such a target that fix is left out, like one needing an extension + // it lacks; on a target as new as the source it is still a real failure. + it('accepts a syntax, feature or parameter an older target does not have', () => { + for (const code of ['42601', '0A000', '22023']) { + expect(unmetDependency('CREATE TABLE t (c text UNIQUE NULLS NOT DISTINCT)', err(code), true), code).toContain('older') + expect(unmetDependency('CREATE TABLE t (c text UNIQUE NULLS NOT DISTINCT)', err(code), false), code).toBeUndefined() + } + }) + it('rejects every other failure', () => { expect(unmetDependency('CREATE TABLE t (id int)', err('42P07'))).toBeUndefined() expect(unmetDependency('ALTER TABLE t ADD CONSTRAINT u UNIQUE (id)', err('23505'))).toBeUndefined() expect(unmetDependency('CREATE TABLE t (id int)', new Error('connection terminated'))).toBeUndefined() }) }) + +// A view the target has and the source lacks is dropped by one fix, and its +// grants revoked by others. Whichever ran first, the revokes are done: the +// object they would take privileges from is gone (issue #147). +describe('promote: revoking on an object the same apply drops', () => { + beforeEach(() => { + log.length = 0 + committed = new Set(['old_v']) + }) + + const issue = (id: string, check: 'schema' | 'roles', up: string) => + ({ id, check, severity: 'warning' as const, title: id, description: 'test', sql: { up, down: '' } }) + const scanOf = (issues: ReturnType[]): ScanResult => ({ + timestamp: '', source: 's', target: 't', score: 0, postureScore: null, + checks: [{ check: 'schema', status: 'drifted', issues, durationMs: 1 }], + summary: { total: issues.length, critical: 0, warning: issues.length, info: 0 }, + }) + + it('counts the revokes as done when the drop ran first', async () => { + // The stand-in "drops" old_v by forgetting it; the revokes then fail. + committed = new Set() + const result = await promote({ + dbUrl: 'postgres://t', + scanResult: scanOf([ + issue('drop-old_v', 'schema', "SELECT 'drops old_v'"), + issue('revoke-anon', 'roles', 'REVOKE SELECT ON TABLE public.old_v FROM anon'), + issue('revoke-authenticated', 'roles', 'REVOKE SELECT ON TABLE public.old_v FROM authenticated'), + ]), + }) + + expect(result.errors).toEqual([]) + expect(result.applied.map(f => f.issueId)).toEqual(['drop-old_v', 'revoke-anon', 'revoke-authenticated']) + expect(result.applied.filter(f => f.note).map(f => f.note)).toEqual([ + 'nothing to revoke: the object is not on the target', + 'nothing to revoke: the object is not on the target', + ]) + expect(log).toContain('COMMIT') + }) + + it('still retries a revoke whose object a later fix creates', async () => { + committed = new Set() + const result = await promote({ + dbUrl: 'postgres://t', + scanResult: scanOf([ + issue('revoke-anon', 'roles', 'REVOKE SELECT ON TABLE public.made FROM anon'), + issue('create-made', 'schema', "SELECT 'provides:made'"), + ]), + }) + + expect(result.errors).toEqual([]) + expect(result.applied.map(f => [f.issueId, f.note])).toEqual([['create-made', undefined], ['revoke-anon', undefined]]) + }) + + it('still leaves out a grant on an object the target lacks', async () => { + committed = new Set() + const result = await promote({ + dbUrl: 'postgres://t', + scanResult: scanOf([issue('grant-anon', 'roles', "SELECT 'needs:absent' /* GRANT */")]), + }) + + expect(result.applied).toEqual([]) + expect(result.errors.map(e => e.issueId)).toEqual(['grant-anon']) + }) +}) + +describe('revokesFromAbsent', () => { + it('is a revoke, or revokes, on one object', () => { + expect(revokesFromAbsent('REVOKE SELECT ON TABLE "public"."v" FROM "anon";')).toBe(true) + expect(revokesFromAbsent('REVOKE GRANT OPTION FOR SELECT ON public.v FROM anon; REVOKE SELECT ON public.v FROM anon;')).toBe(true) + expect(revokesFromAbsent('REVOKE EXECUTE ON FUNCTION public.f(int) FROM anon;')).toBe(true) + }) + + it('is not a grant, a drop, several objects or a whole schema', () => { + expect(revokesFromAbsent('GRANT SELECT ON public.v TO anon;')).toBe(false) + expect(revokesFromAbsent('DROP VIEW public.v;')).toBe(false) + expect(revokesFromAbsent('REVOKE SELECT ON public.a, public.b FROM anon;')).toBe(false) + expect(revokesFromAbsent('REVOKE SELECT ON public.a FROM anon; REVOKE SELECT ON public.b FROM anon;')).toBe(false) + expect(revokesFromAbsent('REVOKE SELECT ON ALL TABLES IN SCHEMA public FROM anon;')).toBe(false) + expect(revokesFromAbsent('not sql')).toBe(false) + }) +}) diff --git a/packages/cli/test/restore-coverage.test.ts b/packages/cli/test/restore-coverage.test.ts index 6b1cf88..0884d12 100644 --- a/packages/cli/test/restore-coverage.test.ts +++ b/packages/cli/test/restore-coverage.test.ts @@ -156,6 +156,12 @@ describe('previewSnapshotRestore: layer coverage', () => { * member of publication`. */ describe('rolesNamedBy', () => { + it('names the roles default privileges are for and grant to, not those they revoke from', () => { + expect(rolesNamedBy('ALTER DEFAULT PRIVILEGES FOR ROLE "owner" IN SCHEMA app GRANT SELECT ON TABLES TO reader, PUBLIC;')) + .toEqual(['owner', 'reader']) + expect(rolesNamedBy('ALTER DEFAULT PRIVILEGES REVOKE EXECUTE ON FUNCTIONS FROM gone;')).toEqual([]) + }) + it('names the role a grant needs', () => { expect(rolesNamedBy('GRANT SELECT ON "public"."orders" TO "anon";')).toEqual(['anon']) }) diff --git a/packages/cli/test/review-fixes.test.ts b/packages/cli/test/review-fixes.test.ts index d34191b..7029494 100644 --- a/packages/cli/test/review-fixes.test.ts +++ b/packages/cli/test/review-fixes.test.ts @@ -42,6 +42,16 @@ describe('platformOwnedObject by subject', () => { }) describe('tolerableFailure', () => { + // A snapshot of PostgreSQL 18 holds a VIRTUAL generated column, which 17 + // cannot parse: into an older server that statement is left out, and the + // restore goes on; into one as new it is still a real failure. + it('leaves out what an older server has no syntax for, only when it is older', () => { + const err = Object.assign(new Error('syntax error at or near ")"'), { code: '42601' }) + const sql = 'CREATE TABLE public.v (a integer, b integer GENERATED ALWAYS AS ((a * 2)));' + expect(tolerableFailure(sql, err, new Set(), new Set(), new Map(), true)).toContain('older than the snapshot') + expect(tolerableFailure(sql, err, new Set(), new Set(), new Map(), false)).toBeUndefined() + }) + const plain = new Set(['public', 'pg_catalog', 'information_schema']) const supabase = new Set([...plain, 'auth', 'storage', 'graphql', 'extensions']) const missing = (what: string) => ({ code: '3F000', message: `schema "${what}" does not exist` }) diff --git a/packages/cli/test/snapshot.test.ts b/packages/cli/test/snapshot.test.ts index ecfbc35..0f78344 100644 --- a/packages/cli/test/snapshot.test.ts +++ b/packages/cli/test/snapshot.test.ts @@ -542,6 +542,46 @@ describe('captureSnapshot: grants are captured one statement per role and object }) }) +// Grants on schemas, and default privileges, were left out of roles.sql: a +// restore lost USAGE on a schema of the project's own — so its tables were +// out of the Data API's reach — and what new tables are granted. +describe('roles.sql: schemas and default privileges', () => { + let dir: string + beforeEach(async () => { dir = await mkdtemp(join(tmpdir(), 'sf-snap-roles-')) }) + afterEach(async () => { await rm(dir, { recursive: true, force: true }) }) + + it('grants on a schema, and makes each role\'s defaults what they are', async () => { + const grants = [ + { grantee: 'reader', table_schema: 'app', table_name: '', column_name: null, privilege_type: 'USAGE', is_grantable: false, keyword: 'SCHEMA', args: null }, + ] + const defaults = [ + // One schema's entry adds to the global one. + { role: 'owner', schema: 'app', kind: 'r', action: 'GRANT', grantee: 'reader', privilege_type: 'SELECT', is_grantable: false }, + // The global one, against PostgreSQL's built-in default for functions. + { role: 'owner', schema: null, kind: 'f', action: 'REVOKE', grantee: 'PUBLIC', privilege_type: 'EXECUTE', is_grantable: false }, + { role: 'owner', schema: null, kind: 'S', action: 'GRANT', grantee: 'reader', privilege_type: 'SELECT', is_grantable: true }, + { role: 'owner', schema: null, kind: 'S', action: 'GRANT', grantee: 'reader', privilege_type: 'USAGE', is_grantable: true }, + ] + const queryFn = (async (_url: string, sql: string) => + sql.includes('pg_default_acl') ? defaults : sql.includes('nspacl') ? grants : []) as unknown as QueryFn + const result = await captureSnapshot({ + envName: 'prod', + env: { dbUrl: 'postgres://example' }, + config: { environments: { prod: { dbUrl: 'postgres://example' } } } as never, + outputDir: dir, + queryFn, + fetchFn: (async () => new Response('[]', { status: 200 })) as never, + }) + const lines = (await readFile(join(result.dir, 'roles.sql'), 'utf8')).split('\n').filter(l => /^(GRANT|ALTER)/.test(l)) + expect(lines).toEqual([ + 'GRANT USAGE ON SCHEMA "app" TO "reader";', + 'ALTER DEFAULT PRIVILEGES FOR ROLE "owner" IN SCHEMA "app" GRANT SELECT ON TABLES TO "reader";', + 'ALTER DEFAULT PRIVILEGES FOR ROLE "owner" REVOKE EXECUTE ON FUNCTIONS FROM PUBLIC;', + 'ALTER DEFAULT PRIVILEGES FOR ROLE "owner" GRANT SELECT, USAGE ON SEQUENCES TO "reader" WITH GRANT OPTION;', + ]) + }) +}) + describe('a snapshot without schema.sql', () => { const manifest = (schema: Record) => ({ timestamp: '20250101T120000Z', diff --git a/packages/cli/test/state-diff.test.ts b/packages/cli/test/state-diff.test.ts index 18b7e01..cf7b440 100644 --- a/packages/cli/test/state-diff.test.ts +++ b/packages/cli/test/state-diff.test.ts @@ -104,4 +104,20 @@ describe('diffState', () => { 'sequence public.free_seq: unexpected', ]) }) + + // A table renamed on PostgreSQL 18 keeps its NOT NULLs' old names; one made + // afresh, or on an older server, has others or none. Matched by column. + it('matches a NOT NULL constraint by its column, not its name', () => { + const nn = (name: string, column: string) => + ({ name, type: 'not_null', definition: `NOT NULL ${column}`, validated: true, deferrable: false, deferred: false }) + const a = empty(); a.tables = [table({ constraints: [nn('config_key_not_null', 'key')] }) as never] + const b = empty(); b.tables = [table({ constraints: [nn('settings_key_not_null', 'key')] }) as never] + expect(diffState(a, b)).toEqual([]) + + const c = empty(); c.tables = [table({ constraints: [nn('config_key_not_null', 'value')] }) as never] + expect(diffState(a, c)).toEqual([ + 'constraint on public.t NOT NULL key: missing', + 'constraint on public.t NOT NULL value: unexpected', + ]) + }) })