From 00bd4ca1d8cb8038d3674aef490f37896926a07d Mon Sep 17 00:00:00 2001 From: Tianzhou Date: Sat, 7 Mar 2026 18:44:02 -0800 Subject: [PATCH 1/3] feat: add support for trigger UPDATE OF columns (#342) Triggers with column-specific UPDATE events (e.g., UPDATE OF email) were losing the column specification during inspection, causing incorrect migration plans that would fire triggers on all updates instead of only on specified column changes. Co-Authored-By: Claude Opus 4.6 --- internal/diff/trigger.go | 16 ++++- ir/inspector.go | 66 +++++++++++++++++++ ir/ir.go | 5 +- .../diff/create_trigger/add_trigger/diff.sql | 5 ++ .../diff/create_trigger/add_trigger/new.sql | 5 ++ .../diff/create_trigger/add_trigger/plan.json | 6 ++ .../diff/create_trigger/add_trigger/plan.sql | 5 ++ .../diff/create_trigger/add_trigger/plan.txt | 6 ++ .../create_trigger/alter_trigger/diff.sql | 2 +- .../diff/create_trigger/alter_trigger/new.sql | 2 +- .../create_trigger/alter_trigger/plan.json | 2 +- .../create_trigger/alter_trigger/plan.sql | 2 +- .../create_trigger/alter_trigger/plan.txt | 2 +- 13 files changed, 116 insertions(+), 8 deletions(-) diff --git a/internal/diff/trigger.go b/internal/diff/trigger.go index b9a7de11..e11650cb 100644 --- a/internal/diff/trigger.go +++ b/internal/diff/trigger.go @@ -46,6 +46,16 @@ func triggersEqual(old, new *ir.Trigger) bool { } } + // Compare update columns + if len(old.UpdateColumns) != len(new.UpdateColumns) { + return false + } + for i, col := range old.UpdateColumns { + if col != new.UpdateColumns[i] { + return false + } + } + // Compare constraint trigger properties if old.IsConstraint != new.IsConstraint { return false @@ -215,7 +225,11 @@ func generateTriggerSQLWithMode(trigger *ir.Trigger, targetSchema string) string for _, orderEvent := range eventOrder { for _, triggerEvent := range trigger.Events { if triggerEvent == orderEvent { - events = append(events, string(triggerEvent)) + if triggerEvent == ir.TriggerEventUpdate && len(trigger.UpdateColumns) > 0 { + events = append(events, "UPDATE OF "+strings.Join(trigger.UpdateColumns, ", ")) + } else { + events = append(events, string(triggerEvent)) + } break } } diff --git a/ir/inspector.go b/ir/inspector.go index 1e7e1dbc..7a6206ba 100644 --- a/ir/inspector.go +++ b/ir/inspector.go @@ -1452,6 +1452,65 @@ func extractWhenClauseFromTriggerDef(triggerDef string) string { return strings.TrimSpace(triggerDef[start:end]) } +// extractUpdateColumnsFromTriggerDef extracts column names from UPDATE OF col1, col2 in a trigger definition +// returned by pg_get_triggerdef(). The format is: +// "CREATE TRIGGER name BEFORE INSERT OR UPDATE OF col1, col2 ON table ..." +func extractUpdateColumnsFromTriggerDef(triggerDef string) []string { + upper := strings.ToUpper(triggerDef) + updateOfIdx := strings.Index(upper, "UPDATE OF ") + if updateOfIdx == -1 { + return nil + } + + // Start after "UPDATE OF " + start := updateOfIdx + len("UPDATE OF ") + + // Find " ON " which terminates the event/column list + onIdx := strings.Index(upper[start:], " ON ") + if onIdx == -1 { + return nil + } + + // Extract the column list substring + colListStr := strings.TrimSpace(triggerDef[start : start+onIdx]) + + // Handle potential " OR " separating additional events after the columns + // e.g., "UPDATE OF col1, col2 OR DELETE ON ..." + // We need to check if there's an OR followed by another event keyword + eventKeywords := []string{"INSERT", "UPDATE", "DELETE", "TRUNCATE"} + parts := strings.Split(colListStr, " OR ") + // Only keep the first part (column list); the rest would be other events + if len(parts) > 1 { + // Check if subsequent parts are event keywords + for i := 1; i < len(parts); i++ { + trimmed := strings.TrimSpace(strings.ToUpper(parts[i])) + isEvent := false + for _, kw := range eventKeywords { + if trimmed == kw || strings.HasPrefix(trimmed, kw+" ") { + isEvent = true + break + } + } + if isEvent { + colListStr = strings.TrimSpace(parts[0]) + break + } + } + } + + // Split by comma and trim each column name + rawCols := strings.Split(colListStr, ",") + var columns []string + for _, col := range rawCols { + col = strings.TrimSpace(col) + if col != "" { + columns = append(columns, col) + } + } + + return columns +} + // extractFunctionCallFromTriggerDef extracts the function call (with arguments) from a trigger definition // returned by pg_get_triggerdef(). The format is: // "... EXECUTE FUNCTION function_name(arg1, arg2)" @@ -1548,6 +1607,12 @@ func (i *Inspector) buildTriggers(ctx context.Context, schema *IR, targetSchema condition = extractWhenClauseFromTriggerDef(triggerRow.TriggerDefinition.String) } + // Extract UPDATE OF columns from trigger definition + var updateColumns []string + if triggerRow.TriggerDefinition.Valid { + updateColumns = extractUpdateColumnsFromTriggerDef(triggerRow.TriggerDefinition.String) + } + // Extract transition table names oldTable := "" if triggerRow.OldTable.Valid { @@ -1577,6 +1642,7 @@ func (i *Inspector) buildTriggers(ctx context.Context, schema *IR, targetSchema Table: tableName, Timing: timing, Events: events, + UpdateColumns: updateColumns, Level: level, Function: functionCall, Condition: condition, diff --git a/ir/ir.go b/ir/ir.go index 82976e45..ebe18013 100644 --- a/ir/ir.go +++ b/ir/ir.go @@ -278,8 +278,9 @@ type Trigger struct { Table string `json:"table"` Name string `json:"name"` Timing TriggerTiming `json:"timing"` // BEFORE, AFTER, INSTEAD OF - Events []TriggerEvent `json:"events"` // INSERT, UPDATE, DELETE - Level TriggerLevel `json:"level"` // ROW, STATEMENT + Events []TriggerEvent `json:"events"` // INSERT, UPDATE, DELETE + UpdateColumns []string `json:"update_columns,omitempty"` // Column names for UPDATE OF + Level TriggerLevel `json:"level"` // ROW, STATEMENT Function string `json:"function"` Condition string `json:"condition,omitempty"` // WHEN condition Comment string `json:"comment,omitempty"` diff --git a/testdata/diff/create_trigger/add_trigger/diff.sql b/testdata/diff/create_trigger/add_trigger/diff.sql index e8b5b456..6f7c2e73 100644 --- a/testdata/diff/create_trigger/add_trigger/diff.sql +++ b/testdata/diff/create_trigger/add_trigger/diff.sql @@ -8,6 +8,11 @@ CREATE OR REPLACE TRIGGER employees_last_modified_trigger FOR EACH ROW EXECUTE FUNCTION update_last_modified(); +CREATE OR REPLACE TRIGGER employees_salary_update_trigger + BEFORE UPDATE OF salary ON employees + FOR EACH ROW + EXECUTE FUNCTION update_last_modified(); + CREATE OR REPLACE TRIGGER employees_truncate_log_trigger AFTER TRUNCATE ON employees FOR EACH STATEMENT diff --git a/testdata/diff/create_trigger/add_trigger/new.sql b/testdata/diff/create_trigger/add_trigger/new.sql index ff849047..6453df3f 100644 --- a/testdata/diff/create_trigger/add_trigger/new.sql +++ b/testdata/diff/create_trigger/add_trigger/new.sql @@ -23,6 +23,11 @@ CREATE TRIGGER employees_insert_timestamp_trigger FOR EACH ROW EXECUTE FUNCTION public.update_last_modified(); +CREATE TRIGGER employees_salary_update_trigger + BEFORE UPDATE OF salary ON public.employees + FOR EACH ROW + EXECUTE FUNCTION public.update_last_modified(); + CREATE TRIGGER employees_truncate_log_trigger AFTER TRUNCATE ON public.employees FOR EACH STATEMENT diff --git a/testdata/diff/create_trigger/add_trigger/plan.json b/testdata/diff/create_trigger/add_trigger/plan.json index fbbfa04e..cd19a087 100644 --- a/testdata/diff/create_trigger/add_trigger/plan.json +++ b/testdata/diff/create_trigger/add_trigger/plan.json @@ -20,6 +20,12 @@ "operation": "create", "path": "public.employees.employees_last_modified_trigger" }, + { + "sql": "CREATE OR REPLACE TRIGGER employees_salary_update_trigger\n BEFORE UPDATE OF salary ON employees\n FOR EACH ROW\n EXECUTE FUNCTION update_last_modified();", + "type": "table.trigger", + "operation": "create", + "path": "public.employees.employees_salary_update_trigger" + }, { "sql": "CREATE OR REPLACE TRIGGER employees_truncate_log_trigger\n AFTER TRUNCATE ON employees\n FOR EACH STATEMENT\n EXECUTE FUNCTION update_last_modified();", "type": "table.trigger", diff --git a/testdata/diff/create_trigger/add_trigger/plan.sql b/testdata/diff/create_trigger/add_trigger/plan.sql index e8b5b456..6f7c2e73 100644 --- a/testdata/diff/create_trigger/add_trigger/plan.sql +++ b/testdata/diff/create_trigger/add_trigger/plan.sql @@ -8,6 +8,11 @@ CREATE OR REPLACE TRIGGER employees_last_modified_trigger FOR EACH ROW EXECUTE FUNCTION update_last_modified(); +CREATE OR REPLACE TRIGGER employees_salary_update_trigger + BEFORE UPDATE OF salary ON employees + FOR EACH ROW + EXECUTE FUNCTION update_last_modified(); + CREATE OR REPLACE TRIGGER employees_truncate_log_trigger AFTER TRUNCATE ON employees FOR EACH STATEMENT diff --git a/testdata/diff/create_trigger/add_trigger/plan.txt b/testdata/diff/create_trigger/add_trigger/plan.txt index 957b8fa7..a7d32981 100644 --- a/testdata/diff/create_trigger/add_trigger/plan.txt +++ b/testdata/diff/create_trigger/add_trigger/plan.txt @@ -8,6 +8,7 @@ Tables: ~ employees + employees_insert_timestamp_trigger (trigger) + employees_last_modified_trigger (trigger) + + employees_salary_update_trigger (trigger) + employees_truncate_log_trigger (trigger) Views: @@ -27,6 +28,11 @@ CREATE OR REPLACE TRIGGER employees_last_modified_trigger FOR EACH ROW EXECUTE FUNCTION update_last_modified(); +CREATE OR REPLACE TRIGGER employees_salary_update_trigger + BEFORE UPDATE OF salary ON employees + FOR EACH ROW + EXECUTE FUNCTION update_last_modified(); + CREATE OR REPLACE TRIGGER employees_truncate_log_trigger AFTER TRUNCATE ON employees FOR EACH STATEMENT diff --git a/testdata/diff/create_trigger/alter_trigger/diff.sql b/testdata/diff/create_trigger/alter_trigger/diff.sql index 322b5df2..072a1c58 100644 --- a/testdata/diff/create_trigger/alter_trigger/diff.sql +++ b/testdata/diff/create_trigger/alter_trigger/diff.sql @@ -1,5 +1,5 @@ CREATE OR REPLACE TRIGGER employees_last_modified_trigger - BEFORE INSERT OR UPDATE ON employees + BEFORE INSERT OR UPDATE OF salary ON employees FOR EACH ROW WHEN (((NEW.salary IS NOT NULL))) EXECUTE FUNCTION update_last_modified(); diff --git a/testdata/diff/create_trigger/alter_trigger/new.sql b/testdata/diff/create_trigger/alter_trigger/new.sql index 9b37151b..2536cf65 100644 --- a/testdata/diff/create_trigger/alter_trigger/new.sql +++ b/testdata/diff/create_trigger/alter_trigger/new.sql @@ -14,7 +14,7 @@ END; $$ LANGUAGE plpgsql; CREATE TRIGGER employees_last_modified_trigger - BEFORE INSERT OR UPDATE ON public.employees + BEFORE INSERT OR UPDATE OF salary ON public.employees FOR EACH ROW WHEN (NEW.salary IS NOT NULL) EXECUTE FUNCTION public.update_last_modified(); \ No newline at end of file diff --git a/testdata/diff/create_trigger/alter_trigger/plan.json b/testdata/diff/create_trigger/alter_trigger/plan.json index 96bb9663..80ff4e80 100644 --- a/testdata/diff/create_trigger/alter_trigger/plan.json +++ b/testdata/diff/create_trigger/alter_trigger/plan.json @@ -9,7 +9,7 @@ { "steps": [ { - "sql": "CREATE OR REPLACE TRIGGER employees_last_modified_trigger\n BEFORE INSERT OR UPDATE ON employees\n FOR EACH ROW\n WHEN (((NEW.salary IS NOT NULL)))\n EXECUTE FUNCTION update_last_modified();", + "sql": "CREATE OR REPLACE TRIGGER employees_last_modified_trigger\n BEFORE INSERT OR UPDATE OF salary ON employees\n FOR EACH ROW\n WHEN (((NEW.salary IS NOT NULL)))\n EXECUTE FUNCTION update_last_modified();", "type": "table.trigger", "operation": "alter", "path": "public.employees.employees_last_modified_trigger" diff --git a/testdata/diff/create_trigger/alter_trigger/plan.sql b/testdata/diff/create_trigger/alter_trigger/plan.sql index 322b5df2..072a1c58 100644 --- a/testdata/diff/create_trigger/alter_trigger/plan.sql +++ b/testdata/diff/create_trigger/alter_trigger/plan.sql @@ -1,5 +1,5 @@ CREATE OR REPLACE TRIGGER employees_last_modified_trigger - BEFORE INSERT OR UPDATE ON employees + BEFORE INSERT OR UPDATE OF salary ON employees FOR EACH ROW WHEN (((NEW.salary IS NOT NULL))) EXECUTE FUNCTION update_last_modified(); diff --git a/testdata/diff/create_trigger/alter_trigger/plan.txt b/testdata/diff/create_trigger/alter_trigger/plan.txt index abf40f6c..be50e2ad 100644 --- a/testdata/diff/create_trigger/alter_trigger/plan.txt +++ b/testdata/diff/create_trigger/alter_trigger/plan.txt @@ -11,7 +11,7 @@ DDL to be executed: -------------------------------------------------- CREATE OR REPLACE TRIGGER employees_last_modified_trigger - BEFORE INSERT OR UPDATE ON employees + BEFORE INSERT OR UPDATE OF salary ON employees FOR EACH ROW WHEN (((NEW.salary IS NOT NULL))) EXECUTE FUNCTION update_last_modified(); From 8e481e1cb691cbeeb48bec5ea76c6a50ceae54b2 Mon Sep 17 00:00:00 2001 From: Tianzhou Date: Sat, 7 Mar 2026 19:14:29 -0800 Subject: [PATCH 2/3] fix: guard UPDATE OF column extraction with tgtype bitmask check Only extract UPDATE OF columns when the trigger actually has an UPDATE event, preventing false positives if the substring appears elsewhere. Co-Authored-By: Claude Opus 4.6 --- ir/inspector.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ir/inspector.go b/ir/inspector.go index 7a6206ba..36d6aa86 100644 --- a/ir/inspector.go +++ b/ir/inspector.go @@ -1607,9 +1607,9 @@ func (i *Inspector) buildTriggers(ctx context.Context, schema *IR, targetSchema condition = extractWhenClauseFromTriggerDef(triggerRow.TriggerDefinition.String) } - // Extract UPDATE OF columns from trigger definition + // Extract UPDATE OF columns from trigger definition (only for triggers with UPDATE events) var updateColumns []string - if triggerRow.TriggerDefinition.Valid { + if triggerRow.TriggerDefinition.Valid && triggerRow.TriggerType&triggerTypeUpdate != 0 { updateColumns = extractUpdateColumnsFromTriggerDef(triggerRow.TriggerDefinition.String) } From d51911bf2b36f4bce89402ada72c85747c4543fa Mon Sep 17 00:00:00 2001 From: Tianzhou Date: Sun, 8 Mar 2026 05:25:50 -0700 Subject: [PATCH 3/3] fix: clean up leaked cluster-level roles in TestIgnorePrivileges TestIgnorePrivileges creates cluster-level PostgreSQL roles (app_reader, deploy_bot, admin_role) and ALTER DEFAULT PRIVILEGES rules in the shared embedded PG instance. Since Go runs tests alphabetically, these persist and contaminate TestPlanAndApply, causing unexpected GRANT statements in plan output for all table/view/index tests. Co-Authored-By: Claude Opus 4.6 --- cmd/ignore_integration_test.go | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/cmd/ignore_integration_test.go b/cmd/ignore_integration_test.go index 18f5cee1..c06956da 100644 --- a/cmd/ignore_integration_test.go +++ b/cmd/ignore_integration_test.go @@ -1130,6 +1130,40 @@ ALTER DEFAULT PRIVILEGES GRANT ALL ON TABLES TO deploy_bot; t.Error("Plan should not include changes for admin_role (ignored)") } }) + + // Clean up cluster-level objects (roles, default privileges) from sharedEmbeddedPG. + // The plan subtest applies SQL with CREATE ROLE and ALTER DEFAULT PRIVILEGES to the + // shared embedded PG instance. These are cluster-level objects that persist after the + // temp schema is dropped, contaminating subsequent tests (e.g., TestPlanAndApply). + cleanupSharedEmbeddedPG(t) +} + +// cleanupSharedEmbeddedPG removes cluster-level objects (roles, default privileges) +// that were created in sharedEmbeddedPG by privilege tests. +func cleanupSharedEmbeddedPG(t *testing.T) { + t.Helper() + + sharedConn, _, _, _, _, _ := testutil.ConnectToPostgres(t, sharedEmbeddedPG) + defer sharedConn.Close() + + // Must clean up in order: revoke default privileges, revoke object privileges, then drop roles. + // Each statement runs independently since some roles may not exist. + cleanupStatements := []string{ + "ALTER DEFAULT PRIVILEGES REVOKE ALL ON TABLES FROM app_reader", + "ALTER DEFAULT PRIVILEGES REVOKE ALL ON TABLES FROM deploy_bot", + "REASSIGN OWNED BY app_reader TO testuser", + "DROP OWNED BY app_reader", + "REASSIGN OWNED BY deploy_bot TO testuser", + "DROP OWNED BY deploy_bot", + "REASSIGN OWNED BY admin_role TO testuser", + "DROP OWNED BY admin_role", + "DROP ROLE IF EXISTS app_reader", + "DROP ROLE IF EXISTS deploy_bot", + "DROP ROLE IF EXISTS admin_role", + } + for _, stmt := range cleanupStatements { + sharedConn.Exec(stmt) // Ignore errors; some roles may not exist + } } // verifyPlanOutput checks that plan output excludes ignored objects