From 067bfc4d68dbad1a5801e03e58ee5aee465d26b2 Mon Sep 17 00:00:00 2001 From: Armand Parajon Date: Sat, 17 Oct 2026 02:28:19 -0400 Subject: [PATCH 1/3] feat(lint): flag a CONCURRENTLY operation inside a transaction block PostgreSQL refuses every CONCURRENTLY form inside an explicit block, so a script that wraps one in BEGIN ... COMMIT fails at that statement and rolls back. Lint now tracks the block a script opens and reports the operation as concurrent-in-transaction, an error, with how to fix it. statement.TransactionControlOf reads BEGIN, COMMIT, and their variants from the parse tree. The lint report moves to format_version 3. Co-Authored-By: Claude Opus 5.5 --- demo/tour.sh | 2 +- docs/cli-output-examples.md | 2 +- docs/lint-report.md | 10 ++-- docs/postgres-online-ddl-reference.md | 13 +++++ internal/cli/lint_test.go | 24 ++++++++ internal/cli/lint_text.go | 6 ++ pkg/lint/lint.go | 46 +++++++++++++-- pkg/lint/lint_test.go | 64 ++++++++++++++++++++- pkg/lint/transaction_integration_test.go | 71 ++++++++++++++++++++++++ pkg/statement/transaction.go | 55 ++++++++++++++++++ pkg/statement/transaction_test.go | 45 +++++++++++++++ 11 files changed, 325 insertions(+), 13 deletions(-) create mode 100644 pkg/lint/transaction_integration_test.go create mode 100644 pkg/statement/transaction.go create mode 100644 pkg/statement/transaction_test.go diff --git a/demo/tour.sh b/demo/tour.sh index 32b4700b..eef03f0b 100755 --- a/demo/tour.sh +++ b/demo/tour.sh @@ -326,7 +326,7 @@ run_offline() { if [ "$CHECK" = 1 ]; then out=$("$PGS" lint --json risky.sql) || status=$? assert_eq "lint exit" 1 "$status" - assert_eq "lint format_version" 2 "$(jq -r '.format_version' <<<"$out")" + assert_eq "lint format_version" 3 "$(jq -r '.format_version' <<<"$out")" assert_eq "lint errors" 1 "$(jq -r '.errors' <<<"$out")" else "$PGS" lint risky.sql || echo "(exit $? — error findings gate, by design)" diff --git a/docs/cli-output-examples.md b/docs/cli-output-examples.md index 76d1a691..95f6217c 100644 --- a/docs/cli-output-examples.md +++ b/docs/cli-output-examples.md @@ -453,7 +453,7 @@ ALTER TABLE users ADD CONSTRAINT users_email_key UNIQUE (email); ```console $ pg-sprite lint /tmp/changes.sql --json { - "format_version": 2, + "format_version": 3, "postgres_versions": "14-18", "findings": [ { diff --git a/docs/lint-report.md b/docs/lint-report.md index d72e2056..b7096264 100644 --- a/docs/lint-report.md +++ b/docs/lint-report.md @@ -15,9 +15,10 @@ and the closed vocabularies below (codes, severities, and the embedded planner r adding a value to any of them is a contract change and bumps `format_version`, even if no field is added or renamed. -The current version is **2**: version 2 added the `replica-identity` reason to the embedded -Reasons vocabulary. The linter itself never reports it, because the reason is judged from live -replica identity facts the linter does not have. +The current version is **3**: version 3 added the `concurrent-in-transaction` code. Version 2 +added the `replica-identity` reason to the embedded Reasons vocabulary. The linter itself never +reports that reason, because it is judged from live replica identity facts the linter does not +have. ### Relationship to the plan report @@ -70,7 +71,7 @@ codes make the conservatism visible instead of burying it: | `operation` | string | always | Operator-facing label of the flagged operation (`DROP COLUMN legacy_a`). Display only — never branch on it. | | `code` | string | always | The typed finding kind (see Codes). Automation branches on this, never on prose. | | `severity` | string | always | What the engine would do about it (see Severities). | -| `reason` | string | classifier findings | The planner's typed cause, drawn from the plan report's Reasons vocabulary. Absent for destructive findings, which are a property of the operation, not a routing decision. | +| `reason` | string | classifier findings | The planner's typed cause, drawn from the plan report's Reasons vocabulary. Absent for `destructive` and `concurrent-in-transaction` findings, which are a property of the operation or the script, not a routing decision. | | `suggestion` | array | when constructible | The ordered safer SQL, present only for `blocking-idiom` findings where the planner constructed the rewrite. A safer form of the submitted statement, not a semantic equivalent — running it by hand forgoes the engine's execution-time guards (invalid-index detection after a concurrent build). Its absence still means the submitted form blocks — the planner does not construct rewrites for multi-operation statements or for operations that need catalog knowledge (ATTACH PARTITION's proving CHECK); the [suggest report](suggest-report.md) emits typed guidance naming the manual path for exactly those cases. | | `suggestion_execution` | string | with `suggestion` | The typed execution contract for `suggestion`, drawn from the plan report's Execution contracts vocabulary (`autocommit-each-step`: each step in its own implicit transaction, never inside an enclosing transaction block; a failed step leaves partial state the runner must detect and recover). A consumer that runs the suggestion branches on this. Present exactly when `suggestion` is. | @@ -84,6 +85,7 @@ codes make the conservatism visible instead of burying it: | `possible-table-rewrite` | warning | `ALTER TABLE t ALTER COLUMN c TYPE bigint` | The linter cannot verify the operation against live column facts, so the engine would fail closed to the rewrite path — but the change may be a free relabel a live database would prove. | | `app-breaking-rename` | warning | `ALTER TABLE t RENAME COLUMN email TO email_address` | PostgreSQL runs a column or table rename as a metadata-only catalog flip, but a rename cannot land atomically across running application instances — code still referencing the old name breaks the instant it commits. For a column, expand/contract instead: add the new column, dual-write and backfill, switch reads, then drop the old column as its own reviewed change. For a table, coordinate the rename with the application deploy that adopts the new name. Index renames are not flagged — SQL never references an index by name. | | `destructive` | warning | `ALTER TABLE t DROP COLUMN legacy` | The operation discards live structure (a column, constraint, or index drop) and cannot be undone by re-running the schema. | +| `concurrent-in-transaction` | error | `BEGIN; CREATE INDEX CONCURRENTLY i ON t (c); COMMIT` | A `CONCURRENTLY` operation sits inside a transaction block the script opens with `BEGIN` or `START TRANSACTION`. PostgreSQL refuses every `CONCURRENTLY` form inside a block, so the script fails at that statement and the block rolls back. Run the statement outside the block. A savepoint, or a `COMMIT AND CHAIN` that opens the next block at once, leaves the script inside its block. | A lint `code` is what the finding *is*; the finding's `reason` field is the classifier's typed cause for it, drawn from the plan report's Reason vocabulary diff --git a/docs/postgres-online-ddl-reference.md b/docs/postgres-online-ddl-reference.md index 326daef9..cb48fe81 100644 --- a/docs/postgres-online-ddl-reference.md +++ b/docs/postgres-online-ddl-reference.md @@ -526,6 +526,19 @@ about existing rows and is recreated by a metadata-only statement). Emitted alongside the routing decision as a warning so destructive intent is always visible in review. +### `concurrent-in-transaction` + +| Verdict | Lock | Scan / rewrite | Lint exit | +|---|---|---|---| +| lint error — the script fails at this statement | none: PostgreSQL refuses the statement before it starts | — | 1 | + +A `CONCURRENTLY` operation (`CREATE INDEX`, `DROP INDEX`, `REINDEX`, or `DETACH +PARTITION`) sits inside a transaction block the script opens with `BEGIN` or +`START TRANSACTION`. PostgreSQL refuses every `CONCURRENTLY` form inside a block, +so the script stops at that statement and the whole block rolls back. Move the +statement out of the block and run it on its own. Only `lint` reports this code: +it is a property of the script, and `migrate` takes one statement at a time. + ### `rewrite-required` | Verdict | Lock (as written) | Scan / rewrite | Dry-run exit | diff --git a/internal/cli/lint_test.go b/internal/cli/lint_test.go index ddd7e38d..b0a2797a 100644 --- a/internal/cli/lint_test.go +++ b/internal/cli/lint_test.go @@ -113,6 +113,30 @@ func TestLintTextSuggestionCarriesExecutionCaveat(t *testing.T) { "the caveat names the recovery check a manual run takes on") } +// A CONCURRENTLY statement inside a BEGIN … COMMIT block names why the +// script fails there and how to fix it, and links its own reference +// section. This is the renderer's own unit test — everything else asserts +// typed fields. +func TestLintTextConcurrentInTransactionSaysHowToFixIt(t *testing.T) { + var out strings.Builder + cmd := LintCmd{} + err := cmd.runLint(strings.NewReader("BEGIN;\nCREATE INDEX CONCURRENTLY i ON t (c);\nCOMMIT;\n"), &out) + require.ErrorIs(t, err, ErrLintFindings) + assert.Contains(t, out.String(), `:2:1: + CREATE INDEX CONCURRENTLY i ON t (c); + +error[concurrent-in-transaction]: + CREATE INDEX i — PostgreSQL refuses CONCURRENTLY inside a transaction + block, so the script fails here and the block rolls back + +help: + run this statement on its own, outside the BEGIN … COMMIT block + +docs: + `+onlineDDLReferenceURL+`#concurrent-in-transaction +`) +} + func TestLintParseFailureIsErrorNotFinding(t *testing.T) { var out strings.Builder cmd := LintCmd{} diff --git a/internal/cli/lint_text.go b/internal/cli/lint_text.go index 373b1cca..905420a2 100644 --- a/internal/cli/lint_text.go +++ b/internal/cli/lint_text.go @@ -31,6 +31,9 @@ func writeLintText(out io.Writer, pal palette, name string, report lint.Report) statement = f.Statement } w.diag(string(f.Severity), string(f.Code), lintMessage(f)) + if f.Code == lint.CodeConcurrentInTransaction { + w.diag("help", "", "run this statement on its own, outside the BEGIN … COMMIT block") + } if f.Code == lint.CodePossibleTableRewrite { w.diag("note", "", "the table was not introspected, so this is the conservative classification; with live facts the same change may classify as cheaper") } @@ -60,6 +63,9 @@ func lintMessage(f lint.Finding) string { if f.Code == lint.CodeDestructive { return f.Operation + " — discards live data or structure" } + if f.Code == lint.CodeConcurrentInTransaction { + return f.Operation + " — PostgreSQL refuses CONCURRENTLY inside a transaction block, so the script fails here and the block rolls back" + } if f.Reason != "" { return fmt.Sprintf("%s — %s", f.Operation, impactText(f.Reason)) } diff --git a/pkg/lint/lint.go b/pkg/lint/lint.go index 43fe2d65..da802a67 100644 --- a/pkg/lint/lint.go +++ b/pkg/lint/lint.go @@ -17,8 +17,9 @@ import ( // FormatVersion identifies the report contract. A consumer must reject a // report whose version it does not understand instead of guessing at the // field semantics. Version 2 added the replica-identity planner reason to -// the embedded Reasons vocabulary. -const FormatVersion = 2 +// the embedded Reasons vocabulary; version 3 added the +// concurrent-in-transaction code. +const FormatVersion = 3 // Severity ranks a finding by what the engine would do with it. type Severity string @@ -71,6 +72,12 @@ const ( // whether an index is unique — and a dropped unique index whose gap // admitted duplicates cannot be recreated at all. CodeDestructive Code = "destructive" + // CodeConcurrentInTransaction: a CONCURRENTLY operation sits inside + // an explicit transaction block the script opens with BEGIN or START + // TRANSACTION. PostgreSQL refuses every CONCURRENTLY form inside a + // block, so the script fails at that statement and the block rolls + // back. Run the statement outside the block, on its own. + CodeConcurrentInTransaction Code = "concurrent-in-transaction" ) // Finding is one lint result: the statement it is about, the typed code, @@ -141,12 +148,23 @@ func Check(sql string) (Report, error) { PostgresVersions: planner.RulesPostgresVersions, Findings: []Finding{}, } + inBlock := false for i, stmt := range stmts { - findings, err := checkStatement(i+1, stmt) + findings, err := checkStatement(i+1, stmt, inBlock) if err != nil { return Report{}, fmt.Errorf("statement %d: %w", i+1, err) } report.Findings = append(report.Findings, findings...) + control, err := statement.TransactionControlOf(stmt.SQL) + if err != nil { + return Report{}, fmt.Errorf("statement %d: %w", i+1, err) + } + switch control { + case statement.TransactionBegin: + inBlock = true + case statement.TransactionEnd: + inBlock = false + } } for _, f := range report.Findings { if f.Severity == SeverityError { @@ -162,8 +180,11 @@ func Check(sql string) (Report, error) { // from the classifier's routing decisions: a routing finding where the // route warrants one, and a destructive finding where the decision is // marked destructive. The classifier is the single rulebook — the linter -// adds severity and presentation, never a second opinion. -func checkStatement(index int, stmt statement.SourceStatement) ([]Finding, error) { +// adds severity and presentation, never a second opinion. The one finding +// it adds is about the script rather than the statement: a CONCURRENTLY +// operation inside an explicit transaction block, which PostgreSQL refuses +// whatever the classifier makes of the operation. +func checkStatement(index int, stmt statement.SourceStatement, inBlock bool) ([]Finding, error) { classified, err := planner.Classify(stmt.SQL, planner.Facts{}) if err != nil { return nil, err @@ -173,6 +194,21 @@ func checkStatement(index int, stmt statement.SourceStatement) ([]Finding, error f.Statement, f.Line, f.Column, f.SQL = index, stmt.Line, stmt.Column, stmt.SQL return f } + if inBlock { + ops, err := statement.ParseOps(stmt.SQL) + if err != nil { + return nil, err + } + for _, op := range ops { + if op.Concurrent { + findings = append(findings, place(Finding{ + Operation: op.Describe(), + Code: CodeConcurrentInTransaction, + Severity: SeverityError, + })) + } + } + } for _, d := range classified.Decisions { if f, flagged := decisionFinding(d); flagged { findings = append(findings, place(f)) diff --git a/pkg/lint/lint_test.go b/pkg/lint/lint_test.go index 24fc7cf5..f2cc7ad0 100644 --- a/pkg/lint/lint_test.go +++ b/pkg/lint/lint_test.go @@ -196,7 +196,7 @@ func TestReportJSONShape(t *testing.T) { suggestion, err := json.Marshal(report.Findings[0].Suggestion) require.NoError(t, err) assert.JSONEq(t, `{ - "format_version": 2, + "format_version": 3, "postgres_versions": "14-18", "findings": [ { @@ -223,10 +223,70 @@ func TestReportJSONCleanFindingsAreEmptyArray(t *testing.T) { raw, err := json.Marshal(report) require.NoError(t, err) assert.JSONEq(t, `{ - "format_version": 2, + "format_version": 3, "postgres_versions": "14-18", "findings": [], "errors": 0, "warnings": 0 }`, string(raw)) } + +// A CONCURRENTLY operation inside an explicit transaction block is an +// error whatever the classifier makes of it, and only while the block is +// open: a savepoint or COMMIT AND CHAIN keeps the script inside it, and a +// COMMIT, ROLLBACK, or PREPARE TRANSACTION closes it. +func TestCheckFlagsConcurrentInsideTransactionBlock(t *testing.T) { + cases := []struct { + name string + script string + // flagged lists the statement indexes carrying the finding. + flagged []int + }{ + {name: "create index in a block", script: "BEGIN; CREATE INDEX CONCURRENTLY i ON t (c); COMMIT", flagged: []int{2}}, + {name: "start transaction", script: "START TRANSACTION; CREATE INDEX CONCURRENTLY i ON t (c); COMMIT", flagged: []int{2}}, + {name: "drop index", script: "BEGIN; DROP INDEX CONCURRENTLY i; COMMIT", flagged: []int{2}}, + {name: "reindex", script: "BEGIN; REINDEX INDEX CONCURRENTLY i; COMMIT", flagged: []int{2}}, + {name: "detach partition", script: "BEGIN; ALTER TABLE t DETACH PARTITION p CONCURRENTLY; COMMIT", flagged: []int{2}}, + {name: "block never closed", script: "BEGIN; CREATE INDEX CONCURRENTLY i ON t (c)", flagged: []int{2}}, + {name: "after a savepoint", script: "BEGIN; SAVEPOINT s; CREATE INDEX CONCURRENTLY i ON t (c); COMMIT", flagged: []int{3}}, + {name: "after commit and chain", script: "BEGIN; COMMIT AND CHAIN; CREATE INDEX CONCURRENTLY i ON t (c); COMMIT", flagged: []int{3}}, + {name: "after commit", script: "BEGIN; ALTER TABLE t ADD COLUMN c int; COMMIT; CREATE INDEX CONCURRENTLY i ON t (c)"}, + {name: "after rollback", script: "BEGIN; ROLLBACK; CREATE INDEX CONCURRENTLY i ON t (c)"}, + {name: "after prepare transaction", script: "BEGIN; PREPARE TRANSACTION 'g'; CREATE INDEX CONCURRENTLY i ON t (c)"}, + {name: "no block", script: "CREATE INDEX CONCURRENTLY i ON t (c)"}, + {name: "blocking form in a block", script: "BEGIN; CREATE INDEX i ON t (c); COMMIT"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + report, err := lint.Check(tc.script) + require.NoError(t, err) + var flagged []int + for _, f := range report.Findings { + if f.Code != lint.CodeConcurrentInTransaction { + continue + } + flagged = append(flagged, f.Statement) + assert.Equal(t, lint.SeverityError, f.Severity) + assert.Empty(t, f.Reason) + assert.Empty(t, f.Suggestion) + } + assert.Equal(t, tc.flagged, flagged) + }) + } +} + +func TestCheckConcurrentInTransactionNamesTheOperation(t *testing.T) { + report, err := lint.Check("BEGIN;\nCREATE INDEX CONCURRENTLY t_c_idx ON t (c);\nCOMMIT;") + require.NoError(t, err) + var found *lint.Finding + for i := range report.Findings { + if report.Findings[i].Code == lint.CodeConcurrentInTransaction { + found = &report.Findings[i] + } + } + require.NotNil(t, found) + assert.Equal(t, 2, found.Statement) + assert.Equal(t, 2, found.Line) + assert.Equal(t, "CREATE INDEX CONCURRENTLY t_c_idx ON t (c)", found.SQL) + assert.Equal(t, "CREATE INDEX t_c_idx", found.Operation) +} diff --git a/pkg/lint/transaction_integration_test.go b/pkg/lint/transaction_integration_test.go new file mode 100644 index 00000000..285c4849 --- /dev/null +++ b/pkg/lint/transaction_integration_test.go @@ -0,0 +1,71 @@ +package lint_test + +import ( + "errors" + "fmt" + "testing" + + "github.com/jackc/pgx/v5/pgconn" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/block/pg-sprite/internal/testutil" + "github.com/block/pg-sprite/pkg/dbconn" + "github.com/block/pg-sprite/pkg/lint" +) + +// The concurrent-in-transaction finding rests on PostgreSQL refusing every +// CONCURRENTLY form inside an explicit block. Each case runs a script lint +// flags, statement by statement on one session, and requires the server +// to refuse the flagged statement with active_sql_transaction. +func TestServerRefusesWhatConcurrentInTransactionFlags(t *testing.T) { + url := testutil.StartPostgres(t) + pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: url}) + require.NoError(t, err) + t.Cleanup(pool.Close) + + for _, sql := range []string{ + "CREATE INDEX CONCURRENTLY t_c_new ON %[1]s.t (c)", + "DROP INDEX CONCURRENTLY %[1]s.t_c_idx", + "REINDEX INDEX CONCURRENTLY %[1]s.t_c_idx", + "ALTER TABLE %[1]s.p DETACH PARTITION %[1]s.p_low CONCURRENTLY", + } { + t.Run(sql, func(t *testing.T) { + schema := testutil.NewSchema(t, pool) + for _, ddl := range []string{ + "CREATE TABLE %[1]s.t (c int)", + "CREATE INDEX t_c_idx ON %[1]s.t (c)", + "CREATE TABLE %[1]s.p (id int) PARTITION BY RANGE (id)", + "CREATE TABLE %[1]s.p_low PARTITION OF %[1]s.p FOR VALUES FROM (0) TO (10)", + } { + _, err := pool.Exec(t.Context(), fmt.Sprintf(ddl, schema)) + require.NoError(t, err) + } + flagged := fmt.Sprintf(sql, schema) + report, err := lint.Check("BEGIN; " + flagged + "; COMMIT") + require.NoError(t, err) + require.True(t, hasCode(report, 2, lint.CodeConcurrentInTransaction), "lint flags %s inside a block", flagged) + + conn, err := pool.Acquire(t.Context()) + require.NoError(t, err) + defer conn.Release() + _, err = conn.Exec(t.Context(), "BEGIN") + require.NoError(t, err) + _, err = conn.Exec(t.Context(), flagged) + var pgErr *pgconn.PgError + require.True(t, errors.As(err, &pgErr), "the server refuses %s inside a block: %v", flagged, err) + assert.Equal(t, "25001", pgErr.Code, "active_sql_transaction for %s", flagged) + _, err = conn.Exec(t.Context(), "ROLLBACK") + require.NoError(t, err) + }) + } +} + +func hasCode(report lint.Report, stmt int, code lint.Code) bool { + for _, f := range report.Findings { + if f.Statement == stmt && f.Code == code { + return true + } + } + return false +} diff --git a/pkg/statement/transaction.go b/pkg/statement/transaction.go new file mode 100644 index 00000000..8f454445 --- /dev/null +++ b/pkg/statement/transaction.go @@ -0,0 +1,55 @@ +package statement + +import ( + "fmt" + + pganalyze "github.com/pganalyze/pg_query_go/v6" + pgquery "github.com/wasilibs/go-pgquery" +) + +// TransactionControl is what a statement does to an explicit transaction +// block, read from the parse tree. +type TransactionControl int + +// The effects a statement can have on an explicit transaction block. +const ( + // TransactionNone: the statement neither opens nor closes a block. A + // savepoint statement, and a COMMIT or ROLLBACK AND CHAIN, which + // closes one block and opens the next at once, leave a script inside + // its block. + TransactionNone TransactionControl = iota + // TransactionBegin: BEGIN or START TRANSACTION opens a block. + TransactionBegin + // TransactionEnd: COMMIT, END, ROLLBACK, ABORT, or PREPARE + // TRANSACTION closes the open block. + TransactionEnd +) + +// TransactionControlOf reports what sql, exactly one statement, does to an +// explicit transaction block. +func TransactionControlOf(sql string) (TransactionControl, error) { + tree, err := pgquery.Parse(sql) + if err != nil { + return TransactionNone, fmt.Errorf("parse statement: %w", err) + } + if n := len(tree.GetStmts()); n != 1 { + return TransactionNone, fmt.Errorf("%w: got %d", ErrNotOneStatement, n) + } + tx := tree.GetStmts()[0].GetStmt().GetTransactionStmt() + if tx == nil { + return TransactionNone, nil + } + switch tx.GetKind() { + case pganalyze.TransactionStmtKind_TRANS_STMT_BEGIN, pganalyze.TransactionStmtKind_TRANS_STMT_START: + return TransactionBegin, nil + case pganalyze.TransactionStmtKind_TRANS_STMT_COMMIT, pganalyze.TransactionStmtKind_TRANS_STMT_ROLLBACK: + if tx.GetChain() { + return TransactionNone, nil + } + return TransactionEnd, nil + case pganalyze.TransactionStmtKind_TRANS_STMT_PREPARE: + return TransactionEnd, nil + default: + return TransactionNone, nil + } +} diff --git a/pkg/statement/transaction_test.go b/pkg/statement/transaction_test.go new file mode 100644 index 00000000..7226490a --- /dev/null +++ b/pkg/statement/transaction_test.go @@ -0,0 +1,45 @@ +package statement_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/block/pg-sprite/pkg/statement" +) + +func TestTransactionControlOf(t *testing.T) { + cases := []struct { + sql string + want statement.TransactionControl + }{ + {"BEGIN", statement.TransactionBegin}, + {"BEGIN ISOLATION LEVEL SERIALIZABLE", statement.TransactionBegin}, + {"START TRANSACTION", statement.TransactionBegin}, + {"COMMIT", statement.TransactionEnd}, + {"END", statement.TransactionEnd}, + {"ROLLBACK", statement.TransactionEnd}, + {"ABORT", statement.TransactionEnd}, + {"PREPARE TRANSACTION 'gid'", statement.TransactionEnd}, + {"COMMIT AND CHAIN", statement.TransactionNone}, + {"ROLLBACK AND CHAIN", statement.TransactionNone}, + {"SAVEPOINT s", statement.TransactionNone}, + {"RELEASE SAVEPOINT s", statement.TransactionNone}, + {"ROLLBACK TO SAVEPOINT s", statement.TransactionNone}, + {"COMMIT PREPARED 'gid'", statement.TransactionNone}, + {"CREATE INDEX CONCURRENTLY i ON t (c)", statement.TransactionNone}, + } + for _, tc := range cases { + t.Run(tc.sql, func(t *testing.T) { + got, err := statement.TransactionControlOf(tc.sql) + require.NoError(t, err) + assert.Equal(t, tc.want, got) + }) + } +} + +func TestTransactionControlOfRequiresOneStatement(t *testing.T) { + _, err := statement.TransactionControlOf("BEGIN; COMMIT") + require.ErrorIs(t, err, statement.ErrNotOneStatement) +} From e84b3a8d19e94e83f980d56a3f314b4ff5e87d45 Mon Sep 17 00:00:00 2001 From: Armand Parajon Date: Sat, 17 Oct 2026 11:46:55 -0400 Subject: [PATCH 2/3] fix(lint): read a REINDEX CONCURRENTLY option's value REINDEX (CONCURRENTLY false) and (CONCURRENTLY 0) rebuild with the blocking lock, which PostgreSQL runs inside a transaction block, but the parser counted any CONCURRENTLY option as concurrent. Lint flagged them as concurrent-in-transaction, and the concurrent rewrite appended a second CONCURRENTLY option the server refuses as conflicting. The option's value is now read as the server's defGetBoolean reads it, through one helper that replaces the duplicate in ops.go, and the rewrite replaces a disabled option instead of adding one. Co-Authored-By: Claude Opus 5.5 --- pkg/lint/lint_test.go | 3 ++ pkg/lint/transaction_integration_test.go | 39 +++++++++++++++++++++ pkg/statement/ops.go | 13 +------ pkg/statement/ops_test.go | 43 ++++++++++++++++++++++++ pkg/statement/rewrite.go | 6 +++- pkg/statement/statement.go | 42 +++++++++++++++++++---- 6 files changed, 127 insertions(+), 19 deletions(-) diff --git a/pkg/lint/lint_test.go b/pkg/lint/lint_test.go index f2cc7ad0..655daad3 100644 --- a/pkg/lint/lint_test.go +++ b/pkg/lint/lint_test.go @@ -246,6 +246,9 @@ func TestCheckFlagsConcurrentInsideTransactionBlock(t *testing.T) { {name: "start transaction", script: "START TRANSACTION; CREATE INDEX CONCURRENTLY i ON t (c); COMMIT", flagged: []int{2}}, {name: "drop index", script: "BEGIN; DROP INDEX CONCURRENTLY i; COMMIT", flagged: []int{2}}, {name: "reindex", script: "BEGIN; REINDEX INDEX CONCURRENTLY i; COMMIT", flagged: []int{2}}, + {name: "reindex with concurrently true", script: "BEGIN; REINDEX (CONCURRENTLY true) INDEX i; COMMIT", flagged: []int{2}}, + {name: "reindex with concurrently false", script: "BEGIN; REINDEX (CONCURRENTLY false) INDEX i; COMMIT"}, + {name: "reindex with concurrently 0", script: "BEGIN; REINDEX (CONCURRENTLY 0) INDEX i; COMMIT"}, {name: "detach partition", script: "BEGIN; ALTER TABLE t DETACH PARTITION p CONCURRENTLY; COMMIT", flagged: []int{2}}, {name: "block never closed", script: "BEGIN; CREATE INDEX CONCURRENTLY i ON t (c)", flagged: []int{2}}, {name: "after a savepoint", script: "BEGIN; SAVEPOINT s; CREATE INDEX CONCURRENTLY i ON t (c); COMMIT", flagged: []int{3}}, diff --git a/pkg/lint/transaction_integration_test.go b/pkg/lint/transaction_integration_test.go index 285c4849..f19a6296 100644 --- a/pkg/lint/transaction_integration_test.go +++ b/pkg/lint/transaction_integration_test.go @@ -28,6 +28,7 @@ func TestServerRefusesWhatConcurrentInTransactionFlags(t *testing.T) { "CREATE INDEX CONCURRENTLY t_c_new ON %[1]s.t (c)", "DROP INDEX CONCURRENTLY %[1]s.t_c_idx", "REINDEX INDEX CONCURRENTLY %[1]s.t_c_idx", + "REINDEX (CONCURRENTLY true) INDEX %[1]s.t_c_idx", "ALTER TABLE %[1]s.p DETACH PARTITION %[1]s.p_low CONCURRENTLY", } { t.Run(sql, func(t *testing.T) { @@ -61,6 +62,44 @@ func TestServerRefusesWhatConcurrentInTransactionFlags(t *testing.T) { } } +// A REINDEX whose CONCURRENTLY option is switched off rebuilds with the +// blocking lock, which PostgreSQL runs inside a block. Lint leaves it +// unflagged, and the server runs it. +func TestServerRunsAReindexWithConcurrentlyOff(t *testing.T) { + url := testutil.StartPostgres(t) + pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: url}) + require.NoError(t, err) + t.Cleanup(pool.Close) + + for _, sql := range []string{ + "REINDEX (CONCURRENTLY false) INDEX %[1]s.t_c_idx", + "REINDEX (CONCURRENTLY 0) INDEX %[1]s.t_c_idx", + "REINDEX (CONCURRENTLY off) INDEX %[1]s.t_c_idx", + } { + t.Run(sql, func(t *testing.T) { + schema := testutil.NewSchema(t, pool) + for _, ddl := range []string{"CREATE TABLE %[1]s.t (c int)", "CREATE INDEX t_c_idx ON %[1]s.t (c)"} { + _, err := pool.Exec(t.Context(), fmt.Sprintf(ddl, schema)) + require.NoError(t, err) + } + stmt := fmt.Sprintf(sql, schema) + report, err := lint.Check("BEGIN; " + stmt + "; COMMIT") + require.NoError(t, err) + assert.False(t, hasCode(report, 2, lint.CodeConcurrentInTransaction), "lint leaves %s unflagged", stmt) + + conn, err := pool.Acquire(t.Context()) + require.NoError(t, err) + defer conn.Release() + _, err = conn.Exec(t.Context(), "BEGIN") + require.NoError(t, err) + _, err = conn.Exec(t.Context(), stmt) + require.NoError(t, err, "the server runs %s inside a block", stmt) + _, err = conn.Exec(t.Context(), "COMMIT") + require.NoError(t, err) + }) + } +} + func hasCode(report lint.Report, stmt int, code lint.Code) bool { for _, f := range report.Findings { if f.Statement == stmt && f.Code == code { diff --git a/pkg/statement/ops.go b/pkg/statement/ops.go index 5eb5980b..ef984af4 100644 --- a/pkg/statement/ops.go +++ b/pkg/statement/ops.go @@ -367,7 +367,7 @@ func ParseOps(sql string) ([]Op, error) { return []Op{{ Kind: OpReindex, Name: re.GetRelation().GetRelname(), - Concurrent: reindexConcurrent(re), + Concurrent: reindexConcurrently(re), }}, nil case node.GetRenameStmt() != nil: return []Op{renameOp(node.GetRenameStmt())}, nil @@ -618,17 +618,6 @@ func renameOp(ren *pganalyze.RenameStmt) Op { } } -// reindexConcurrent reports whether a REINDEX statement carries the -// CONCURRENTLY option (a DefElem in the statement's parameter list). -func reindexConcurrent(re *pganalyze.ReindexStmt) bool { - for _, p := range re.GetParams() { - if p.GetDefElem().GetDefname() == "concurrently" { - return true - } - } - return false -} - // typeRef returns the bare grammar type name (last path element, without // the pg_catalog qualification) and its integer modifiers. func typeRef(tn *pganalyze.TypeName) (string, []int32) { diff --git a/pkg/statement/ops_test.go b/pkg/statement/ops_test.go index 2093ba79..8b21b389 100644 --- a/pkg/statement/ops_test.go +++ b/pkg/statement/ops_test.go @@ -1,6 +1,7 @@ package statement_test import ( + "strings" "testing" "github.com/stretchr/testify/assert" @@ -307,6 +308,31 @@ func TestParseOpsShapes(t *testing.T) { sql: "REINDEX INDEX CONCURRENTLY i", want: statement.Op{Kind: statement.OpReindex, Name: "i", Concurrent: true}, }, + { + name: "reindex with concurrently true", + sql: "REINDEX (CONCURRENTLY TRUE) INDEX i", + want: statement.Op{Kind: statement.OpReindex, Name: "i", Concurrent: true}, + }, + { + name: "reindex with concurrently 1", + sql: "REINDEX (CONCURRENTLY 1) INDEX i", + want: statement.Op{Kind: statement.OpReindex, Name: "i", Concurrent: true}, + }, + { + name: "reindex with concurrently false", + sql: "REINDEX (CONCURRENTLY false) INDEX i", + want: statement.Op{Kind: statement.OpReindex, Name: "i"}, + }, + { + name: "reindex with concurrently off", + sql: "REINDEX (CONCURRENTLY Off) INDEX i", + want: statement.Op{Kind: statement.OpReindex, Name: "i"}, + }, + { + name: "reindex with concurrently 0", + sql: "REINDEX (CONCURRENTLY 0) INDEX i", + want: statement.Op{Kind: statement.OpReindex, Name: "i"}, + }, { name: "create table", sql: "CREATE TABLE t (id int PRIMARY KEY)", @@ -415,6 +441,23 @@ func TestConcurrentlyRewrites(t *testing.T) { } } +// A REINDEX with CONCURRENTLY switched off is rewritten to the concurrent +// rebuild by replacing the option, never by adding a second one, which the +// server refuses as a conflicting option. +func TestConcurrentlyReplacesADisabledReindexOption(t *testing.T) { + for _, sql := range []string{"REINDEX (CONCURRENTLY false) INDEX i", "REINDEX (VERBOSE, CONCURRENTLY 0) INDEX i"} { + t.Run(sql, func(t *testing.T) { + safer, err := statement.Concurrently(sql) + require.NoError(t, err) + assert.True(t, parseOneOp(t, safer).Concurrent, "rewritten statement must be concurrent: %s", safer) + assert.Equal(t, 1, strings.Count(strings.ToUpper(safer), "CONCURRENTLY"), "one CONCURRENTLY option: %s", safer) + if strings.Contains(sql, "VERBOSE") { + assert.Contains(t, strings.ToUpper(safer), "VERBOSE", "other options survive: %s", safer) + } + }) + } +} + func TestConcurrentlyRefusesOtherStatements(t *testing.T) { _, err := statement.Concurrently("ALTER TABLE t ADD COLUMN a int") assert.ErrorIs(t, err, statement.ErrNotRewritable) diff --git a/pkg/statement/rewrite.go b/pkg/statement/rewrite.go index 2adaa5bd..f17e442c 100644 --- a/pkg/statement/rewrite.go +++ b/pkg/statement/rewrite.go @@ -3,6 +3,7 @@ package statement import ( "errors" "fmt" + "slices" pganalyze "github.com/pganalyze/pg_query_go/v6" pgquery "github.com/wasilibs/go-pgquery" @@ -36,7 +37,10 @@ func Concurrently(sql string) (string, error) { node.GetDropStmt().Concurrent = true case node.GetReindexStmt() != nil: re := node.GetReindexStmt() - if !reindexConcurrent(re) { + if !reindexConcurrently(re) { + // A CONCURRENTLY false is replaced, not joined: the server + // refuses a repeated option. + re.Params = slices.DeleteFunc(re.Params, isConcurrentlyOption) re.Params = append(re.Params, &pganalyze.Node{ Node: &pganalyze.Node_DefElem{DefElem: &pganalyze.DefElem{Defname: "concurrently"}}, }) diff --git a/pkg/statement/statement.go b/pkg/statement/statement.go index 77e75ae7..a284a231 100644 --- a/pkg/statement/statement.go +++ b/pkg/statement/statement.go @@ -386,14 +386,44 @@ func reindexedRelation(rel *pganalyze.RangeVar, kind IndexRelationKind) IndexRel return IndexRelation{Schema: rel.GetSchemaname(), Name: rel.GetRelname(), Kind: kind} } -// reindexConcurrently reports whether a REINDEX statement used its -// CONCURRENTLY form, which the grammar carries as a generic option rather -// than a dedicated field. +// reindexConcurrently reports whether a REINDEX statement runs +// concurrently. The grammar carries CONCURRENTLY as a generic option, and +// the server reads a value given with it as a boolean: CONCURRENTLY, +// CONCURRENTLY true, and CONCURRENTLY 1 rebuild concurrently, while +// CONCURRENTLY false and CONCURRENTLY 0 rebuild with the blocking lock. The +// last occurrence decides, though the server refuses a repeated option. func reindexConcurrently(stmt *pganalyze.ReindexStmt) bool { + concurrent := false for _, p := range stmt.GetParams() { - if strings.EqualFold(p.GetDefElem().GetDefname(), "concurrently") { - return true + if isConcurrentlyOption(p) { + concurrent = optionEnabled(p.GetDefElem().GetArg()) } } - return false + return concurrent +} + +// isConcurrentlyOption reports whether a REINDEX parameter is the +// CONCURRENTLY option. +func isConcurrentlyOption(p *pganalyze.Node) bool { + return strings.EqualFold(p.GetDefElem().GetDefname(), "concurrently") +} + +// optionEnabled reads a boolean option's value as the server's +// defGetBoolean does: no value, a nonzero integer, or true or on in any +// case enable it; zero, false, and off disable it. Any other value the +// server rejects outright, and it reads as enabled here, the reading that +// flags and refuses more. +func optionEnabled(arg *pganalyze.Node) bool { + switch { + case arg == nil: + return true + case arg.GetInteger() != nil: + return arg.GetInteger().GetIval() != 0 + case arg.GetString_() != nil: + switch strings.ToLower(arg.GetString_().GetSval()) { + case "false", "off": + return false + } + } + return true } From 302da95e966b5445560c5b312afc60abfe80074e Mon Sep 17 00:00:00 2001 From: Armand Parajon Date: Sun, 18 Oct 2026 18:37:09 -0400 Subject: [PATCH 3/3] docs(lint): name the concurrent-in-transaction error in the design doc's lint row Co-Authored-By: Claude Opus 5.5 --- docs/low-level-design.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/low-level-design.md b/docs/low-level-design.md index c438d6b8..d7d3e144 100644 --- a/docs/low-level-design.md +++ b/docs/low-level-design.md @@ -347,7 +347,7 @@ Classification belongs to `pkg/planner`; `pkg/statement` supplies typed operatio | Invocation | Behaviour | | --- | --- | -| `lint` | Offline (no database): classify every statement with zero live facts and report typed findings — unsupported operations are errors (non-zero exit), blocking idioms (with the safer SQL), conservative rewrites, and destructive drops are warnings. | +| `lint` | Offline (no database): classify every statement with zero live facts and report typed findings — unsupported operations and a `CONCURRENTLY` operation inside an explicit `BEGIN` … `COMMIT` block, which PostgreSQL refuses, are errors (non-zero exit), blocking idioms (with the safer SQL), conservative rewrites, and destructive drops are warnings. | | `diff` / `migrate --dry-run` | Print the classified, routed plan and safer SQL where applicable. **Never writes the live table.** `diff` has no `--dry-run` flag because it never executes the plan (its desired-state diff does run the desired DDL in an always-rolled-back scratch transaction — see the scratch-schema note above). | | `migrate` (default) | Run the statement gate, classify and route exactly as dry-run would, then execute the routed SQL: the planner's **safer native sequence by default** when the submitted form blocks, the submitted form as a bounded optimistic attempt otherwise. The verdict's `executed_sql` reports the substitution. | | `migrate --force` | Run the statement **exactly as submitted**, overriding a safer-sequence substitution or a rewrite-required / backend-unavailable refusal. Gated — see below. |