Skip to content

plan: expose can_run_in_transaction on JSON Step #524

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
tianzhou wants to merge 1 commit into from
+144 −69
Closed

plan: expose can_run_in_transaction on JSON Step #524

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 28 additions & 11 deletions internal/plan/plan.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,9 @@ type Directive struct {

// Step represents a single execution step with SQL and optional directive
type Step struct {
SQL string `json:"sql"`
Directive *Directive `json:"directive,omitempty"`
SQL string `json:"sql"`
Directive *Directive `json:"directive,omitempty"`
CanRunInTransaction bool `json:"can_run_in_transaction"`
// Metadata for summary generation
Type string `json:"type,omitempty"` // e.g., "table", "index"
Operation string `json:"operation,omitempty"` // e.g., "create", "alter", "drop"
Expand Down Expand Up @@ -174,11 +175,12 @@ func groupDiffs(diffs []diff.Diff) []ExecutionGroup {
// For operations with rewrites, create one step per rewrite statement
for _, rewriteStep := range rewriteSteps {
step := Step{
SQL: rewriteStep.SQL,
Type: d.Type.String(),
Operation: d.Operation.String(),
Path: d.Path,
Directive: rewriteStep.Directive,
SQL: rewriteStep.SQL,
Type: d.Type.String(),
Operation: d.Operation.String(),
Path: d.Path,
Directive: rewriteStep.Directive,
CanRunInTransaction: rewriteStep.CanRunInTransaction,
}

// Check if this step needs isolation (has directive or cannot run in transaction)
Expand All @@ -202,10 +204,11 @@ func groupDiffs(diffs []diff.Diff) []ExecutionGroup {
// For operations without rewrites, create one step per canonical statement
for _, stmt := range d.Statements {
step := Step{
SQL: stmt.SQL,
Type: d.Type.String(),
Operation: d.Operation.String(),
Path: d.Path,
SQL: stmt.SQL,
Type: d.Type.String(),
Operation: d.Operation.String(),
Path: d.Path,
CanRunInTransaction: true,
}
// Canonical statements don't have directives
transactionalSteps = append(transactionalSteps, step)
Expand Down Expand Up @@ -385,6 +388,20 @@ func FromJSON(jsonData []byte) (*Plan, error) {
if err := json.Unmarshal(jsonData, &plan); err != nil {
return nil, fmt.Errorf("failed to unmarshal plan JSON: %w", err)
}

// 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
Comment on lines +399 to +400

@greptile-apps greptile-apps Bot Aug 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Explicit false values are overwritten

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.

}
}
}
Comment on lines +392 to +403

return &plan, nil
}

Expand Down
18 changes: 12 additions & 6 deletions testdata/diff/migrate/v1/plan.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,37 +12,43 @@
"sql": "CREATE TABLE IF NOT EXISTS department (\n dept_no text,\n dept_name text NOT NULL,\n CONSTRAINT department_pkey PRIMARY KEY (dept_no)\n);",
"type": "table",
"operation": "create",
"path": "public.department"
"path": "public.department",
"can_run_in_transaction": true
},
{
"sql": "CREATE TABLE IF NOT EXISTS employee (\n emp_no SERIAL,\n birth_date date NOT NULL,\n first_name text NOT NULL,\n last_name text NOT NULL,\n gender text NOT NULL,\n hire_date date NOT NULL,\n CONSTRAINT employee_pkey PRIMARY KEY (emp_no)\n);",
"type": "table",
"operation": "create",
"path": "public.employee"
"path": "public.employee",
"can_run_in_transaction": true
},
{
"sql": "CREATE TABLE IF NOT EXISTS dept_emp (\n emp_no integer,\n dept_no text,\n from_date date NOT NULL,\n to_date date NOT NULL,\n CONSTRAINT dept_emp_pkey PRIMARY KEY (emp_no, dept_no),\n CONSTRAINT dept_emp_dept_no_fkey FOREIGN KEY (dept_no) REFERENCES department (dept_no),\n CONSTRAINT dept_emp_emp_no_fkey FOREIGN KEY (emp_no) REFERENCES employee (emp_no)\n);",
"type": "table",
"operation": "create",
"path": "public.dept_emp"
"path": "public.dept_emp",
"can_run_in_transaction": true
},
{
"sql": "CREATE TABLE IF NOT EXISTS dept_manager (\n emp_no integer,\n dept_no text,\n from_date date NOT NULL,\n to_date date NOT NULL,\n CONSTRAINT dept_manager_pkey PRIMARY KEY (emp_no, dept_no),\n CONSTRAINT dept_manager_dept_no_fkey FOREIGN KEY (dept_no) REFERENCES department (dept_no),\n CONSTRAINT dept_manager_emp_no_fkey FOREIGN KEY (emp_no) REFERENCES employee (emp_no)\n);",
"type": "table",
"operation": "create",
"path": "public.dept_manager"
"path": "public.dept_manager",
"can_run_in_transaction": true
},
{
"sql": "CREATE TABLE IF NOT EXISTS salary (\n emp_no integer,\n amount integer NOT NULL,\n from_date date,\n to_date date NOT NULL,\n CONSTRAINT salary_pkey PRIMARY KEY (emp_no, from_date),\n CONSTRAINT salary_emp_no_fkey FOREIGN KEY (emp_no) REFERENCES employee (emp_no)\n);",
"type": "table",
"operation": "create",
"path": "public.salary"
"path": "public.salary",
"can_run_in_transaction": true
},
{
"sql": "CREATE TABLE IF NOT EXISTS title (\n emp_no integer,\n title text,\n from_date date,\n to_date date,\n CONSTRAINT title_pkey PRIMARY KEY (emp_no, title, from_date),\n CONSTRAINT title_emp_no_fkey FOREIGN KEY (emp_no) REFERENCES employee (emp_no)\n);",
"type": "table",
"operation": "create",
"path": "public.title"
"path": "public.title",
"can_run_in_transaction": true
}
]
}
Expand Down
75 changes: 50 additions & 25 deletions testdata/diff/migrate/v2/plan.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,91 +12,106 @@
"sql": "ALTER TABLE department\nADD CONSTRAINT department_dept_name_key UNIQUE (dept_name);",
"type": "table.constraint",
"operation": "create",
"path": "public.department.department_dept_name_key"
"path": "public.department.department_dept_name_key",
"can_run_in_transaction": true
},
{
"sql": "ALTER TABLE dept_emp DROP CONSTRAINT dept_emp_dept_no_fkey;",
"type": "table.constraint",
"operation": "drop",
"path": "public.dept_emp.dept_emp_dept_no_fkey"
"path": "public.dept_emp.dept_emp_dept_no_fkey",
"can_run_in_transaction": true
},
{
"sql": "ALTER TABLE dept_emp\nADD CONSTRAINT dept_emp_dept_no_fkey FOREIGN KEY (dept_no) REFERENCES department (dept_no) ON DELETE CASCADE NOT VALID;",
"type": "table.constraint",
"operation": "create",
"path": "public.dept_emp.dept_emp_dept_no_fkey"
"path": "public.dept_emp.dept_emp_dept_no_fkey",
"can_run_in_transaction": true
},
{
"sql": "ALTER TABLE dept_emp VALIDATE CONSTRAINT dept_emp_dept_no_fkey;",
"type": "table.constraint",
"operation": "create",
"path": "public.dept_emp.dept_emp_dept_no_fkey"
"path": "public.dept_emp.dept_emp_dept_no_fkey",
"can_run_in_transaction": true
},
{
"sql": "ALTER TABLE dept_emp DROP CONSTRAINT dept_emp_emp_no_fkey;",
"type": "table.constraint",
"operation": "drop",
"path": "public.dept_emp.dept_emp_emp_no_fkey"
"path": "public.dept_emp.dept_emp_emp_no_fkey",
"can_run_in_transaction": true
},
{
"sql": "ALTER TABLE dept_emp\nADD CONSTRAINT dept_emp_emp_no_fkey FOREIGN KEY (emp_no) REFERENCES employee (emp_no) ON DELETE CASCADE NOT VALID;",
"type": "table.constraint",
"operation": "create",
"path": "public.dept_emp.dept_emp_emp_no_fkey"
"path": "public.dept_emp.dept_emp_emp_no_fkey",
"can_run_in_transaction": true
},
{
"sql": "ALTER TABLE dept_emp VALIDATE CONSTRAINT dept_emp_emp_no_fkey;",
"type": "table.constraint",
"operation": "create",
"path": "public.dept_emp.dept_emp_emp_no_fkey"
"path": "public.dept_emp.dept_emp_emp_no_fkey",
"can_run_in_transaction": true
},
{
"sql": "ALTER TABLE dept_manager DROP CONSTRAINT dept_manager_dept_no_fkey;",
"type": "table.constraint",
"operation": "drop",
"path": "public.dept_manager.dept_manager_dept_no_fkey"
"path": "public.dept_manager.dept_manager_dept_no_fkey",
"can_run_in_transaction": true
},
{
"sql": "ALTER TABLE dept_manager\nADD CONSTRAINT dept_manager_dept_no_fkey FOREIGN KEY (dept_no) REFERENCES department (dept_no) ON DELETE CASCADE NOT VALID;",
"type": "table.constraint",
"operation": "create",
"path": "public.dept_manager.dept_manager_dept_no_fkey"
"path": "public.dept_manager.dept_manager_dept_no_fkey",
"can_run_in_transaction": true
},
{
"sql": "ALTER TABLE dept_manager VALIDATE CONSTRAINT dept_manager_dept_no_fkey;",
"type": "table.constraint",
"operation": "create",
"path": "public.dept_manager.dept_manager_dept_no_fkey"
"path": "public.dept_manager.dept_manager_dept_no_fkey",
"can_run_in_transaction": true
},
{
"sql": "ALTER TABLE dept_manager DROP CONSTRAINT dept_manager_emp_no_fkey;",
"type": "table.constraint",
"operation": "drop",
"path": "public.dept_manager.dept_manager_emp_no_fkey"
"path": "public.dept_manager.dept_manager_emp_no_fkey",
"can_run_in_transaction": true
},
{
"sql": "ALTER TABLE dept_manager\nADD CONSTRAINT dept_manager_emp_no_fkey FOREIGN KEY (emp_no) REFERENCES employee (emp_no) ON DELETE CASCADE NOT VALID;",
"type": "table.constraint",
"operation": "create",
"path": "public.dept_manager.dept_manager_emp_no_fkey"
"path": "public.dept_manager.dept_manager_emp_no_fkey",
"can_run_in_transaction": true
},
{
"sql": "ALTER TABLE dept_manager VALIDATE CONSTRAINT dept_manager_emp_no_fkey;",
"type": "table.constraint",
"operation": "create",
"path": "public.dept_manager.dept_manager_emp_no_fkey"
"path": "public.dept_manager.dept_manager_emp_no_fkey",
"can_run_in_transaction": true
},
{
"sql": "ALTER TABLE employee\nADD CONSTRAINT employee_gender_check CHECK (gender IN ('M'::text, 'F'::text)) NOT VALID;",
"type": "table.constraint",
"operation": "create",
"path": "public.employee.employee_gender_check"
"path": "public.employee.employee_gender_check",
"can_run_in_transaction": true
},
{
"sql": "ALTER TABLE employee VALIDATE CONSTRAINT employee_gender_check;",
"type": "table.constraint",
"operation": "create",
"path": "public.employee.employee_gender_check"
"path": "public.employee.employee_gender_check",
"can_run_in_transaction": true
}
]
},
Expand All @@ -106,7 +121,8 @@
"sql": "CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_employee_hire_date ON employee (hire_date);",
"type": "table.index",
"operation": "create",
"path": "public.employee.idx_employee_hire_date"
"path": "public.employee.idx_employee_hire_date",
"can_run_in_transaction": false
}
]
},
Expand All @@ -120,7 +136,8 @@
},
"type": "table.index",
"operation": "create",
"path": "public.employee.idx_employee_hire_date"
"path": "public.employee.idx_employee_hire_date",
"can_run_in_transaction": true
}
]
},
Expand All @@ -130,19 +147,22 @@
"sql": "ALTER TABLE salary DROP CONSTRAINT salary_emp_no_fkey;",
"type": "table.constraint",
"operation": "drop",
"path": "public.salary.salary_emp_no_fkey"
"path": "public.salary.salary_emp_no_fkey",
"can_run_in_transaction": true
},
{
"sql": "ALTER TABLE salary\nADD CONSTRAINT salary_emp_no_fkey FOREIGN KEY (emp_no) REFERENCES employee (emp_no) ON DELETE CASCADE NOT VALID;",
"type": "table.constraint",
"operation": "create",
"path": "public.salary.salary_emp_no_fkey"
"path": "public.salary.salary_emp_no_fkey",
"can_run_in_transaction": true
},
{
"sql": "ALTER TABLE salary VALIDATE CONSTRAINT salary_emp_no_fkey;",
"type": "table.constraint",
"operation": "create",
"path": "public.salary.salary_emp_no_fkey"
"path": "public.salary.salary_emp_no_fkey",
"can_run_in_transaction": true
}
]
},
Expand All @@ -152,7 +172,8 @@
"sql": "CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_salary_amount ON salary (amount);",
"type": "table.index",
"operation": "create",
"path": "public.salary.idx_salary_amount"
"path": "public.salary.idx_salary_amount",
"can_run_in_transaction": false
}
]
},
Expand All @@ -166,7 +187,8 @@
},
"type": "table.index",
"operation": "create",
"path": "public.salary.idx_salary_amount"
"path": "public.salary.idx_salary_amount",
"can_run_in_transaction": true
}
]
},
Expand All @@ -176,19 +198,22 @@
"sql": "ALTER TABLE title DROP CONSTRAINT title_emp_no_fkey;",
"type": "table.constraint",
"operation": "drop",
"path": "public.title.title_emp_no_fkey"
"path": "public.title.title_emp_no_fkey",
"can_run_in_transaction": true
},
{
"sql": "ALTER TABLE title\nADD CONSTRAINT title_emp_no_fkey FOREIGN KEY (emp_no) REFERENCES employee (emp_no) ON DELETE CASCADE NOT VALID;",
"type": "table.constraint",
"operation": "create",
"path": "public.title.title_emp_no_fkey"
"path": "public.title.title_emp_no_fkey",
"can_run_in_transaction": true
},
{
"sql": "ALTER TABLE title VALIDATE CONSTRAINT title_emp_no_fkey;",
"type": "table.constraint",
"operation": "create",
"path": "public.title.title_emp_no_fkey"
"path": "public.title.title_emp_no_fkey",
"can_run_in_transaction": true
}
]
}
Expand Down
12 changes: 8 additions & 4 deletions testdata/diff/migrate/v3/plan.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,25 +12,29 @@
"sql": "CREATE TABLE IF NOT EXISTS audit (\n id SERIAL,\n operation text NOT NULL,\n query text,\n user_name text NOT NULL,\n changed_at timestamptz DEFAULT CURRENT_TIMESTAMP,\n CONSTRAINT audit_pkey PRIMARY KEY (id)\n);",
"type": "table",
"operation": "create",
"path": "public.audit"
"path": "public.audit",
"can_run_in_transaction": true
},
{
"sql": "CREATE INDEX IF NOT EXISTS idx_audit_changed_at ON audit (changed_at);",
"type": "table.index",
"operation": "create",
"path": "public.audit.idx_audit_changed_at"
"path": "public.audit.idx_audit_changed_at",
"can_run_in_transaction": true
},
{
"sql": "CREATE OR REPLACE FUNCTION log_dml_operations()\nRETURNS trigger\nLANGUAGE plpgsql\nVOLATILE\nAS $$\nBEGIN\n IF (TG_OP = 'INSERT') THEN\n INSERT INTO audit (operation, query, user_name)\n VALUES ('INSERT', current_query(), current_user);\n RETURN NEW;\n ELSIF (TG_OP = 'UPDATE') THEN\n INSERT INTO audit (operation, query, user_name)\n VALUES ('UPDATE', current_query(), current_user);\n RETURN NEW;\n ELSIF (TG_OP = 'DELETE') THEN\n INSERT INTO audit (operation, query, user_name)\n VALUES ('DELETE', current_query(), current_user);\n RETURN OLD;\n END IF;\n RETURN NULL;\nEND;\n$$;",
"type": "function",
"operation": "create",
"path": "public.log_dml_operations"
"path": "public.log_dml_operations",
"can_run_in_transaction": true
},
{
"sql": "CREATE OR REPLACE TRIGGER salary_log_trigger\n AFTER UPDATE OR DELETE ON salary\n FOR EACH ROW\n EXECUTE FUNCTION log_dml_operations();",
"type": "table.trigger",
"operation": "create",
"path": "public.salary.salary_log_trigger"
"path": "public.salary.salary_log_trigger",
"can_run_in_transaction": true
}
]
}
Expand Down
Loading
Loading