From 8b151bd3a78628e63a115bf9494d2b60cc5f3266 Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Thu, 17 Sep 2026 00:26:07 -0400 Subject: [PATCH] fix(relay): refuse a multi-action ALTER TABLE instead of pre-checking its first action `ALTER TABLE t ADD COLUMN IF NOT EXISTS a TEXT, ADD COLUMN IF NOT EXISTS b TEXT` derived the target for `a` alone, so once `a` existed the whole statement was skipped and `b` was never added. The first subcommand parses, so neither the parse throw nor the census caught it. A lock-taking ALTER TABLE with a comma outside parentheses, quotes and comments now throws at boot. One action per statement, or no pre-check is possible. Commas inside a parenthesised type, a CHECK body, a quoted default or a comment are unaffected, and push's 18 statements still parse. --- .../src/schema-lock-target.test.ts | 46 +++++++++++++++++++ .../postgres-schema/src/schema-lock-target.ts | 40 +++++++++++++++- 2 files changed, 85 insertions(+), 1 deletion(-) diff --git a/cloud/packages/postgres-schema/src/schema-lock-target.test.ts b/cloud/packages/postgres-schema/src/schema-lock-target.test.ts index ede376786e1..0c7868af3f0 100644 --- a/cloud/packages/postgres-schema/src/schema-lock-target.test.ts +++ b/cloud/packages/postgres-schema/src/schema-lock-target.test.ts @@ -200,3 +200,49 @@ describe('requireSchemaLockTarget', () => { expect(requireSchemaLockTarget(statement)).toBeUndefined() }) }) + +describe('multi-action ALTER TABLE', () => { + it('throws rather than deriving only the first subcommand', () => { + // Deriving `a` and skipping on it would drop `b` for the life of the database, and the first + // subcommand parses fine, so nothing else here would catch it. + const statement = + 'ALTER TABLE t ADD COLUMN IF NOT EXISTS a TEXT, ADD COLUMN IF NOT EXISTS b TEXT' + expect(schemaLockTarget(statement)).toEqual({ + kind: 'column', + table: 't', + name: 'a', + skipWhen: 'present' + }) + expect(() => requireSchemaLockTarget(statement)).toThrow(/unparsed_schema_lock_target/) + }) + + it('throws on a constraint swap written as one statement', () => { + expect(() => + requireSchemaLockTarget( + 'ALTER TABLE t DROP CONSTRAINT IF EXISTS old, ADD CONSTRAINT new CHECK (x > 0)' + ) + ).toThrow(/unparsed_schema_lock_target/) + }) + + it.each([ + ['a parenthesised type', 'ALTER TABLE t ADD COLUMN IF NOT EXISTS a NUMERIC(10, 2)'], + ['a CHECK body', "ALTER TABLE t ADD CONSTRAINT c CHECK (r IN ('us-central1', 'asia-east2'))"], + ['a quoted comma', `ALTER TABLE t ADD COLUMN IF NOT EXISTS a TEXT DEFAULT 'x, y'`], + ['a doubled quote before a comma', `ALTER TABLE t ADD COLUMN a TEXT DEFAULT 'it''s, fine'`], + ['a trailing line comment', 'ALTER TABLE t ADD COLUMN a TEXT -- one, two'], + ['a trailing block comment', 'ALTER TABLE t ADD COLUMN a TEXT /* one, two */'] + ])('does not throw on %s', (_label, statement) => { + expect(() => requireSchemaLockTarget(statement)).not.toThrow() + }) + + it('throws on a block comment sitting where the column name belongs', () => { + // Unparseable for an ordinary reason, and still the right answer: no target means no pre-check. + expect(() => + requireSchemaLockTarget('ALTER TABLE t ADD COLUMN /* note */ a TEXT') + ).toThrow(/unparsed_schema_lock_target/) + }) + + it('leaves a multi-column CREATE INDEX alone', () => { + expect(() => requireSchemaLockTarget('CREATE INDEX IF NOT EXISTS i ON t(a, b)')).not.toThrow() + }) +}) diff --git a/cloud/packages/postgres-schema/src/schema-lock-target.ts b/cloud/packages/postgres-schema/src/schema-lock-target.ts index d0728181a68..ee289a95b8b 100644 --- a/cloud/packages/postgres-schema/src/schema-lock-target.ts +++ b/cloud/packages/postgres-schema/src/schema-lock-target.ts @@ -110,14 +110,52 @@ export function schemaLockTarget(statement: string): SchemaLockTarget | undefine return undefined } +const ALTER_TABLE = /^ALTER\s+TABLE\b/i + +// A comma that separates ALTER TABLE subcommands rather than sitting inside a type, a default, or a +// CHECK body. Quotes and parentheses are tracked so `CHECK (r IN ('a', 'b'))` and `NUMERIC(10, 2)` +// do not read as one. +function hasTopLevelComma(sql: string): boolean { + let depth = 0 + let quote: string | undefined + for (let index = 0; index < sql.length; index += 1) { + const character = sql[index] + if (quote !== undefined) { + if (character !== quote) continue + if (sql[index + 1] === quote) index += 1 + else quote = undefined + continue + } + if (character === "'" || character === '"') quote = character + else if (character === '-' && sql[index + 1] === '-') { + const newline = sql.indexOf('\n', index) + if (newline === -1) return false + index = newline + } else if (character === '/' && sql[index + 1] === '*') { + const close = sql.indexOf('*/', index + 2) + if (close === -1) return false + index = close + 1 + } else if (character === '(') depth += 1 + else if (character === ')') depth -= 1 + else if (character === ',' && depth === 0) return true + } + return false +} + // An index or column statement whose target cannot be read is the dangerous case: it would be sent // unchecked and take the lock the pre-check exists to avoid, silently and on every boot. An // auto-named `CREATE INDEX ON t(c)` lands here too, because nothing in the text says what the // catalog will call it. Fail the boot with the statement instead. export function requireSchemaLockTarget(statement: string): SchemaLockTarget | undefined { + const sql = sqlWithoutLeadingComments(statement) + // A multi-action ALTER TABLE parses to its FIRST subcommand's target only, so skipping on that + // one object would silently drop every later action for the life of the database. One action per + // statement, or no pre-check is possible. + if (ALTER_TABLE.test(sql) && hasTopLevelComma(sql)) { + throw new Error(`unparsed_schema_lock_target: ${sql}`) + } const target = schemaLockTarget(statement) if (target) return target - const sql = sqlWithoutLeadingComments(statement) if (MUST_PARSE.some((shape) => shape.test(sql))) { throw new Error(`unparsed_schema_lock_target: ${sql}`) }