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.

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()
+  })
+})

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}`)
   }