plan: expose can_run_in_transaction on JSON Step - #524
Conversation
Add CanRunInTransaction bool to the Step struct so consumers of `plan --output-json` can determine which steps require isolation without grepping SQL for CONCURRENTLY or inferring it from wait directives. The data was already computed internally for execution grouping; this just surfaces it in the JSON output. Also adds a backward-compat fixup in FromJSON: old plan files that lack the field will default to true for non-CONCURRENTLY steps, since canonical statements are always transactional. Closes #523
Greptile SummaryExposes per-step transaction eligibility in plan JSON.
Confidence Score: 4/5The explicit-false deserialization defect should be fixed before merging because it can silently reverse transaction-safety metadata supplied by a plan. FromJSON conflates an absent boolean with an explicit false and conditionally rewrites both to true based on SQL text, breaking round-trip preservation of the newly exposed field. Files Needing Attention: internal/plan/plan.go Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Plan JSON step] --> B[Unmarshal boolean]
B --> C{Value is false?}
C -- No --> D[Preserve true]
C -- Yes --> E{SQL contains uppercase CONCURRENTLY?}
E -- Yes --> F[Preserve false]
E -- No --> G[Replace with true]
H[Explicit false] --> B
I[Missing field] --> B
Reviews (1): Last reviewed commit: "plan: expose can_run_in_transaction on J..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if !s.CanRunInTransaction && !strings.Contains(s.SQL, "CONCURRENTLY") { | ||
| s.CanRunInTransaction = true |
There was a problem hiding this comment.
When a current-format plan explicitly sets can_run_in_transaction to false for SQL without the exact uppercase substring CONCURRENTLY, FromJSON changes the value to true because an omitted boolean and an explicit false are indistinguishable after unmarshalling, causing consumers to receive transaction-safety metadata that contradicts the input plan.
There was a problem hiding this comment.
Pull request overview
This PR extends the plan --output-json contract by exposing can_run_in_transaction on each JSON Step, leveraging the already-computed RewriteStep.CanRunInTransaction so external consumers (e.g., migration generators like goose) can reliably determine when a step must run outside a transaction.
Changes:
- Added
CanRunInTransactiontointernal/plan.Stepwith JSON tagcan_run_in_transaction. - Propagated the value from rewrite steps during rewrite→step conversion in
groupDiffs; canonical statements default totrue. - Updated migrate plan JSON fixtures (v1–v5) to include the new field.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/plan/plan.go | Adds can_run_in_transaction to plan JSON steps, wires it through rewrite grouping, and adds backward-compat handling when loading plans from JSON. |
| testdata/diff/migrate/v1/plan.json | Updates fixture to include can_run_in_transaction on steps. |
| testdata/diff/migrate/v2/plan.json | Updates fixture to include can_run_in_transaction on steps (including false for CONCURRENTLY steps). |
| testdata/diff/migrate/v3/plan.json | Updates fixture to include can_run_in_transaction on steps. |
| testdata/diff/migrate/v4/plan.json | Updates fixture to include can_run_in_transaction on steps (including false for CONCURRENTLY steps). |
| testdata/diff/migrate/v5/plan.json | Updates fixture to include can_run_in_transaction on steps. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Backward compat: old plans may lack can_run_in_transaction. | ||
| // JSON unmarshaling defaults missing bools to false, but the correct | ||
| // default for most steps is true (only CONCURRENTLY steps are non-transactional). | ||
| // We detect missing-by-absence-of-CONCURRENTLY, which covers the canonical case. | ||
| for i := range plan.Groups { | ||
| for j := range plan.Groups[i].Steps { | ||
| s := &plan.Groups[i].Steps[j] | ||
| if !s.CanRunInTransaction && !strings.Contains(s.SQL, "CONCURRENTLY") { | ||
| s.CanRunInTransaction = true | ||
| } | ||
| } | ||
| } |
Closes #523.
What
Adds
can_run_in_transaction(bool) to theStepstruct inplan --output-json. The data was already computed internally (RewriteStep.CanRunInTransaction) for execution grouping — this just surfaces it so consumers can determine which steps must run outside a transaction.Why
When generating migration files from plan JSON (e.g., goose), consumers need to know which statements require
-- +goose NO TRANSACTION. Currently the only signals are grepping SQL forCONCURRENTLYor inferring isolation from adjacentwaitdirectives — both fragile.Changes
CanRunInTransaction booltoStepstruct with JSON tagcan_run_in_transactiongroupDiffs)truefor canonical statements (always transactional)FromJSONhandles old plan files missing the field