From 182b29d187cb8319917fc08ad97fc1b89a0fe36c Mon Sep 17 00:00:00 2001 From: Tianzhou Date: Mon, 16 Feb 2026 02:51:04 -0800 Subject: [PATCH 1/3] fix: INSTEAD OF triggers on views missing from dump (#287) INSTEAD OF triggers on views were not being extracted during schema inspection because buildTriggers() only checked the Tables map, not the Views map. This also fixes the latent INSTEAD_OF formatting bug (should be INSTEAD OF with a space) and adds proper schema normalization for view triggers during plan generation. Co-Authored-By: Claude Opus 4.6 --- cmd/plan/plan.go | 9 ++ internal/diff/diff.go | 29 ++++-- internal/diff/trigger.go | 46 +++++++++ internal/diff/view.go | 98 ++++++++++++++++++- internal/dump/formatter.go | 12 +++ ir/inspector.go | 68 ++++++++----- ir/ir.go | 15 +-- .../diff.sql | 4 + .../issue_287_instead_of_view_trigger/new.sql | 23 +++++ .../issue_287_instead_of_view_trigger/old.sql | 18 ++++ .../plan.json | 26 +++++ .../plan.sql | 17 ++++ .../plan.txt | 33 +++++++ 13 files changed, 358 insertions(+), 40 deletions(-) create mode 100644 testdata/diff/create_trigger/issue_287_instead_of_view_trigger/diff.sql create mode 100644 testdata/diff/create_trigger/issue_287_instead_of_view_trigger/new.sql create mode 100644 testdata/diff/create_trigger/issue_287_instead_of_view_trigger/old.sql create mode 100644 testdata/diff/create_trigger/issue_287_instead_of_view_trigger/plan.json create mode 100644 testdata/diff/create_trigger/issue_287_instead_of_view_trigger/plan.sql create mode 100644 testdata/diff/create_trigger/issue_287_instead_of_view_trigger/plan.txt diff --git a/cmd/plan/plan.go b/cmd/plan/plan.go index 6f3855e5..2e172755 100644 --- a/cmd/plan/plan.go +++ b/cmd/plan/plan.go @@ -499,6 +499,15 @@ func normalizeSchemaNames(irData *ir.IR, fromSchema, toSchema string) { } index.Where = replaceString(index.Where) } + + // Normalize schema names in view triggers (e.g., INSTEAD OF triggers) + for _, trigger := range view.Triggers { + if trigger.Schema == fromSchema { + trigger.Schema = toSchema + } + trigger.Function = stripQualifiers(replaceString(trigger.Function)) + trigger.Condition = stripQualifiers(replaceString(trigger.Condition)) + } } // Functions diff --git a/internal/diff/diff.go b/internal/diff/diff.go index 8aa1610f..9a8318a5 100644 --- a/internal/diff/diff.go +++ b/internal/diff/diff.go @@ -25,6 +25,7 @@ const ( DiffTypeTableColumnComment DiffTypeTableIndexComment DiffTypeView + DiffTypeViewTrigger DiffTypeViewComment DiffTypeMaterializedView DiffTypeMaterializedViewComment @@ -67,6 +68,8 @@ func (d DiffType) String() string { return "table.index.comment" case DiffTypeView: return "view" + case DiffTypeViewTrigger: + return "view.trigger" case DiffTypeViewComment: return "view.comment" case DiffTypeMaterializedView: @@ -137,6 +140,8 @@ func (d *DiffType) UnmarshalJSON(data []byte) error { *d = DiffTypeTableIndexComment case "view": *d = DiffTypeView + case "view.trigger": + *d = DiffTypeViewTrigger case "view.comment": *d = DiffTypeViewComment case "materialized_view": @@ -349,10 +354,13 @@ type viewDiff struct { CommentChanged bool OldComment string NewComment string - AddedIndexes []*ir.Index // For materialized views - DroppedIndexes []*ir.Index // For materialized views - ModifiedIndexes []*IndexDiff // For materialized views - RequiresRecreate bool // For materialized views with structural changes that require DROP + CREATE + AddedIndexes []*ir.Index // For materialized views + DroppedIndexes []*ir.Index // For materialized views + ModifiedIndexes []*IndexDiff // For materialized views + AddedTriggers []*ir.Trigger // For INSTEAD OF triggers on views + DroppedTriggers []*ir.Trigger // For INSTEAD OF triggers on views + ModifiedTriggers []*triggerDiff // For INSTEAD OF triggers on views + RequiresRecreate bool // For materialized views with structural changes that require DROP + CREATE } // tableDiff represents changes to a table @@ -813,7 +821,11 @@ func GenerateMigration(oldIR, newIR *ir.IR, targetSchema string) []Diff { } } - if structurallyDifferent || commentChanged || indexesChanged { + // Diff triggers on views (e.g., INSTEAD OF triggers) + addedTriggers, droppedTriggers, modifiedTriggers := diffViewTriggers(oldView, newView) + triggersChanged := len(addedTriggers) > 0 || len(droppedTriggers) > 0 || len(modifiedTriggers) > 0 + + if structurallyDifferent || commentChanged || indexesChanged || triggersChanged { // For materialized views with structural changes, mark for recreation if newView.Materialized && structurallyDifferent { diff.modifiedViews = append(diff.modifiedViews, &viewDiff{ @@ -824,8 +836,11 @@ func GenerateMigration(oldIR, newIR *ir.IR, targetSchema string) []Diff { } else { // For regular views or comment-only changes, use the modify approach viewDiff := &viewDiff{ - Old: oldView, - New: newView, + Old: oldView, + New: newView, + AddedTriggers: addedTriggers, + DroppedTriggers: droppedTriggers, + ModifiedTriggers: modifiedTriggers, } // Check for comment changes diff --git a/internal/diff/trigger.go b/internal/diff/trigger.go index 020d028c..c2eb9b0c 100644 --- a/internal/diff/trigger.go +++ b/internal/diff/trigger.go @@ -242,3 +242,49 @@ func generateTriggerSQLWithMode(trigger *ir.Trigger, targetSchema string) string return stmt } + +// generateCreateViewTriggersSQL generates CREATE TRIGGER statements for view triggers (e.g., INSTEAD OF) +func generateCreateViewTriggersSQL(triggers []*ir.Trigger, targetSchema string, collector *diffCollector) { + sortedTriggers := make([]*ir.Trigger, len(triggers)) + copy(sortedTriggers, triggers) + sort.Slice(sortedTriggers, func(i, j int) bool { + return sortedTriggers[i].Name < sortedTriggers[j].Name + }) + + for _, trigger := range sortedTriggers { + sql := generateTriggerSQLWithMode(trigger, targetSchema) + + context := &diffContext{ + Type: DiffTypeViewTrigger, + Operation: DiffOperationCreate, + Path: fmt.Sprintf("%s.%s.%s", trigger.Schema, trigger.Table, trigger.Name), + Source: trigger, + CanRunInTransaction: true, + } + + collector.collect(context, sql) + } +} + +// generateDropViewTriggersSQL generates DROP TRIGGER statements for view triggers +func generateDropViewTriggersSQL(triggers []*ir.Trigger, targetSchema string, collector *diffCollector) { + sortedTriggers := make([]*ir.Trigger, len(triggers)) + copy(sortedTriggers, triggers) + sort.Slice(sortedTriggers, func(i, j int) bool { + return sortedTriggers[i].Name < sortedTriggers[j].Name + }) + + for _, trigger := range sortedTriggers { + viewName := getTableNameWithSchema(trigger.Schema, trigger.Table, targetSchema) + sql := fmt.Sprintf("DROP TRIGGER IF EXISTS %s ON %s;", trigger.Name, viewName) + + context := &diffContext{ + Type: DiffTypeViewTrigger, + Operation: DiffOperationDrop, + Path: fmt.Sprintf("%s.%s.%s", trigger.Schema, trigger.Table, trigger.Name), + Source: trigger, + CanRunInTransaction: true, + } + collector.collect(context, sql) + } +} diff --git a/internal/diff/view.go b/internal/diff/view.go index b45edefd..0c2aaaba 100644 --- a/internal/diff/view.go +++ b/internal/diff/view.go @@ -77,6 +77,15 @@ func generateCreateViewsSQL(views []*ir.View, targetSchema string, collector *di // Generate index SQL for materialized view indexes - use MaterializedView types generateCreateIndexesSQLWithType(indexList, targetSchema, collector, DiffTypeMaterializedViewIndex, DiffTypeMaterializedViewIndexComment) } + + // Create triggers on this view (e.g., INSTEAD OF triggers) + if len(view.Triggers) > 0 { + triggerList := make([]*ir.Trigger, 0, len(view.Triggers)) + for _, trigger := range view.Triggers { + triggerList = append(triggerList, trigger) + } + generateCreateViewTriggersSQL(triggerList, targetSchema, collector) + } } } @@ -227,8 +236,12 @@ func generateModifyViewsSQL(diffs []*viewDiff, targetSchema string, collector *d hasIndexChanges := len(diff.AddedIndexes) > 0 || len(diff.DroppedIndexes) > 0 || len(diff.ModifiedIndexes) > 0 indexOnlyChange := diff.New.Materialized && hasIndexChanges && definitionsEqual && !diff.CommentChanged - // Handle comment-only or index-only changes - if commentOnlyChange || indexOnlyChange { + // Check if only triggers changed (for INSTEAD OF triggers on views) + hasTriggerChanges := len(diff.AddedTriggers) > 0 || len(diff.DroppedTriggers) > 0 || len(diff.ModifiedTriggers) > 0 + triggerOnlyChange := hasTriggerChanges && definitionsEqual && !diff.CommentChanged && !hasIndexChanges + + // Handle non-structural changes (comment-only, index-only, or trigger-only) + if commentOnlyChange || indexOnlyChange || triggerOnlyChange { // Only generate COMMENT ON VIEW statement if comment actually changed if diff.CommentChanged { viewName := qualifyEntityName(diff.New.Schema, diff.New.Name, targetSchema) @@ -332,6 +345,48 @@ func generateModifyViewsSQL(diffs []*viewDiff, targetSchema string, collector *d generateCreateIndexesSQLWithType(indexList, targetSchema, collector, DiffTypeMaterializedViewIndex, DiffTypeMaterializedViewIndexComment) } } + + // Handle trigger changes (e.g., INSTEAD OF triggers) - applies to both branches above + if len(diff.DroppedTriggers) > 0 { + generateDropViewTriggersSQL(diff.DroppedTriggers, targetSchema, collector) + } + if len(diff.AddedTriggers) > 0 { + generateCreateViewTriggersSQL(diff.AddedTriggers, targetSchema, collector) + } + for _, triggerDiff := range diff.ModifiedTriggers { + if triggerDiff.New.IsConstraint { + viewName := getTableNameWithSchema(diff.New.Schema, diff.New.Name, targetSchema) + dropSQL := fmt.Sprintf("DROP TRIGGER IF EXISTS %s ON %s;", triggerDiff.Old.Name, viewName) + dropContext := &diffContext{ + Type: DiffTypeViewTrigger, + Operation: DiffOperationDrop, + Path: fmt.Sprintf("%s.%s.%s", diff.New.Schema, diff.New.Name, triggerDiff.Old.Name), + Source: triggerDiff.Old, + CanRunInTransaction: true, + } + collector.collect(dropContext, dropSQL) + + createSQL := generateTriggerSQLWithMode(triggerDiff.New, targetSchema) + createContext := &diffContext{ + Type: DiffTypeViewTrigger, + Operation: DiffOperationCreate, + Path: fmt.Sprintf("%s.%s.%s", diff.New.Schema, diff.New.Name, triggerDiff.New.Name), + Source: triggerDiff.New, + CanRunInTransaction: true, + } + collector.collect(createContext, createSQL) + } else { + sql := generateTriggerSQLWithMode(triggerDiff.New, targetSchema) + context := &diffContext{ + Type: DiffTypeViewTrigger, + Operation: DiffOperationAlter, + Path: fmt.Sprintf("%s.%s.%s", diff.New.Schema, diff.New.Name, triggerDiff.New.Name), + Source: triggerDiff.New, + CanRunInTransaction: true, + } + collector.collect(context, sql) + } + } } // Phase 2: Recreate all dependent views AFTER all materialized views have been processed. @@ -432,6 +487,45 @@ func generateViewSQL(view *ir.View, targetSchema string) string { return fmt.Sprintf("%s %s AS\n%s;", createClause, viewName, view.Definition) } +// diffViewTriggers computes added, dropped, and modified triggers between two views +func diffViewTriggers(oldView, newView *ir.View) ([]*ir.Trigger, []*ir.Trigger, []*triggerDiff) { + oldTriggers := oldView.Triggers + newTriggers := newView.Triggers + if oldTriggers == nil { + oldTriggers = make(map[string]*ir.Trigger) + } + if newTriggers == nil { + newTriggers = make(map[string]*ir.Trigger) + } + + var added []*ir.Trigger + var dropped []*ir.Trigger + var modified []*triggerDiff + + for name, trigger := range newTriggers { + if _, exists := oldTriggers[name]; !exists { + added = append(added, trigger) + } + } + for name, trigger := range oldTriggers { + if _, exists := newTriggers[name]; !exists { + dropped = append(dropped, trigger) + } + } + for name, newTrigger := range newTriggers { + if oldTrigger, exists := oldTriggers[name]; exists { + if !triggersEqual(oldTrigger, newTrigger) { + modified = append(modified, &triggerDiff{ + Old: oldTrigger, + New: newTrigger, + }) + } + } + } + + return added, dropped, modified +} + // viewsEqual compares two views for equality // Both IRs come from pg_get_viewdef() at the same PostgreSQL version, so string comparison is sufficient func viewsEqual(old, new *ir.View) bool { diff --git a/internal/dump/formatter.go b/internal/dump/formatter.go index ba787fc3..9dd262db 100644 --- a/internal/dump/formatter.go +++ b/internal/dump/formatter.go @@ -239,6 +239,9 @@ func (f *DumpFormatter) getObjectDirectory(objectType string) string { case "table.index", "table.trigger", "table.constraint", "table.policy", "table.rls", "table.comment", "table.column.comment", "table.index.comment": // These are included with their tables return "tables" + case "view.trigger": + // View triggers are included with their views + return "views" case "view.comment": // View comments are included with their views return "views" @@ -269,6 +272,15 @@ func (f *DumpFormatter) getGroupingName(step diff.Diff) string { if parts := strings.Split(step.Path, "."); len(parts) >= 2 { return parts[1] // Return table name } + case diff.DiffTypeViewTrigger: + // For view triggers, group with view + if tableName := f.extractTableNameFromContext(step); tableName != "" { + return tableName + } + // Fallback: extract view name from path + if parts := strings.Split(step.Path, "."); len(parts) >= 2 { + return parts[1] // Return view name + } case diff.DiffTypeViewComment: // For view comments, group with view if step.Source != nil { diff --git a/ir/inspector.go b/ir/inspector.go index 97650649..d8e412dd 100644 --- a/ir/inspector.go +++ b/ir/inspector.go @@ -94,11 +94,18 @@ func (i *Inspector) BuildIR(ctx context.Context, targetSchema string) (*IR, erro }, } - // Concurrent Group 3: Table-Dependent Objects + // Concurrent Group 3: View and table-dependent objects (views must load first) group3 := queryGroup{ - name: "table-dependent objects", + name: "views", funcs: []func(context.Context, *IR, string) error{ i.buildViews, + }, + } + + // Group 4: Objects that depend on both tables AND views (must run after views are loaded) + group4 := queryGroup{ + name: "table-and-view-dependent objects", + funcs: []func(context.Context, *IR, string) error{ i.buildTriggers, i.buildRLSPolicies, }, @@ -125,11 +132,16 @@ func (i *Inspector) BuildIR(ctx context.Context, targetSchema string) (*IR, erro return nil, err } - // Group 3 runs after table details are loaded + // Group 3 runs after table details are loaded (views must be loaded before triggers) if err := i.executeConcurrentGroup(ctx, schema, targetSchema, group3); err != nil { return nil, err } + // Group 4 runs after views are loaded (triggers can reference views for INSTEAD OF triggers) + if err := i.executeConcurrentGroup(ctx, schema, targetSchema, group4); err != nil { + return nil, err + } + // Build indexes after views are loaded (indexes can reference materialized views) if err := i.buildIndexes(ctx, schema, targetSchema); err != nil { return nil, fmt.Errorf("failed to build indexes: %w", err) @@ -1440,29 +1452,37 @@ func (i *Inspector) buildTriggers(ctx context.Context, schema *IR, targetSchema schemaName := triggerRow.TriggerSchema triggerName := triggerRow.TriggerName - // Get the table + // Find where to store this trigger: table, view, or ignored external table targetDBSchema := schema.getOrCreateSchema(schemaName) - table, exists := targetDBSchema.Tables[tableName] - if !exists { + var triggerMap map[string]*Trigger + + if table, exists := targetDBSchema.Tables[tableName]; exists { + triggerMap = table.Triggers + } else if view, exists := targetDBSchema.Views[tableName]; exists { + // Trigger on a view (e.g., INSTEAD OF triggers) + if view.Triggers == nil { + view.Triggers = make(map[string]*Trigger) + } + triggerMap = view.Triggers + } else if i.ignoreConfig != nil && i.ignoreConfig.ShouldIgnoreTable(tableName) { // Check if the table is ignored - if so, create external table stub to hold trigger // This allows users to manage triggers on externally-managed tables - if i.ignoreConfig != nil && i.ignoreConfig.ShouldIgnoreTable(tableName) { - table = &Table{ - Schema: schemaName, - Name: tableName, - Type: TableTypeBase, - IsExternal: true, - Columns: []*Column{}, - Constraints: make(map[string]*Constraint), - Indexes: make(map[string]*Index), - Triggers: make(map[string]*Trigger), - Policies: make(map[string]*RLSPolicy), - } - targetDBSchema.Tables[tableName] = table - } else { - // Table doesn't exist and isn't ignored - skip this trigger - continue + table := &Table{ + Schema: schemaName, + Name: tableName, + Type: TableTypeBase, + IsExternal: true, + Columns: []*Column{}, + Constraints: make(map[string]*Constraint), + Indexes: make(map[string]*Index), + Triggers: make(map[string]*Trigger), + Policies: make(map[string]*RLSPolicy), } + targetDBSchema.Tables[tableName] = table + triggerMap = table.Triggers + } else { + // Relation doesn't exist and isn't ignored - skip this trigger + continue } // Decode trigger type bitmask to extract timing, events, and level @@ -1530,8 +1550,8 @@ func (i *Inspector) buildTriggers(ctx context.Context, schema *IR, targetSchema Comment: comment, } - // Add trigger to table - table.Triggers[triggerName] = trigger + // Add trigger to the appropriate map + triggerMap[triggerName] = trigger } return nil diff --git a/ir/ir.go b/ir/ir.go index c6ec0842..f3c18f65 100644 --- a/ir/ir.go +++ b/ir/ir.go @@ -119,12 +119,13 @@ type TableDependency struct { // View represents a database view type View struct { - Schema string `json:"schema"` - Name string `json:"name"` - Definition string `json:"definition"` - Comment string `json:"comment,omitempty"` - Materialized bool `json:"materialized,omitempty"` - Indexes map[string]*Index `json:"indexes,omitempty"` // For materialized views only + Schema string `json:"schema"` + Name string `json:"name"` + Definition string `json:"definition"` + Comment string `json:"comment,omitempty"` + Materialized bool `json:"materialized,omitempty"` + Indexes map[string]*Index `json:"indexes,omitempty"` // For materialized views only + Triggers map[string]*Trigger `json:"triggers,omitempty"` // For INSTEAD OF triggers on views } // Function represents a database function @@ -294,7 +295,7 @@ type TriggerTiming string const ( TriggerTimingBefore TriggerTiming = "BEFORE" TriggerTimingAfter TriggerTiming = "AFTER" - TriggerTimingInsteadOf TriggerTiming = "INSTEAD_OF" + TriggerTimingInsteadOf TriggerTiming = "INSTEAD OF" ) // TriggerEvent represents the event that triggers the trigger diff --git a/testdata/diff/create_trigger/issue_287_instead_of_view_trigger/diff.sql b/testdata/diff/create_trigger/issue_287_instead_of_view_trigger/diff.sql new file mode 100644 index 00000000..b6965db4 --- /dev/null +++ b/testdata/diff/create_trigger/issue_287_instead_of_view_trigger/diff.sql @@ -0,0 +1,4 @@ +CREATE OR REPLACE TRIGGER trg_user_emails_insert + INSTEAD OF INSERT ON user_emails + FOR EACH ROW + EXECUTE FUNCTION insert_user_emails(); diff --git a/testdata/diff/create_trigger/issue_287_instead_of_view_trigger/new.sql b/testdata/diff/create_trigger/issue_287_instead_of_view_trigger/new.sql new file mode 100644 index 00000000..4be3fc39 --- /dev/null +++ b/testdata/diff/create_trigger/issue_287_instead_of_view_trigger/new.sql @@ -0,0 +1,23 @@ +CREATE TABLE public.users ( + id serial PRIMARY KEY, + email text NOT NULL +); + +CREATE VIEW public.user_emails AS +SELECT id, email +FROM public.users; + +CREATE OR REPLACE FUNCTION public.insert_user_emails() +RETURNS trigger AS $$ +BEGIN + INSERT INTO public.users (email) + VALUES (NEW.email) + RETURNING id, email INTO NEW.id, NEW.email; + RETURN NEW; +END; +$$ LANGUAGE plpgsql; + +CREATE TRIGGER trg_user_emails_insert + INSTEAD OF INSERT ON public.user_emails + FOR EACH ROW + EXECUTE FUNCTION public.insert_user_emails(); diff --git a/testdata/diff/create_trigger/issue_287_instead_of_view_trigger/old.sql b/testdata/diff/create_trigger/issue_287_instead_of_view_trigger/old.sql new file mode 100644 index 00000000..4716a207 --- /dev/null +++ b/testdata/diff/create_trigger/issue_287_instead_of_view_trigger/old.sql @@ -0,0 +1,18 @@ +CREATE TABLE public.users ( + id serial PRIMARY KEY, + email text NOT NULL +); + +CREATE VIEW public.user_emails AS +SELECT id, email +FROM public.users; + +CREATE OR REPLACE FUNCTION public.insert_user_emails() +RETURNS trigger AS $$ +BEGIN + INSERT INTO public.users (email) + VALUES (NEW.email) + RETURNING id, email INTO NEW.id, NEW.email; + RETURN NEW; +END; +$$ LANGUAGE plpgsql; diff --git a/testdata/diff/create_trigger/issue_287_instead_of_view_trigger/plan.json b/testdata/diff/create_trigger/issue_287_instead_of_view_trigger/plan.json new file mode 100644 index 00000000..99dca178 --- /dev/null +++ b/testdata/diff/create_trigger/issue_287_instead_of_view_trigger/plan.json @@ -0,0 +1,26 @@ +{ + "version": "1.0.0", + "pgschema_version": "1.7.0", + "created_at": "1970-01-01T00:00:00Z", + "source_fingerprint": { + "hash": "66e7d57ad55a56551d40de026dab84610355b0d511ccea489d7d0e4ac6bc0db8" + }, + "groups": [ + { + "steps": [ + { + "sql": "CREATE OR REPLACE TRIGGER trg_user_emails_insert\n INSTEAD OF INSERT ON user_emails\n FOR EACH ROW\n EXECUTE FUNCTION insert_user_emails();", + "type": "view.trigger", + "operation": "create", + "path": "public.user_emails.trg_user_emails_insert" + }, + { + "sql": "CREATE OR REPLACE FUNCTION insert_user_emails()\nRETURNS trigger\nLANGUAGE plpgsql\nVOLATILE\nAS $$\nBEGIN\n INSERT INTO users (email)\n VALUES (NEW.email)\n RETURNING id, email INTO NEW.id, NEW.email;\n RETURN NEW;\nEND;\n$$;", + "type": "function", + "operation": "alter", + "path": "public.insert_user_emails" + } + ] + } + ] +} diff --git a/testdata/diff/create_trigger/issue_287_instead_of_view_trigger/plan.sql b/testdata/diff/create_trigger/issue_287_instead_of_view_trigger/plan.sql new file mode 100644 index 00000000..02927f40 --- /dev/null +++ b/testdata/diff/create_trigger/issue_287_instead_of_view_trigger/plan.sql @@ -0,0 +1,17 @@ +CREATE OR REPLACE TRIGGER trg_user_emails_insert + INSTEAD OF INSERT ON user_emails + FOR EACH ROW + EXECUTE FUNCTION insert_user_emails(); + +CREATE OR REPLACE FUNCTION insert_user_emails() +RETURNS trigger +LANGUAGE plpgsql +VOLATILE +AS $$ +BEGIN + INSERT INTO users (email) + VALUES (NEW.email) + RETURNING id, email INTO NEW.id, NEW.email; + RETURN NEW; +END; +$$; diff --git a/testdata/diff/create_trigger/issue_287_instead_of_view_trigger/plan.txt b/testdata/diff/create_trigger/issue_287_instead_of_view_trigger/plan.txt new file mode 100644 index 00000000..6b53b2f0 --- /dev/null +++ b/testdata/diff/create_trigger/issue_287_instead_of_view_trigger/plan.txt @@ -0,0 +1,33 @@ +Plan: 2 to modify. + +Summary by type: + functions: 1 to modify + views: 1 to modify + +Functions: + ~ insert_user_emails + +Views: + ~ user_emails + + trg_user_emails_insert (trigger) + +DDL to be executed: +-------------------------------------------------- + +CREATE OR REPLACE TRIGGER trg_user_emails_insert + INSTEAD OF INSERT ON user_emails + FOR EACH ROW + EXECUTE FUNCTION insert_user_emails(); + +CREATE OR REPLACE FUNCTION insert_user_emails() +RETURNS trigger +LANGUAGE plpgsql +VOLATILE +AS $$ +BEGIN + INSERT INTO users (email) + VALUES (NEW.email) + RETURNING id, email INTO NEW.id, NEW.email; + RETURN NEW; +END; +$$; From 496e102a2c546be6da53fe7895bd839d33e10096 Mon Sep 17 00:00:00 2001 From: Tianzhou Date: Mon, 16 Feb 2026 05:13:50 -0800 Subject: [PATCH 2/3] test: merge INSTEAD OF view trigger into existing add_trigger test case Co-Authored-By: Claude Opus 4.6 --- .../diff/create_trigger/add_trigger/diff.sql | 5 +++ .../diff/create_trigger/add_trigger/new.sql | 21 +++++++++++- .../diff/create_trigger/add_trigger/old.sql | 14 ++++++++ .../diff/create_trigger/add_trigger/plan.json | 16 +++++++-- .../diff/create_trigger/add_trigger/plan.sql | 18 ++++++++++ .../diff/create_trigger/add_trigger/plan.txt | 29 +++++++++++++++- .../diff.sql | 4 --- .../issue_287_instead_of_view_trigger/new.sql | 23 ------------- .../issue_287_instead_of_view_trigger/old.sql | 18 ---------- .../plan.json | 26 --------------- .../plan.sql | 17 ---------- .../plan.txt | 33 ------------------- 12 files changed, 99 insertions(+), 125 deletions(-) delete mode 100644 testdata/diff/create_trigger/issue_287_instead_of_view_trigger/diff.sql delete mode 100644 testdata/diff/create_trigger/issue_287_instead_of_view_trigger/new.sql delete mode 100644 testdata/diff/create_trigger/issue_287_instead_of_view_trigger/old.sql delete mode 100644 testdata/diff/create_trigger/issue_287_instead_of_view_trigger/plan.json delete mode 100644 testdata/diff/create_trigger/issue_287_instead_of_view_trigger/plan.sql delete mode 100644 testdata/diff/create_trigger/issue_287_instead_of_view_trigger/plan.txt diff --git a/testdata/diff/create_trigger/add_trigger/diff.sql b/testdata/diff/create_trigger/add_trigger/diff.sql index 1815ae12..e8b5b456 100644 --- a/testdata/diff/create_trigger/add_trigger/diff.sql +++ b/testdata/diff/create_trigger/add_trigger/diff.sql @@ -12,3 +12,8 @@ CREATE OR REPLACE TRIGGER employees_truncate_log_trigger AFTER TRUNCATE ON employees FOR EACH STATEMENT EXECUTE FUNCTION update_last_modified(); + +CREATE OR REPLACE TRIGGER trg_employee_emails_insert + INSTEAD OF INSERT ON employee_emails + FOR EACH ROW + EXECUTE FUNCTION insert_employee_emails(); diff --git a/testdata/diff/create_trigger/add_trigger/new.sql b/testdata/diff/create_trigger/add_trigger/new.sql index 5c3d10ee..ff849047 100644 --- a/testdata/diff/create_trigger/add_trigger/new.sql +++ b/testdata/diff/create_trigger/add_trigger/new.sql @@ -26,4 +26,23 @@ CREATE TRIGGER employees_insert_timestamp_trigger CREATE TRIGGER employees_truncate_log_trigger AFTER TRUNCATE ON public.employees FOR EACH STATEMENT - EXECUTE FUNCTION public.update_last_modified(); \ No newline at end of file + EXECUTE FUNCTION public.update_last_modified(); + +CREATE VIEW public.employee_emails AS +SELECT id, name +FROM public.employees; + +CREATE OR REPLACE FUNCTION public.insert_employee_emails() +RETURNS trigger AS $$ +BEGIN + INSERT INTO public.employees (name) + VALUES (NEW.name) + RETURNING id, name INTO NEW.id, NEW.name; + RETURN NEW; +END; +$$ LANGUAGE plpgsql; + +CREATE TRIGGER trg_employee_emails_insert + INSTEAD OF INSERT ON public.employee_emails + FOR EACH ROW + EXECUTE FUNCTION public.insert_employee_emails(); \ No newline at end of file diff --git a/testdata/diff/create_trigger/add_trigger/old.sql b/testdata/diff/create_trigger/add_trigger/old.sql index 4237103a..aad80c81 100644 --- a/testdata/diff/create_trigger/add_trigger/old.sql +++ b/testdata/diff/create_trigger/add_trigger/old.sql @@ -11,4 +11,18 @@ BEGIN NEW.last_modified = CURRENT_TIMESTAMP; RETURN NEW; END; +$$ LANGUAGE plpgsql; + +CREATE VIEW public.employee_emails AS +SELECT id, name +FROM public.employees; + +CREATE OR REPLACE FUNCTION public.insert_employee_emails() +RETURNS trigger AS $$ +BEGIN + INSERT INTO public.employees (name) + VALUES (NEW.name) + RETURNING id, name INTO NEW.id, NEW.name; + RETURN NEW; +END; $$ LANGUAGE plpgsql; \ No newline at end of file diff --git a/testdata/diff/create_trigger/add_trigger/plan.json b/testdata/diff/create_trigger/add_trigger/plan.json index 5032dac2..b9403166 100644 --- a/testdata/diff/create_trigger/add_trigger/plan.json +++ b/testdata/diff/create_trigger/add_trigger/plan.json @@ -1,9 +1,9 @@ { "version": "1.0.0", - "pgschema_version": "1.6.2", + "pgschema_version": "1.7.0", "created_at": "1970-01-01T00:00:00Z", "source_fingerprint": { - "hash": "3639b3a4017a2b7805f4e45a9109357539227d8a0a3bb39814349bb1805daeab" + "hash": "3f73c416e0d69d8eaf60edf93de71971e02d53d0750d912f069eba5c0b394329" }, "groups": [ { @@ -25,6 +25,18 @@ "type": "table.trigger", "operation": "create", "path": "public.employees.employees_truncate_log_trigger" + }, + { + "sql": "CREATE OR REPLACE TRIGGER trg_employee_emails_insert\n INSTEAD OF INSERT ON employee_emails\n FOR EACH ROW\n EXECUTE FUNCTION insert_employee_emails();", + "type": "view.trigger", + "operation": "create", + "path": "public.employee_emails.trg_employee_emails_insert" + }, + { + "sql": "CREATE OR REPLACE FUNCTION insert_employee_emails()\nRETURNS trigger\nLANGUAGE plpgsql\nVOLATILE\nAS $$\nBEGIN\n INSERT INTO employees (name)\n VALUES (NEW.name)\n RETURNING id, name INTO NEW.id, NEW.name;\n RETURN NEW;\nEND;\n$$;", + "type": "function", + "operation": "alter", + "path": "public.insert_employee_emails" } ] } diff --git a/testdata/diff/create_trigger/add_trigger/plan.sql b/testdata/diff/create_trigger/add_trigger/plan.sql index 1815ae12..75302ca5 100644 --- a/testdata/diff/create_trigger/add_trigger/plan.sql +++ b/testdata/diff/create_trigger/add_trigger/plan.sql @@ -12,3 +12,21 @@ CREATE OR REPLACE TRIGGER employees_truncate_log_trigger AFTER TRUNCATE ON employees FOR EACH STATEMENT EXECUTE FUNCTION update_last_modified(); + +CREATE OR REPLACE TRIGGER trg_employee_emails_insert + INSTEAD OF INSERT ON employee_emails + FOR EACH ROW + EXECUTE FUNCTION insert_employee_emails(); + +CREATE OR REPLACE FUNCTION insert_employee_emails() +RETURNS trigger +LANGUAGE plpgsql +VOLATILE +AS $$ +BEGIN + INSERT INTO employees (name) + VALUES (NEW.name) + RETURNING id, name INTO NEW.id, NEW.name; + RETURN NEW; +END; +$$; diff --git a/testdata/diff/create_trigger/add_trigger/plan.txt b/testdata/diff/create_trigger/add_trigger/plan.txt index ee6b258c..c34d68da 100644 --- a/testdata/diff/create_trigger/add_trigger/plan.txt +++ b/testdata/diff/create_trigger/add_trigger/plan.txt @@ -1,7 +1,12 @@ -Plan: 1 to modify. +Plan: 3 to modify. Summary by type: + functions: 1 to modify tables: 1 to modify + views: 1 to modify + +Functions: + ~ insert_employee_emails Tables: ~ employees @@ -9,6 +14,10 @@ Tables: + employees_last_modified_trigger (trigger) + employees_truncate_log_trigger (trigger) +Views: + ~ employee_emails + + trg_employee_emails_insert (trigger) + DDL to be executed: -------------------------------------------------- @@ -26,3 +35,21 @@ CREATE OR REPLACE TRIGGER employees_truncate_log_trigger AFTER TRUNCATE ON employees FOR EACH STATEMENT EXECUTE FUNCTION update_last_modified(); + +CREATE OR REPLACE TRIGGER trg_employee_emails_insert + INSTEAD OF INSERT ON employee_emails + FOR EACH ROW + EXECUTE FUNCTION insert_employee_emails(); + +CREATE OR REPLACE FUNCTION insert_employee_emails() +RETURNS trigger +LANGUAGE plpgsql +VOLATILE +AS $$ +BEGIN + INSERT INTO employees (name) + VALUES (NEW.name) + RETURNING id, name INTO NEW.id, NEW.name; + RETURN NEW; +END; +$$; diff --git a/testdata/diff/create_trigger/issue_287_instead_of_view_trigger/diff.sql b/testdata/diff/create_trigger/issue_287_instead_of_view_trigger/diff.sql deleted file mode 100644 index b6965db4..00000000 --- a/testdata/diff/create_trigger/issue_287_instead_of_view_trigger/diff.sql +++ /dev/null @@ -1,4 +0,0 @@ -CREATE OR REPLACE TRIGGER trg_user_emails_insert - INSTEAD OF INSERT ON user_emails - FOR EACH ROW - EXECUTE FUNCTION insert_user_emails(); diff --git a/testdata/diff/create_trigger/issue_287_instead_of_view_trigger/new.sql b/testdata/diff/create_trigger/issue_287_instead_of_view_trigger/new.sql deleted file mode 100644 index 4be3fc39..00000000 --- a/testdata/diff/create_trigger/issue_287_instead_of_view_trigger/new.sql +++ /dev/null @@ -1,23 +0,0 @@ -CREATE TABLE public.users ( - id serial PRIMARY KEY, - email text NOT NULL -); - -CREATE VIEW public.user_emails AS -SELECT id, email -FROM public.users; - -CREATE OR REPLACE FUNCTION public.insert_user_emails() -RETURNS trigger AS $$ -BEGIN - INSERT INTO public.users (email) - VALUES (NEW.email) - RETURNING id, email INTO NEW.id, NEW.email; - RETURN NEW; -END; -$$ LANGUAGE plpgsql; - -CREATE TRIGGER trg_user_emails_insert - INSTEAD OF INSERT ON public.user_emails - FOR EACH ROW - EXECUTE FUNCTION public.insert_user_emails(); diff --git a/testdata/diff/create_trigger/issue_287_instead_of_view_trigger/old.sql b/testdata/diff/create_trigger/issue_287_instead_of_view_trigger/old.sql deleted file mode 100644 index 4716a207..00000000 --- a/testdata/diff/create_trigger/issue_287_instead_of_view_trigger/old.sql +++ /dev/null @@ -1,18 +0,0 @@ -CREATE TABLE public.users ( - id serial PRIMARY KEY, - email text NOT NULL -); - -CREATE VIEW public.user_emails AS -SELECT id, email -FROM public.users; - -CREATE OR REPLACE FUNCTION public.insert_user_emails() -RETURNS trigger AS $$ -BEGIN - INSERT INTO public.users (email) - VALUES (NEW.email) - RETURNING id, email INTO NEW.id, NEW.email; - RETURN NEW; -END; -$$ LANGUAGE plpgsql; diff --git a/testdata/diff/create_trigger/issue_287_instead_of_view_trigger/plan.json b/testdata/diff/create_trigger/issue_287_instead_of_view_trigger/plan.json deleted file mode 100644 index 99dca178..00000000 --- a/testdata/diff/create_trigger/issue_287_instead_of_view_trigger/plan.json +++ /dev/null @@ -1,26 +0,0 @@ -{ - "version": "1.0.0", - "pgschema_version": "1.7.0", - "created_at": "1970-01-01T00:00:00Z", - "source_fingerprint": { - "hash": "66e7d57ad55a56551d40de026dab84610355b0d511ccea489d7d0e4ac6bc0db8" - }, - "groups": [ - { - "steps": [ - { - "sql": "CREATE OR REPLACE TRIGGER trg_user_emails_insert\n INSTEAD OF INSERT ON user_emails\n FOR EACH ROW\n EXECUTE FUNCTION insert_user_emails();", - "type": "view.trigger", - "operation": "create", - "path": "public.user_emails.trg_user_emails_insert" - }, - { - "sql": "CREATE OR REPLACE FUNCTION insert_user_emails()\nRETURNS trigger\nLANGUAGE plpgsql\nVOLATILE\nAS $$\nBEGIN\n INSERT INTO users (email)\n VALUES (NEW.email)\n RETURNING id, email INTO NEW.id, NEW.email;\n RETURN NEW;\nEND;\n$$;", - "type": "function", - "operation": "alter", - "path": "public.insert_user_emails" - } - ] - } - ] -} diff --git a/testdata/diff/create_trigger/issue_287_instead_of_view_trigger/plan.sql b/testdata/diff/create_trigger/issue_287_instead_of_view_trigger/plan.sql deleted file mode 100644 index 02927f40..00000000 --- a/testdata/diff/create_trigger/issue_287_instead_of_view_trigger/plan.sql +++ /dev/null @@ -1,17 +0,0 @@ -CREATE OR REPLACE TRIGGER trg_user_emails_insert - INSTEAD OF INSERT ON user_emails - FOR EACH ROW - EXECUTE FUNCTION insert_user_emails(); - -CREATE OR REPLACE FUNCTION insert_user_emails() -RETURNS trigger -LANGUAGE plpgsql -VOLATILE -AS $$ -BEGIN - INSERT INTO users (email) - VALUES (NEW.email) - RETURNING id, email INTO NEW.id, NEW.email; - RETURN NEW; -END; -$$; diff --git a/testdata/diff/create_trigger/issue_287_instead_of_view_trigger/plan.txt b/testdata/diff/create_trigger/issue_287_instead_of_view_trigger/plan.txt deleted file mode 100644 index 6b53b2f0..00000000 --- a/testdata/diff/create_trigger/issue_287_instead_of_view_trigger/plan.txt +++ /dev/null @@ -1,33 +0,0 @@ -Plan: 2 to modify. - -Summary by type: - functions: 1 to modify - views: 1 to modify - -Functions: - ~ insert_user_emails - -Views: - ~ user_emails - + trg_user_emails_insert (trigger) - -DDL to be executed: --------------------------------------------------- - -CREATE OR REPLACE TRIGGER trg_user_emails_insert - INSTEAD OF INSERT ON user_emails - FOR EACH ROW - EXECUTE FUNCTION insert_user_emails(); - -CREATE OR REPLACE FUNCTION insert_user_emails() -RETURNS trigger -LANGUAGE plpgsql -VOLATILE -AS $$ -BEGIN - INSERT INTO users (email) - VALUES (NEW.email) - RETURNING id, email INTO NEW.id, NEW.email; - RETURN NEW; -END; -$$; From 9a4c6db50967147ac325bbd8b5b0b5b9d453b341 Mon Sep 17 00:00:00 2001 From: Tianzhou Date: Mon, 16 Feb 2026 05:46:46 -0800 Subject: [PATCH 3/3] fix: address review feedback for view trigger support (#287) - Normalize view triggers in normalizeView() for consistent dump output - Sort diffViewTriggers output deterministically to avoid flaky diffs - Move view trigger drops to early drop phase before function drops Co-Authored-By: Claude Opus 4.6 --- internal/diff/diff.go | 3 ++- internal/diff/trigger.go | 54 ++++++++++++++++++++++++---------------- internal/diff/view.go | 16 +++++++++--- ir/normalize.go | 9 +++++-- 4 files changed, 55 insertions(+), 27 deletions(-) diff --git a/internal/diff/diff.go b/internal/diff/diff.go index 9a8318a5..3e25077e 100644 --- a/internal/diff/diff.go +++ b/internal/diff/diff.go @@ -1643,8 +1643,9 @@ func (d *ddlDiff) generateDropSQL(targetSchema string, collector *diffCollector, generateDropPrivilegesSQL(d.droppedPrivileges, targetSchema, collector) generateDropDefaultPrivilegesSQL(d.droppedDefaultPrivileges, targetSchema, collector) - // Drop triggers from modified tables first (triggers depend on functions) + // Drop triggers from modified tables and views first (triggers depend on functions) generateDropTriggersFromModifiedTables(d.modifiedTables, targetSchema, collector) + generateDropTriggersFromModifiedViews(d.modifiedViews, targetSchema, collector) // Drop functions generateDropFunctionsSQL(d.droppedFunctions, targetSchema, collector) diff --git a/internal/diff/trigger.go b/internal/diff/trigger.go index c2eb9b0c..b9a7de11 100644 --- a/internal/diff/trigger.go +++ b/internal/diff/trigger.go @@ -174,6 +174,39 @@ func generateDropTriggersFromModifiedTables(tables []*tableDiff, targetSchema st } } +// generateDropTriggersFromModifiedViews collects and drops all triggers from modified views +// This ensures view triggers are dropped before their associated functions +func generateDropTriggersFromModifiedViews(views []*viewDiff, targetSchema string, collector *diffCollector) { + var allTriggers []*ir.Trigger + + // Collect all dropped triggers from modified views + for _, viewDiff := range views { + for _, trigger := range viewDiff.DroppedTriggers { + allTriggers = append(allTriggers, trigger) + } + } + + // Sort all triggers by name for consistent ordering + sort.Slice(allTriggers, func(i, j int) bool { + return allTriggers[i].Name < allTriggers[j].Name + }) + + // Generate DROP TRIGGER statements for all collected triggers + for _, trigger := range allTriggers { + tableName := getTableNameWithSchema(trigger.Schema, trigger.Table, targetSchema) + sql := fmt.Sprintf("DROP TRIGGER IF EXISTS %s ON %s;", trigger.Name, tableName) + + context := &diffContext{ + Type: DiffTypeViewTrigger, + Operation: DiffOperationDrop, + Path: fmt.Sprintf("%s.%s.%s", trigger.Schema, trigger.Table, trigger.Name), + Source: trigger, + CanRunInTransaction: true, + } + collector.collect(context, sql) + } +} + // generateTriggerSQLWithMode generates CREATE [OR REPLACE] TRIGGER or CREATE CONSTRAINT TRIGGER statement func generateTriggerSQLWithMode(trigger *ir.Trigger, targetSchema string) string { // Build event list in standard order: INSERT, UPDATE, DELETE, TRUNCATE @@ -266,25 +299,4 @@ func generateCreateViewTriggersSQL(triggers []*ir.Trigger, targetSchema string, } } -// generateDropViewTriggersSQL generates DROP TRIGGER statements for view triggers -func generateDropViewTriggersSQL(triggers []*ir.Trigger, targetSchema string, collector *diffCollector) { - sortedTriggers := make([]*ir.Trigger, len(triggers)) - copy(sortedTriggers, triggers) - sort.Slice(sortedTriggers, func(i, j int) bool { - return sortedTriggers[i].Name < sortedTriggers[j].Name - }) - - for _, trigger := range sortedTriggers { - viewName := getTableNameWithSchema(trigger.Schema, trigger.Table, targetSchema) - sql := fmt.Sprintf("DROP TRIGGER IF EXISTS %s ON %s;", trigger.Name, viewName) - context := &diffContext{ - Type: DiffTypeViewTrigger, - Operation: DiffOperationDrop, - Path: fmt.Sprintf("%s.%s.%s", trigger.Schema, trigger.Table, trigger.Name), - Source: trigger, - CanRunInTransaction: true, - } - collector.collect(context, sql) - } -} diff --git a/internal/diff/view.go b/internal/diff/view.go index 0c2aaaba..2919d79a 100644 --- a/internal/diff/view.go +++ b/internal/diff/view.go @@ -347,9 +347,8 @@ func generateModifyViewsSQL(diffs []*viewDiff, targetSchema string, collector *d } // Handle trigger changes (e.g., INSTEAD OF triggers) - applies to both branches above - if len(diff.DroppedTriggers) > 0 { - generateDropViewTriggersSQL(diff.DroppedTriggers, targetSchema, collector) - } + // Note: DroppedTriggers are skipped here because they are already processed in the DROP phase + // (see generateDropTriggersFromModifiedViews in trigger.go) if len(diff.AddedTriggers) > 0 { generateCreateViewTriggersSQL(diff.AddedTriggers, targetSchema, collector) } @@ -523,6 +522,17 @@ func diffViewTriggers(oldView, newView *ir.View) ([]*ir.Trigger, []*ir.Trigger, } } + // Sort for deterministic output (Go map iteration is random) + sort.Slice(added, func(i, j int) bool { + return added[i].Name < added[j].Name + }) + sort.Slice(dropped, func(i, j int) bool { + return dropped[i].Name < dropped[j].Name + }) + sort.Slice(modified, func(i, j int) bool { + return modified[i].Old.Name < modified[j].Old.Name + }) + return added, dropped, modified } diff --git a/ir/normalize.go b/ir/normalize.go index 0e1e4af0..07ea054c 100644 --- a/ir/normalize.go +++ b/ir/normalize.go @@ -275,8 +275,13 @@ func normalizeView(view *View) { return } - // No normalization needed - both IR forms come from database inspection - // at the same PostgreSQL version, so pg_get_viewdef() output is identical + // View definition needs no normalization - both IR forms come from database inspection + // at the same PostgreSQL version, so pg_get_viewdef() output is identical. + + // Normalize triggers on the view (e.g., INSTEAD OF triggers) + for _, trigger := range view.Triggers { + normalizeTrigger(trigger) + } } // normalizeFunction normalizes function signature and definition