diff --git a/internal/diff/diff.go b/internal/diff/diff.go index 48f8b5e9..f8146d0f 100644 --- a/internal/diff/diff.go +++ b/internal/diff/diff.go @@ -354,6 +354,7 @@ type viewDiff struct { CommentChanged bool OldComment string NewComment string + OptionsChanged bool // View options (reloptions) changed AddedIndexes []*ir.Index // For materialized views DroppedIndexes []*ir.Index // For materialized views ModifiedIndexes []*IndexDiff // For materialized views @@ -780,7 +781,12 @@ func GenerateMigration(oldIR, newIR *ir.IR, targetSchema string) []Diff { newView := newViews[key] if oldView, exists := oldViews[key]; exists { structurallyDifferent := !viewsEqual(oldView, newView) + // Check if the view definition itself changed (excluding options). + // This is used to decide if materialized views need DROP+CREATE: + // option-only changes should use ALTER VIEW SET/RESET, not recreation. + definitionChanged := oldView.Definition != newView.Definition || oldView.Materialized != newView.Materialized commentChanged := oldView.Comment != newView.Comment + optionsChanged := !viewOptionsEqual(oldView.Options, newView.Options) // Check if indexes changed for materialized views indexesChanged := false @@ -826,10 +832,12 @@ func GenerateMigration(oldIR, newIR *ir.IR, targetSchema string) []Diff { triggersChanged := len(addedTriggers) > 0 || len(droppedTriggers) > 0 || len(modifiedTriggers) > 0 if structurallyDifferent || commentChanged || indexesChanged || triggersChanged { - // For materialized views with structural changes, mark for recreation + // For materialized views with definition changes, mark for recreation. // For regular views with column changes incompatible with CREATE OR REPLACE VIEW, - // also mark for recreation (issue #308) - needsRecreate := structurallyDifferent && (newView.Materialized || viewColumnsRequireRecreate(oldView, newView)) + // also mark for recreation (issue #308). + // Use definitionChanged (not structurallyDifferent) so that option-only changes + // on materialized views use ALTER SET/RESET instead of DROP+CREATE. + needsRecreate := definitionChanged && (newView.Materialized || viewColumnsRequireRecreate(oldView, newView)) if needsRecreate { diff.modifiedViews = append(diff.modifiedViews, &viewDiff{ @@ -845,6 +853,7 @@ func GenerateMigration(oldIR, newIR *ir.IR, targetSchema string) []Diff { AddedTriggers: addedTriggers, DroppedTriggers: droppedTriggers, ModifiedTriggers: modifiedTriggers, + OptionsChanged: optionsChanged, } // Check for comment changes diff --git a/internal/diff/view.go b/internal/diff/view.go index c3755daf..1fe927de 100644 --- a/internal/diff/view.go +++ b/internal/diff/view.go @@ -248,7 +248,7 @@ func generateModifyViewsSQL(diffs []*viewDiff, targetSchema string, collector *d continue } - // Check if only the comment changed and definition is identical + // Check if only metadata changed and definition is identical // Both IRs come from pg_get_viewdef() at the same PostgreSQL version, so string comparison is sufficient definitionsEqual := diff.Old.Definition == diff.New.Definition optionsEqual := viewOptionsEqual(diff.Old.Options, diff.New.Options) @@ -262,8 +262,16 @@ func generateModifyViewsSQL(diffs []*viewDiff, targetSchema string, collector *d hasTriggerChanges := len(diff.AddedTriggers) > 0 || len(diff.DroppedTriggers) > 0 || len(diff.ModifiedTriggers) > 0 triggerOnlyChange := hasTriggerChanges && definitionsEqual && optionsEqual && !diff.CommentChanged && !hasIndexChanges - // Handle non-structural changes (comment-only, index-only, or trigger-only) - if commentOnlyChange || indexOnlyChange || triggerOnlyChange { + // Check if only options changed (e.g., security_invoker, security_barrier) + optionsOnlyChange := diff.OptionsChanged && definitionsEqual && diff.Old.Materialized == diff.New.Materialized + + // Handle non-structural changes (comment-only, index-only, trigger-only, or options-only) + if commentOnlyChange || indexOnlyChange || triggerOnlyChange || optionsOnlyChange { + // Generate ALTER VIEW SET/RESET for option changes + if diff.OptionsChanged { + generateViewOptionChangesSQL(diff, targetSchema, collector) + } + // Only generate COMMENT ON VIEW statement if comment actually changed if diff.CommentChanged { viewName := qualifyEntityName(diff.New.Schema, diff.New.Name, targetSchema) @@ -514,6 +522,84 @@ func generateViewSQL(view *ir.View, targetSchema string) string { return fmt.Sprintf("%s %s%s AS\n%s;", createClause, viewName, withClause, view.Definition) } +// generateViewOptionChangesSQL generates ALTER VIEW SET/RESET statements for option changes. +// Options are stored as sorted []string in "key=value" format (from pg_class.reloptions). +func generateViewOptionChangesSQL(diff *viewDiff, targetSchema string, collector *diffCollector) { + viewName := qualifyEntityName(diff.New.Schema, diff.New.Name, targetSchema) + + diffType := DiffTypeView + viewKeyword := "VIEW" + if diff.New.Materialized { + diffType = DiffTypeMaterializedView + viewKeyword = "MATERIALIZED VIEW" + } + + // Build maps from "key=value" slices for comparison + oldMap := optionsToMap(diff.Old.Options) + newMap := optionsToMap(diff.New.Options) + + // Collect options to SET (added or changed) + var setOptions []string + for _, opt := range diff.New.Options { + key := optionKey(opt) + if oldMap[key] != newMap[key] { + setOptions = append(setOptions, opt) + } + } + + // Collect options to RESET (removed) + var resetOptions []string + for _, opt := range diff.Old.Options { + key := optionKey(opt) + if _, exists := newMap[key]; !exists { + resetOptions = append(resetOptions, key) + } + } + + path := fmt.Sprintf("%s.%s", diff.New.Schema, diff.New.Name) + + if len(setOptions) > 0 { + sql := fmt.Sprintf("ALTER %s %s SET (%s);", viewKeyword, viewName, strings.Join(setOptions, ", ")) + collector.collect(&diffContext{ + Type: diffType, + Operation: DiffOperationAlter, + Path: path, + Source: diff, + CanRunInTransaction: true, + }, sql) + } + + if len(resetOptions) > 0 { + sql := fmt.Sprintf("ALTER %s %s RESET (%s);", viewKeyword, viewName, strings.Join(resetOptions, ", ")) + collector.collect(&diffContext{ + Type: diffType, + Operation: DiffOperationAlter, + Path: path, + Source: diff, + CanRunInTransaction: true, + }, sql) + } +} + +// optionsToMap converts a []string of "key=value" options to a map[string]string. +func optionsToMap(options []string) map[string]string { + m := make(map[string]string, len(options)) + for _, opt := range options { + if idx := strings.Index(opt, "="); idx >= 0 { + m[opt[:idx]] = opt[idx+1:] + } + } + return m +} + +// optionKey extracts the key from a "key=value" option string. +func optionKey(opt string) string { + if idx := strings.Index(opt, "="); idx >= 0 { + return opt[:idx] + } + return opt +} + // diffViewTriggers computes added, dropped, and modified triggers between two views func diffViewTriggers(oldView, newView *ir.View) ([]*ir.Trigger, []*ir.Trigger, []*triggerDiff) { oldTriggers := oldView.Triggers diff --git a/testdata/diff/create_view/issue_350_view_options/diff.sql b/testdata/diff/create_view/issue_350_view_options/diff.sql new file mode 100644 index 00000000..a0a6e032 --- /dev/null +++ b/testdata/diff/create_view/issue_350_view_options/diff.sql @@ -0,0 +1,4 @@ +ALTER VIEW employee_emails SET (security_invoker=true); +ALTER VIEW employee_names SET (security_invoker=true); +ALTER VIEW employee_secure RESET (security_invoker); +ALTER MATERIALIZED VIEW employee_summary SET (fillfactor=80); diff --git a/testdata/diff/create_view/issue_350_view_options/new.sql b/testdata/diff/create_view/issue_350_view_options/new.sql new file mode 100644 index 00000000..f1a2ece0 --- /dev/null +++ b/testdata/diff/create_view/issue_350_view_options/new.sql @@ -0,0 +1,24 @@ +CREATE TABLE public.employees ( + id SERIAL PRIMARY KEY, + name VARCHAR(100), + email VARCHAR(100), + department_id INTEGER +); + +-- Add security_invoker via CREATE VIEW WITH +CREATE VIEW public.employee_names WITH (security_invoker = true) AS +SELECT id, name FROM public.employees; + +-- Add security_invoker via ALTER VIEW SET +CREATE VIEW public.employee_emails AS +SELECT id, email FROM public.employees; + +ALTER VIEW public.employee_emails SET (security_invoker = true); + +-- Remove security_invoker +CREATE VIEW public.employee_secure AS +SELECT id, name FROM public.employees WHERE id > 0; + +-- Add fillfactor to materialized view (should use ALTER, not DROP+CREATE) +CREATE MATERIALIZED VIEW public.employee_summary WITH (fillfactor = 80) AS +SELECT department_id, COUNT(*) AS cnt FROM public.employees GROUP BY department_id; diff --git a/testdata/diff/create_view/issue_350_view_options/old.sql b/testdata/diff/create_view/issue_350_view_options/old.sql new file mode 100644 index 00000000..23000e65 --- /dev/null +++ b/testdata/diff/create_view/issue_350_view_options/old.sql @@ -0,0 +1,22 @@ +CREATE TABLE public.employees ( + id SERIAL PRIMARY KEY, + name VARCHAR(100), + email VARCHAR(100), + department_id INTEGER +); + +-- View without options (will gain security_invoker via WITH) +CREATE VIEW public.employee_names AS +SELECT id, name FROM public.employees; + +-- View without options (will gain security_invoker via ALTER VIEW SET) +CREATE VIEW public.employee_emails AS +SELECT id, email FROM public.employees; + +-- View with security_invoker (will be removed) +CREATE VIEW public.employee_secure WITH (security_invoker = true) AS +SELECT id, name FROM public.employees WHERE id > 0; + +-- Materialized view without options (will gain fillfactor) +CREATE MATERIALIZED VIEW public.employee_summary AS +SELECT department_id, COUNT(*) AS cnt FROM public.employees GROUP BY department_id; diff --git a/testdata/diff/create_view/issue_350_view_options/plan.json b/testdata/diff/create_view/issue_350_view_options/plan.json new file mode 100644 index 00000000..ff837ead --- /dev/null +++ b/testdata/diff/create_view/issue_350_view_options/plan.json @@ -0,0 +1,38 @@ +{ + "version": "1.0.0", + "pgschema_version": "1.7.4", + "created_at": "1970-01-01T00:00:00Z", + "source_fingerprint": { + "hash": "3dd9c8d5398e477d3a0cf9ac459cb7241f0c3cb02931512dc1a27b431dbc31fa" + }, + "groups": [ + { + "steps": [ + { + "sql": "ALTER VIEW employee_emails SET (security_invoker=true);", + "type": "view", + "operation": "alter", + "path": "public.employee_emails" + }, + { + "sql": "ALTER VIEW employee_names SET (security_invoker=true);", + "type": "view", + "operation": "alter", + "path": "public.employee_names" + }, + { + "sql": "ALTER VIEW employee_secure RESET (security_invoker);", + "type": "view", + "operation": "alter", + "path": "public.employee_secure" + }, + { + "sql": "ALTER MATERIALIZED VIEW employee_summary SET (fillfactor=80);", + "type": "materialized_view", + "operation": "alter", + "path": "public.employee_summary" + } + ] + } + ] +} diff --git a/testdata/diff/create_view/issue_350_view_options/plan.sql b/testdata/diff/create_view/issue_350_view_options/plan.sql new file mode 100644 index 00000000..537c4463 --- /dev/null +++ b/testdata/diff/create_view/issue_350_view_options/plan.sql @@ -0,0 +1,7 @@ +ALTER VIEW employee_emails SET (security_invoker=true); + +ALTER VIEW employee_names SET (security_invoker=true); + +ALTER VIEW employee_secure RESET (security_invoker); + +ALTER MATERIALIZED VIEW employee_summary SET (fillfactor=80); diff --git a/testdata/diff/create_view/issue_350_view_options/plan.txt b/testdata/diff/create_view/issue_350_view_options/plan.txt new file mode 100644 index 00000000..08e81d99 --- /dev/null +++ b/testdata/diff/create_view/issue_350_view_options/plan.txt @@ -0,0 +1,24 @@ +Plan: 4 to modify. + +Summary by type: + views: 3 to modify + materialized views: 1 to modify + +Views: + ~ employee_emails + ~ employee_names + ~ employee_secure + +Materialized views: + ~ employee_summary + +DDL to be executed: +-------------------------------------------------- + +ALTER VIEW employee_emails SET (security_invoker=true); + +ALTER VIEW employee_names SET (security_invoker=true); + +ALTER VIEW employee_secure RESET (security_invoker); + +ALTER MATERIALIZED VIEW employee_summary SET (fillfactor=80);