diff --git a/demo/tour.sh b/demo/tour.sh index 32b4700..eef03f0 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 76d1a69..95f6217 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 d72e205..b709626 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/low-level-design.md b/docs/low-level-design.md index c438d6b..d7d3e14 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. | diff --git a/docs/postgres-online-ddl-reference.md b/docs/postgres-online-ddl-reference.md index 326daef..cb48fe8 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 ddd7e38..b0a2797 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 373b1cc..905420a 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 b8e89af..e4466e9 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, @@ -142,13 +149,24 @@ func Check(sql string) (Report, error) { PostgresVersions: planner.RulesPostgresVersions, Findings: []Finding{}, } + inBlock := false var classifier planner.ScriptClassifier for i, stmt := range stmts { - findings, err := checkStatement(&classifier, i+1, stmt) + findings, err := checkStatement(&classifier, 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 { @@ -164,8 +182,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(classifier *planner.ScriptClassifier, 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(classifier *planner.ScriptClassifier, index int, stmt statement.SourceStatement, inBlock bool) ([]Finding, error) { classified, err := classifier.Classify(stmt.SQL) if err != nil { return nil, err @@ -175,6 +196,21 @@ func checkStatement(classifier *planner.ScriptClassifier, index int, stmt statem 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 1478713..0e09f78 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,7 +223,7 @@ 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, @@ -231,6 +231,69 @@ func TestReportJSONCleanFindingsAreEmptyArray(t *testing.T) { }`, 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: "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}}, + {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) +} + // Two unnamed CHECKs on one column in one script: the second finding's // suggestion names its constraint past the first's, as the server would. func TestCheckUnnamedChecksInOneScriptSuggestDistinctNames(t *testing.T) { diff --git a/pkg/lint/transaction_integration_test.go b/pkg/lint/transaction_integration_test.go new file mode 100644 index 0000000..f19a629 --- /dev/null +++ b/pkg/lint/transaction_integration_test.go @@ -0,0 +1,110 @@ +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", + "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) { + 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) + }) + } +} + +// 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 { + return true + } + } + return false +} diff --git a/pkg/statement/ops.go b/pkg/statement/ops.go index 5eb5980..ef984af 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 6b9d9c2..1d33d4d 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 81d3e1f..0876286 100644 --- a/pkg/statement/rewrite.go +++ b/pkg/statement/rewrite.go @@ -40,7 +40,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 c2d3252..bb66417 100644 --- a/pkg/statement/statement.go +++ b/pkg/statement/statement.go @@ -403,14 +403,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 } diff --git a/pkg/statement/transaction.go b/pkg/statement/transaction.go new file mode 100644 index 0000000..8f45444 --- /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 0000000..7226490 --- /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) +}