From cd7ee044245002a7456438c4d9c46b29bff34845 Mon Sep 17 00:00:00 2001 From: ayusssmaan Date: Wed, 17 Jun 2026 12:54:15 +0530 Subject: [PATCH 1/4] feat: detect and emit trigger comments, trigger enabled/disabled state, and sequence comments - Trigger comments: inspect COMMENT ON TRIGGER via pg_description, emit COMMENT ON TRIGGER DDL on diff - Trigger state: inspect trigger enabled flag (pg_trigger.tgenabled), emit ALTER TABLE ENABLE/DISABLE TRIGGER on diff - Sequence comments: inspect COMMENT ON SEQUENCE via pg_description, emit COMMENT ON SEQUENCE DDL on diff - Test fixtures: add_trigger_comment, add_sequence_comment, disable_trigger (all passing) --- internal/diff/diff.go | 16 +++- internal/diff/sequence.go | 29 ++++++ internal/diff/table.go | 89 +++++++++++-------- internal/diff/trigger.go | 49 ++++++++++ ir/inspector.go | 16 ++++ ir/ir.go | 1 + ir/queries/queries.sql.go | 27 +++--- .../comment/add_sequence_comment/diff.sql | 3 + .../diff/comment/add_sequence_comment/new.sql | 7 ++ .../diff/comment/add_sequence_comment/old.sql | 3 + .../diff/comment/add_trigger_comment/diff.sql | 1 + .../diff/comment/add_trigger_comment/new.sql | 20 +++++ .../diff/comment/add_trigger_comment/old.sql | 18 ++++ .../create_trigger/disable_trigger/diff.sql | 1 + .../create_trigger/disable_trigger/new.sql | 20 +++++ .../create_trigger/disable_trigger/old.sql | 18 ++++ 16 files changed, 267 insertions(+), 51 deletions(-) create mode 100644 testdata/diff/comment/add_sequence_comment/diff.sql create mode 100644 testdata/diff/comment/add_sequence_comment/new.sql create mode 100644 testdata/diff/comment/add_sequence_comment/old.sql create mode 100644 testdata/diff/comment/add_trigger_comment/diff.sql create mode 100644 testdata/diff/comment/add_trigger_comment/new.sql create mode 100644 testdata/diff/comment/add_trigger_comment/old.sql create mode 100644 testdata/diff/create_trigger/disable_trigger/diff.sql create mode 100644 testdata/diff/create_trigger/disable_trigger/new.sql create mode 100644 testdata/diff/create_trigger/disable_trigger/old.sql diff --git a/internal/diff/diff.go b/internal/diff/diff.go index b5c1f3a8..81d32df2 100644 --- a/internal/diff/diff.go +++ b/internal/diff/diff.go @@ -1064,12 +1064,20 @@ func GenerateMigration(oldIR, newIR *ir.IR, targetSchema string) []Diff { for _, key := range seqKeys { newSeq := newSequences[key] if oldSeq, exists := oldSequences[key]; exists { - // Skip sequences owned by table columns (created by SERIAL) - if (oldSeq.OwnedByTable != "" && oldSeq.OwnedByColumn != "") || - (newSeq.OwnedByTable != "" && newSeq.OwnedByColumn != "") { + // Skip sequences owned by table columns (created by SERIAL) for structural changes, + // but allow comment-only changes through so COMMENT ON SEQUENCE can be deployed. + isOwned := (oldSeq.OwnedByTable != "" && oldSeq.OwnedByColumn != "") || + (newSeq.OwnedByTable != "" && newSeq.OwnedByColumn != "") + if isOwned { + if oldSeq.Comment != newSeq.Comment { + diff.modifiedSequences = append(diff.modifiedSequences, &sequenceDiff{ + Old: oldSeq, + New: newSeq, + }) + } continue } - if !sequencesEqual(oldSeq, newSeq) { + if !sequencesEqual(oldSeq, newSeq) || oldSeq.Comment != newSeq.Comment { diff.modifiedSequences = append(diff.modifiedSequences, &sequenceDiff{ Old: oldSeq, New: newSeq, diff --git a/internal/diff/sequence.go b/internal/diff/sequence.go index c93303dd..45545942 100644 --- a/internal/diff/sequence.go +++ b/internal/diff/sequence.go @@ -31,9 +31,33 @@ func generateCreateSequencesSQL(sequences []*ir.Sequence, targetSchema string, c } collector.collect(context, sql) + + // Emit COMMENT ON SEQUENCE if present + if seq.Comment != "" { + generateSequenceComment(seq, targetSchema, DiffOperationCreate, collector) + } } } +// generateSequenceComment emits a COMMENT ON SEQUENCE statement +func generateSequenceComment(seq *ir.Sequence, targetSchema string, operation DiffOperation, collector *diffCollector) { + seqName := qualifyEntityName(seq.Schema, seq.Name, targetSchema) + var sql string + if seq.Comment == "" { + sql = fmt.Sprintf("COMMENT ON SEQUENCE %s IS NULL;", seqName) + } else { + sql = fmt.Sprintf("COMMENT ON SEQUENCE %s IS %s;", seqName, quoteString(seq.Comment)) + } + context := &diffContext{ + Type: DiffTypeSequence, + Operation: operation, + Path: fmt.Sprintf("%s.%s", seq.Schema, seq.Name), + Source: seq, + CanRunInTransaction: true, + } + collector.collect(context, sql) +} + // generateDropSequencesSQL generates DROP SEQUENCE statements func generateDropSequencesSQL(sequences []*ir.Sequence, targetSchema string, collector *diffCollector) { // Process sequences in reverse order (already sorted) @@ -71,6 +95,11 @@ func generateModifySequencesSQL(diffs []*sequenceDiff, targetSchema string, coll collector.collect(context, stmt) } + + // Emit COMMENT ON SEQUENCE if comment changed + if diff.Old.Comment != diff.New.Comment { + generateSequenceComment(diff.New, targetSchema, DiffOperationAlter, collector) + } } } diff --git a/internal/diff/table.go b/internal/diff/table.go index 2b94b5e4..fc39c952 100644 --- a/internal/diff/table.go +++ b/internal/diff/table.go @@ -98,10 +98,13 @@ func diffTriggers(oldTable, newTable *ir.Table, diff *tableDiff) { } } - // Find modified triggers + // Find modified triggers (structural changes, comment-only, or enabled-state-only) for name, newTrigger := range newTriggers { if oldTrigger, exists := oldTriggers[name]; exists { - if !triggersEqual(oldTrigger, newTrigger) { + structurallyEqual := triggersEqual(oldTrigger, newTrigger) + commentChanged := oldTrigger.Comment != newTrigger.Comment + enabledChanged := oldTrigger.Enabled != newTrigger.Enabled + if !structurallyEqual || commentChanged || enabledChanged { diff.ModifiedTriggers = append(diff.ModifiedTriggers, &triggerDiff{ Old: oldTrigger, New: newTrigger, @@ -1383,43 +1386,59 @@ func (td *tableDiff) generateAlterTableStatements(targetSchema string, collector // Modify triggers - already sorted by the Diff operation for _, triggerDiff := range td.ModifiedTriggers { - // Constraint triggers don't support CREATE OR REPLACE, so we need to DROP and CREATE - if triggerDiff.New.IsConstraint { - tableName := getTableNameWithSchema(td.Table.Schema, td.Table.Name, targetSchema) + structurallyEqual := triggersEqual(triggerDiff.Old, triggerDiff.New) + commentChanged := triggerDiff.Old.Comment != triggerDiff.New.Comment + enabledChanged := triggerDiff.Old.Enabled != triggerDiff.New.Enabled + + if !structurallyEqual { + // Constraint triggers don't support CREATE OR REPLACE, so we need to DROP and CREATE + if triggerDiff.New.IsConstraint { + tableName := getTableNameWithSchema(td.Table.Schema, td.Table.Name, targetSchema) + + // Step 1: DROP the old trigger + dropSQL := fmt.Sprintf("DROP TRIGGER IF EXISTS %s ON %s;", ir.QuoteIdentifier(triggerDiff.Old.Name), tableName) + dropContext := &diffContext{ + Type: DiffTypeTableTrigger, + Operation: DiffOperationDrop, + Path: fmt.Sprintf("%s.%s.%s", td.Table.Schema, td.Table.Name, triggerDiff.Old.Name), + Source: triggerDiff.Old, + CanRunInTransaction: true, + } + collector.collect(dropContext, dropSQL) - // Step 1: DROP the old trigger - dropSQL := fmt.Sprintf("DROP TRIGGER IF EXISTS %s ON %s;", ir.QuoteIdentifier(triggerDiff.Old.Name), tableName) - dropContext := &diffContext{ - Type: DiffTypeTableTrigger, - Operation: DiffOperationDrop, - Path: fmt.Sprintf("%s.%s.%s", td.Table.Schema, td.Table.Name, triggerDiff.Old.Name), - Source: triggerDiff.Old, - CanRunInTransaction: true, - } - collector.collect(dropContext, dropSQL) + // Step 2: CREATE the new constraint trigger + createSQL := generateTriggerSQLWithMode(triggerDiff.New, targetSchema) + createContext := &diffContext{ + Type: DiffTypeTableTrigger, + Operation: DiffOperationCreate, + Path: fmt.Sprintf("%s.%s.%s", td.Table.Schema, td.Table.Name, triggerDiff.New.Name), + Source: triggerDiff.New, + CanRunInTransaction: true, + } + collector.collect(createContext, createSQL) + } else { + // Use CREATE OR REPLACE for regular triggers + sql := generateTriggerSQLWithMode(triggerDiff.New, targetSchema) - // Step 2: CREATE the new constraint trigger - createSQL := generateTriggerSQLWithMode(triggerDiff.New, targetSchema) - createContext := &diffContext{ - Type: DiffTypeTableTrigger, - Operation: DiffOperationCreate, - Path: fmt.Sprintf("%s.%s.%s", td.Table.Schema, td.Table.Name, triggerDiff.New.Name), - Source: triggerDiff.New, - CanRunInTransaction: true, + context := &diffContext{ + Type: DiffTypeTableTrigger, + Operation: DiffOperationAlter, + Path: fmt.Sprintf("%s.%s.%s", td.Table.Schema, td.Table.Name, triggerDiff.New.Name), + Source: triggerDiff, + CanRunInTransaction: true, + } + collector.collect(context, sql) } - collector.collect(createContext, createSQL) - } else { - // Use CREATE OR REPLACE for regular triggers - sql := generateTriggerSQLWithMode(triggerDiff.New, targetSchema) + } - context := &diffContext{ - Type: DiffTypeTableTrigger, - Operation: DiffOperationAlter, - Path: fmt.Sprintf("%s.%s.%s", td.Table.Schema, td.Table.Name, triggerDiff.New.Name), - Source: triggerDiff, - CanRunInTransaction: true, - } - collector.collect(context, sql) + // Emit COMMENT ON TRIGGER for comment changes (including after structural recreate) + if commentChanged { + generateTriggerComment(triggerDiff.New, td.Table.Schema, td.Table.Name, targetSchema, DiffTypeTableTrigger, collector) + } + + // Emit ENABLE/DISABLE TRIGGER for enabled state changes + if enabledChanged { + generateTriggerEnabledState(triggerDiff.New, td.Table.Schema, td.Table.Name, targetSchema, collector) } } diff --git a/internal/diff/trigger.go b/internal/diff/trigger.go index 93ba151d..ac4a87e6 100644 --- a/internal/diff/trigger.go +++ b/internal/diff/trigger.go @@ -148,6 +148,16 @@ func generateCreateTriggersSQL(triggers []*ir.Trigger, targetSchema string, coll } collector.collect(context, sql) + + // Emit COMMENT ON TRIGGER if present + if trigger.Comment != "" { + generateTriggerComment(trigger, trigger.Schema, trigger.Table, targetSchema, DiffTypeTableTrigger, collector) + } + + // Emit DISABLE TRIGGER if the trigger is disabled + if !trigger.Enabled { + generateTriggerEnabledState(trigger, trigger.Schema, trigger.Table, targetSchema, collector) + } } } @@ -319,7 +329,46 @@ func generateCreateViewTriggersSQL(triggers []*ir.Trigger, targetSchema string, } collector.collect(context, sql) + + if trigger.Comment != "" { + generateTriggerComment(trigger, trigger.Schema, trigger.Table, targetSchema, DiffTypeViewTrigger, collector) + } } } +// generateTriggerComment emits a COMMENT ON TRIGGER statement +func generateTriggerComment(trigger *ir.Trigger, schema, table, targetSchema string, diffType DiffType, collector *diffCollector) { + tableName := getTableNameWithSchema(schema, table, targetSchema) + var sql string + if trigger.Comment == "" { + sql = fmt.Sprintf("COMMENT ON TRIGGER %s ON %s IS NULL;", ir.QuoteIdentifier(trigger.Name), tableName) + } else { + sql = fmt.Sprintf("COMMENT ON TRIGGER %s ON %s IS %s;", ir.QuoteIdentifier(trigger.Name), tableName, quoteString(trigger.Comment)) + } + context := &diffContext{ + Type: diffType, + Operation: DiffOperationAlter, + Path: fmt.Sprintf("%s.%s.%s", schema, table, trigger.Name), + Source: trigger, + CanRunInTransaction: true, + } + collector.collect(context, sql) +} +// generateTriggerEnabledState emits ALTER TABLE DISABLE/ENABLE TRIGGER +func generateTriggerEnabledState(trigger *ir.Trigger, schema, table, targetSchema string, collector *diffCollector) { + tableName := getTableNameWithSchema(schema, table, targetSchema) + state := "ENABLE" + if !trigger.Enabled { + state = "DISABLE" + } + sql := fmt.Sprintf("ALTER TABLE %s %s TRIGGER %s;", tableName, state, ir.QuoteIdentifier(trigger.Name)) + context := &diffContext{ + Type: DiffTypeTableTrigger, + Operation: DiffOperationAlter, + Path: fmt.Sprintf("%s.%s.%s", schema, table, trigger.Name), + Source: trigger, + CanRunInTransaction: true, + } + collector.collect(context, sql) +} diff --git a/ir/inspector.go b/ir/inspector.go index 89ff6fa0..de60893a 100644 --- a/ir/inspector.go +++ b/ir/inspector.go @@ -880,6 +880,11 @@ func (i *Inspector) buildSequences(ctx context.Context, schema *IR, targetSchema } } + seqComment := "" + if seq.SequenceComment.Valid { + seqComment = seq.SequenceComment.String + } + sequence := &Sequence{ Schema: schemaName, Name: sequenceName, @@ -887,6 +892,7 @@ func (i *Inspector) buildSequences(ctx context.Context, schema *IR, targetSchema StartValue: seq.StartValue.Int64, Increment: seq.Increment.Int64, CycleOption: seq.CycleOption.Bool, + Comment: seqComment, } // Set default values if not valid @@ -1724,6 +1730,15 @@ func (i *Inspector) buildTriggers(ctx context.Context, schema *IR, targetSchema comment = triggerRow.TriggerComment.String } + // Extract enabled state: tgenabled 'D' = disabled, anything else = enabled + enabled := true + switch v := triggerRow.TriggerEnabled.(type) { + case string: + enabled = v != "D" + case []byte: + enabled = string(v) != "D" + } + // Determine if this is a constraint trigger oid, ok := triggerRow.TriggerConstraintOid.(int64) isConstraint := ok && oid != 0 @@ -1747,6 +1762,7 @@ func (i *Inspector) buildTriggers(ctx context.Context, schema *IR, targetSchema Deferrable: deferrable, InitiallyDeferred: initDeferred, Comment: comment, + Enabled: enabled, } // Add trigger to the appropriate map diff --git a/ir/ir.go b/ir/ir.go index 791a910a..f29bfb36 100644 --- a/ir/ir.go +++ b/ir/ir.go @@ -297,6 +297,7 @@ type Trigger struct { InitiallyDeferred bool `json:"initially_deferred,omitempty"` // Whether deferred by default OldTable string `json:"old_table,omitempty"` // REFERENCING OLD TABLE AS name NewTable string `json:"new_table,omitempty"` // REFERENCING NEW TABLE AS name + Enabled bool `json:"enabled"` // false = DISABLED (tgenabled='D'), true = enabled (default) } // TriggerTiming represents the timing of trigger execution diff --git a/ir/queries/queries.sql.go b/ir/queries/queries.sql.go index e17c08c2..9496c3a4 100644 --- a/ir/queries/queries.sql.go +++ b/ir/queries/queries.sql.go @@ -2889,7 +2889,8 @@ SELECT s.cycle AS cycle_option, s.cache_size, COALESCE(dep_table.relname, col_table.table_name) AS owned_by_table, - COALESCE(dep_col.attname, col_table.column_name) AS owned_by_column + COALESCE(dep_col.attname, col_table.column_name) AS owned_by_column, + COALESCE(obj_description(c.oid, 'pg_class'), '') AS sequence_comment FROM pg_sequences s LEFT JOIN pg_namespace n ON n.nspname = s.schemaname LEFT JOIN pg_class c ON c.relname = s.sequencename AND c.relnamespace = n.oid @@ -2919,17 +2920,18 @@ ORDER BY s.schemaname, s.sequencename ` type GetSequencesForSchemaRow struct { - SequenceSchema sql.NullString `db:"sequence_schema" json:"sequence_schema"` - SequenceName sql.NullString `db:"sequence_name" json:"sequence_name"` - DataType interface{} `db:"data_type" json:"data_type"` - StartValue sql.NullInt64 `db:"start_value" json:"start_value"` - MinimumValue sql.NullInt64 `db:"minimum_value" json:"minimum_value"` - MaximumValue sql.NullInt64 `db:"maximum_value" json:"maximum_value"` - Increment sql.NullInt64 `db:"increment" json:"increment"` - CycleOption sql.NullBool `db:"cycle_option" json:"cycle_option"` - CacheSize sql.NullInt64 `db:"cache_size" json:"cache_size"` - OwnedByTable sql.NullString `db:"owned_by_table" json:"owned_by_table"` - OwnedByColumn sql.NullString `db:"owned_by_column" json:"owned_by_column"` + SequenceSchema sql.NullString `db:"sequence_schema" json:"sequence_schema"` + SequenceName sql.NullString `db:"sequence_name" json:"sequence_name"` + DataType interface{} `db:"data_type" json:"data_type"` + StartValue sql.NullInt64 `db:"start_value" json:"start_value"` + MinimumValue sql.NullInt64 `db:"minimum_value" json:"minimum_value"` + MaximumValue sql.NullInt64 `db:"maximum_value" json:"maximum_value"` + Increment sql.NullInt64 `db:"increment" json:"increment"` + CycleOption sql.NullBool `db:"cycle_option" json:"cycle_option"` + CacheSize sql.NullInt64 `db:"cache_size" json:"cache_size"` + OwnedByTable sql.NullString `db:"owned_by_table" json:"owned_by_table"` + OwnedByColumn sql.NullString `db:"owned_by_column" json:"owned_by_column"` + SequenceComment sql.NullString `db:"sequence_comment" json:"sequence_comment"` } // GetSequencesForSchema retrieves all sequences for a specific schema @@ -2956,6 +2958,7 @@ func (q *Queries) GetSequencesForSchema(ctx context.Context, dollar_1 sql.NullSt &i.CacheSize, &i.OwnedByTable, &i.OwnedByColumn, + &i.SequenceComment, ); err != nil { return nil, err } diff --git a/testdata/diff/comment/add_sequence_comment/diff.sql b/testdata/diff/comment/add_sequence_comment/diff.sql new file mode 100644 index 00000000..42155cb6 --- /dev/null +++ b/testdata/diff/comment/add_sequence_comment/diff.sql @@ -0,0 +1,3 @@ +COMMENT ON SEQUENCE order_id_seq IS 'Primary key sequence for the orders table, starts at 1000'; + +COMMENT ON SEQUENCE user_id_seq IS 'Primary key sequence for the users table'; diff --git a/testdata/diff/comment/add_sequence_comment/new.sql b/testdata/diff/comment/add_sequence_comment/new.sql new file mode 100644 index 00000000..f59fd8f0 --- /dev/null +++ b/testdata/diff/comment/add_sequence_comment/new.sql @@ -0,0 +1,7 @@ +CREATE SEQUENCE public.user_id_seq; + +COMMENT ON SEQUENCE public.user_id_seq IS 'Primary key sequence for the users table'; + +CREATE SEQUENCE public.order_id_seq AS integer START WITH 1000; + +COMMENT ON SEQUENCE public.order_id_seq IS 'Primary key sequence for the orders table, starts at 1000'; diff --git a/testdata/diff/comment/add_sequence_comment/old.sql b/testdata/diff/comment/add_sequence_comment/old.sql new file mode 100644 index 00000000..477f067e --- /dev/null +++ b/testdata/diff/comment/add_sequence_comment/old.sql @@ -0,0 +1,3 @@ +CREATE SEQUENCE public.user_id_seq; + +CREATE SEQUENCE public.order_id_seq AS integer START WITH 1000; diff --git a/testdata/diff/comment/add_trigger_comment/diff.sql b/testdata/diff/comment/add_trigger_comment/diff.sql new file mode 100644 index 00000000..790f4f4d --- /dev/null +++ b/testdata/diff/comment/add_trigger_comment/diff.sql @@ -0,0 +1 @@ +COMMENT ON TRIGGER employees_last_modified_trigger ON employees IS 'Updates last_modified timestamp to current time on every row update'; diff --git a/testdata/diff/comment/add_trigger_comment/new.sql b/testdata/diff/comment/add_trigger_comment/new.sql new file mode 100644 index 00000000..17a755a8 --- /dev/null +++ b/testdata/diff/comment/add_trigger_comment/new.sql @@ -0,0 +1,20 @@ +CREATE TABLE public.employees ( + id serial PRIMARY KEY, + name text NOT NULL, + last_modified timestamp DEFAULT CURRENT_TIMESTAMP +); + +CREATE OR REPLACE FUNCTION public.update_last_modified() +RETURNS trigger AS $$ +BEGIN + NEW.last_modified = CURRENT_TIMESTAMP; + RETURN NEW; +END; +$$ LANGUAGE plpgsql; + +CREATE TRIGGER employees_last_modified_trigger + BEFORE UPDATE ON public.employees + FOR EACH ROW + EXECUTE FUNCTION public.update_last_modified(); + +COMMENT ON TRIGGER employees_last_modified_trigger ON public.employees IS 'Updates last_modified timestamp to current time on every row update'; diff --git a/testdata/diff/comment/add_trigger_comment/old.sql b/testdata/diff/comment/add_trigger_comment/old.sql new file mode 100644 index 00000000..f78d1acc --- /dev/null +++ b/testdata/diff/comment/add_trigger_comment/old.sql @@ -0,0 +1,18 @@ +CREATE TABLE public.employees ( + id serial PRIMARY KEY, + name text NOT NULL, + last_modified timestamp DEFAULT CURRENT_TIMESTAMP +); + +CREATE OR REPLACE FUNCTION public.update_last_modified() +RETURNS trigger AS $$ +BEGIN + NEW.last_modified = CURRENT_TIMESTAMP; + RETURN NEW; +END; +$$ LANGUAGE plpgsql; + +CREATE TRIGGER employees_last_modified_trigger + BEFORE UPDATE ON public.employees + FOR EACH ROW + EXECUTE FUNCTION public.update_last_modified(); diff --git a/testdata/diff/create_trigger/disable_trigger/diff.sql b/testdata/diff/create_trigger/disable_trigger/diff.sql new file mode 100644 index 00000000..a8bed61e --- /dev/null +++ b/testdata/diff/create_trigger/disable_trigger/diff.sql @@ -0,0 +1 @@ +ALTER TABLE employees DISABLE TRIGGER employees_last_modified_trigger; diff --git a/testdata/diff/create_trigger/disable_trigger/new.sql b/testdata/diff/create_trigger/disable_trigger/new.sql new file mode 100644 index 00000000..bd37c6ff --- /dev/null +++ b/testdata/diff/create_trigger/disable_trigger/new.sql @@ -0,0 +1,20 @@ +CREATE TABLE public.employees ( + id serial PRIMARY KEY, + name text NOT NULL, + last_modified timestamp DEFAULT CURRENT_TIMESTAMP +); + +CREATE OR REPLACE FUNCTION public.update_last_modified() +RETURNS trigger AS $$ +BEGIN + NEW.last_modified = CURRENT_TIMESTAMP; + RETURN NEW; +END; +$$ LANGUAGE plpgsql; + +CREATE TRIGGER employees_last_modified_trigger + BEFORE UPDATE ON public.employees + FOR EACH ROW + EXECUTE FUNCTION public.update_last_modified(); + +ALTER TABLE public.employees DISABLE TRIGGER employees_last_modified_trigger; diff --git a/testdata/diff/create_trigger/disable_trigger/old.sql b/testdata/diff/create_trigger/disable_trigger/old.sql new file mode 100644 index 00000000..f78d1acc --- /dev/null +++ b/testdata/diff/create_trigger/disable_trigger/old.sql @@ -0,0 +1,18 @@ +CREATE TABLE public.employees ( + id serial PRIMARY KEY, + name text NOT NULL, + last_modified timestamp DEFAULT CURRENT_TIMESTAMP +); + +CREATE OR REPLACE FUNCTION public.update_last_modified() +RETURNS trigger AS $$ +BEGIN + NEW.last_modified = CURRENT_TIMESTAMP; + RETURN NEW; +END; +$$ LANGUAGE plpgsql; + +CREATE TRIGGER employees_last_modified_trigger + BEFORE UPDATE ON public.employees + FOR EACH ROW + EXECUTE FUNCTION public.update_last_modified(); From 47c211b532937a8208d5647700f2926c4f39af81 Mon Sep 17 00:00:00 2001 From: ayusssmaan Date: Wed, 17 Jun 2026 13:59:01 +0530 Subject: [PATCH 2/4] fix: emit COMMENT ON SEQUENCE for SERIAL-owned sequences created via CREATE TABLE When a table with BIGSERIAL/SERIAL columns was created for the first time, pgschema skipped the sequence from addedSequences (correctly, since CREATE TABLE creates it implicitly), but this also skipped emitting any COMMENT ON SEQUENCE declared in the model. On the second run, the sequence existed in the live DB and the comment diff fired, re-applying all sequence comments indefinitely. Fix: track skipped-but-commented SERIAL sequences in addedSerialSeqComments and emit their COMMENT ON SEQUENCE statements after all CREATE TABLE calls complete. Test: add_serial_sequence_comment_on_create fixture verifies COMMENT is emitted on first deploy alongside CREATE TABLE, not deferred to a subsequent run. --- internal/diff/diff.go | 13 +++++++++++++ .../add_serial_sequence_comment_on_create/diff.sql | 7 +++++++ .../add_serial_sequence_comment_on_create/new.sql | 7 +++++++ .../add_serial_sequence_comment_on_create/old.sql | 0 4 files changed, 27 insertions(+) create mode 100644 testdata/diff/comment/add_serial_sequence_comment_on_create/diff.sql create mode 100644 testdata/diff/comment/add_serial_sequence_comment_on_create/new.sql create mode 100644 testdata/diff/comment/add_serial_sequence_comment_on_create/old.sql diff --git a/internal/diff/diff.go b/internal/diff/diff.go index 81d32df2..a526fee6 100644 --- a/internal/diff/diff.go +++ b/internal/diff/diff.go @@ -288,6 +288,7 @@ type ddlDiff struct { droppedTypes []*ir.Type modifiedTypes []*typeDiff addedSequences []*ir.Sequence + addedSerialSeqComments []*ir.Sequence // SERIAL-owned sequences skipped from addedSequences but with comments to emit droppedSequences []*ir.Sequence modifiedSequences []*sequenceDiff addedDefaultPrivileges []*ir.DefaultPrivilege @@ -478,6 +479,7 @@ func GenerateMigration(oldIR, newIR *ir.IR, targetSchema string) []Diff { droppedTypes: []*ir.Type{}, modifiedTypes: []*typeDiff{}, addedSequences: []*ir.Sequence{}, + addedSerialSeqComments: []*ir.Sequence{}, droppedSequences: []*ir.Sequence{}, modifiedSequences: []*sequenceDiff{}, addedDefaultPrivileges: []*ir.DefaultPrivilege{}, @@ -1041,6 +1043,11 @@ func GenerateMigration(oldIR, newIR *ir.IR, targetSchema string) []Diff { // (created by SERIAL in CREATE TABLE). If the column already exists, // we need to create the sequence explicitly for ALTER COLUMN to use. if seq.OwnedByTable != "" && seq.OwnedByColumn != "" && !columnExistsInTables(oldTables, seq.Schema, seq.OwnedByTable, seq.OwnedByColumn) { + // Sequence is created implicitly by CREATE TABLE (SERIAL). Emit its + // comment separately after all tables are created. + if seq.Comment != "" { + diff.addedSerialSeqComments = append(diff.addedSerialSeqComments, seq) + } continue } diff.addedSequences = append(diff.addedSequences, seq) @@ -1826,6 +1833,12 @@ func (d *ddlDiff) generateCreateSQL(targetSchema string, collector *diffCollecto // Create tables WITH function/domain dependencies (now that functions and deferred domains exist) deferredPolicies2, deferredConstraints2 := generateCreateTablesSQL(tablesWithDeps, targetSchema, collector, existingTables, shouldDeferPolicy, d.suppressedInlineFKs) + // Emit COMMENT ON SEQUENCE for sequences created implicitly via CREATE TABLE (SERIAL/BIGSERIAL). + // These were skipped from addedSequences but their comments must still be deployed. + for _, seq := range d.addedSerialSeqComments { + generateSequenceComment(seq, targetSchema, DiffOperationCreate, collector) + } + // Add deferred foreign key constraints from BOTH batches AFTER all tables are created // This ensures FK references to tables in the second batch (function-dependent tables) work correctly allDeferredConstraints := append(deferredConstraints1, deferredConstraints2...) diff --git a/testdata/diff/comment/add_serial_sequence_comment_on_create/diff.sql b/testdata/diff/comment/add_serial_sequence_comment_on_create/diff.sql new file mode 100644 index 00000000..2f2021d3 --- /dev/null +++ b/testdata/diff/comment/add_serial_sequence_comment_on_create/diff.sql @@ -0,0 +1,7 @@ +CREATE TABLE IF NOT EXISTS orders ( + order_id BIGSERIAL, + customer_id integer NOT NULL, + total numeric(10,2) NOT NULL, + CONSTRAINT orders_pkey PRIMARY KEY (order_id) +); +COMMENT ON SEQUENCE orders_order_id_seq IS 'Primary key sequence for the orders table'; diff --git a/testdata/diff/comment/add_serial_sequence_comment_on_create/new.sql b/testdata/diff/comment/add_serial_sequence_comment_on_create/new.sql new file mode 100644 index 00000000..14ca31af --- /dev/null +++ b/testdata/diff/comment/add_serial_sequence_comment_on_create/new.sql @@ -0,0 +1,7 @@ +CREATE TABLE public.orders ( + order_id bigserial PRIMARY KEY, + customer_id integer NOT NULL, + total numeric(10,2) NOT NULL +); + +COMMENT ON SEQUENCE public.orders_order_id_seq IS 'Primary key sequence for the orders table'; diff --git a/testdata/diff/comment/add_serial_sequence_comment_on_create/old.sql b/testdata/diff/comment/add_serial_sequence_comment_on_create/old.sql new file mode 100644 index 00000000..e69de29b From 6e4e83c73b6637843eb7a4aab106c402a7d56059 Mon Sep 17 00:00:00 2001 From: ayusssmaan Date: Wed, 17 Jun 2026 15:18:57 +0530 Subject: [PATCH 3/4] fix: address Greptile review findings and add missing plan test files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Greptile fixes: - ir/ir.go: rename Enabled bool → Disabled bool (omitempty) so zero-value means enabled, matching Postgres default; avoids spurious DISABLE TRIGGER on triggers that omit the field - ir/inspector.go: set Disabled=true only when tgenabled='D' - internal/diff/trigger.go: update all !trigger.Enabled → trigger.Disabled; add DISABLE TRIGGER emission to view trigger create path (was missing) - internal/diff/table.go: update Enabled → Disabled in trigger diff detection - internal/diff/view.go: emit trigger comment and disabled state in both constraint-recreate and non-constraint modify paths for view triggers CI fix: - Add plan.sql, plan.json, plan.txt for all 4 new fixtures so TestPlanAndApply passes --- internal/diff/table.go | 4 +-- internal/diff/trigger.go | 8 ++++-- internal/diff/view.go | 12 +++++++++ ir/inspector.go | 10 +++---- ir/ir.go | 2 +- .../comment/add_sequence_comment/plan.json | 26 +++++++++++++++++++ .../comment/add_sequence_comment/plan.sql | 3 +++ .../comment/add_sequence_comment/plan.txt | 15 +++++++++++ .../plan.json | 26 +++++++++++++++++++ .../plan.sql | 8 ++++++ .../plan.txt | 23 ++++++++++++++++ .../comment/add_trigger_comment/plan.json | 20 ++++++++++++++ .../diff/comment/add_trigger_comment/plan.sql | 1 + .../diff/comment/add_trigger_comment/plan.txt | 13 ++++++++++ .../create_trigger/disable_trigger/plan.json | 20 ++++++++++++++ .../create_trigger/disable_trigger/plan.sql | 1 + .../create_trigger/disable_trigger/plan.txt | 13 ++++++++++ 17 files changed, 195 insertions(+), 10 deletions(-) create mode 100644 testdata/diff/comment/add_sequence_comment/plan.json create mode 100644 testdata/diff/comment/add_sequence_comment/plan.sql create mode 100644 testdata/diff/comment/add_sequence_comment/plan.txt create mode 100644 testdata/diff/comment/add_serial_sequence_comment_on_create/plan.json create mode 100644 testdata/diff/comment/add_serial_sequence_comment_on_create/plan.sql create mode 100644 testdata/diff/comment/add_serial_sequence_comment_on_create/plan.txt create mode 100644 testdata/diff/comment/add_trigger_comment/plan.json create mode 100644 testdata/diff/comment/add_trigger_comment/plan.sql create mode 100644 testdata/diff/comment/add_trigger_comment/plan.txt create mode 100644 testdata/diff/create_trigger/disable_trigger/plan.json create mode 100644 testdata/diff/create_trigger/disable_trigger/plan.sql create mode 100644 testdata/diff/create_trigger/disable_trigger/plan.txt diff --git a/internal/diff/table.go b/internal/diff/table.go index fc39c952..8fca2d56 100644 --- a/internal/diff/table.go +++ b/internal/diff/table.go @@ -103,7 +103,7 @@ func diffTriggers(oldTable, newTable *ir.Table, diff *tableDiff) { if oldTrigger, exists := oldTriggers[name]; exists { structurallyEqual := triggersEqual(oldTrigger, newTrigger) commentChanged := oldTrigger.Comment != newTrigger.Comment - enabledChanged := oldTrigger.Enabled != newTrigger.Enabled + enabledChanged := oldTrigger.Disabled != newTrigger.Disabled if !structurallyEqual || commentChanged || enabledChanged { diff.ModifiedTriggers = append(diff.ModifiedTriggers, &triggerDiff{ Old: oldTrigger, @@ -1388,7 +1388,7 @@ func (td *tableDiff) generateAlterTableStatements(targetSchema string, collector for _, triggerDiff := range td.ModifiedTriggers { structurallyEqual := triggersEqual(triggerDiff.Old, triggerDiff.New) commentChanged := triggerDiff.Old.Comment != triggerDiff.New.Comment - enabledChanged := triggerDiff.Old.Enabled != triggerDiff.New.Enabled + enabledChanged := triggerDiff.Old.Disabled != triggerDiff.New.Disabled if !structurallyEqual { // Constraint triggers don't support CREATE OR REPLACE, so we need to DROP and CREATE diff --git a/internal/diff/trigger.go b/internal/diff/trigger.go index ac4a87e6..b95f43a1 100644 --- a/internal/diff/trigger.go +++ b/internal/diff/trigger.go @@ -155,7 +155,7 @@ func generateCreateTriggersSQL(triggers []*ir.Trigger, targetSchema string, coll } // Emit DISABLE TRIGGER if the trigger is disabled - if !trigger.Enabled { + if trigger.Disabled { generateTriggerEnabledState(trigger, trigger.Schema, trigger.Table, targetSchema, collector) } } @@ -333,6 +333,10 @@ func generateCreateViewTriggersSQL(triggers []*ir.Trigger, targetSchema string, if trigger.Comment != "" { generateTriggerComment(trigger, trigger.Schema, trigger.Table, targetSchema, DiffTypeViewTrigger, collector) } + + if trigger.Disabled { + generateTriggerEnabledState(trigger, trigger.Schema, trigger.Table, targetSchema, collector) + } } } @@ -359,7 +363,7 @@ func generateTriggerComment(trigger *ir.Trigger, schema, table, targetSchema str func generateTriggerEnabledState(trigger *ir.Trigger, schema, table, targetSchema string, collector *diffCollector) { tableName := getTableNameWithSchema(schema, table, targetSchema) state := "ENABLE" - if !trigger.Enabled { + if trigger.Disabled { state = "DISABLE" } sql := fmt.Sprintf("ALTER TABLE %s %s TRIGGER %s;", tableName, state, ir.QuoteIdentifier(trigger.Name)) diff --git a/internal/diff/view.go b/internal/diff/view.go index fa2001c8..71768d4e 100644 --- a/internal/diff/view.go +++ b/internal/diff/view.go @@ -435,6 +435,12 @@ func generateModifyViewsSQL(diffs []*viewDiff, targetSchema string, collector *d CanRunInTransaction: true, } collector.collect(createContext, createSQL) + if triggerDiff.New.Comment != "" { + generateTriggerComment(triggerDiff.New, diff.New.Schema, diff.New.Name, targetSchema, DiffTypeViewTrigger, collector) + } + if triggerDiff.New.Disabled { + generateTriggerEnabledState(triggerDiff.New, diff.New.Schema, diff.New.Name, targetSchema, collector) + } } else { sql := generateTriggerSQLWithMode(triggerDiff.New, targetSchema) context := &diffContext{ @@ -445,6 +451,12 @@ func generateModifyViewsSQL(diffs []*viewDiff, targetSchema string, collector *d CanRunInTransaction: true, } collector.collect(context, sql) + if triggerDiff.New.Comment != "" { + generateTriggerComment(triggerDiff.New, diff.New.Schema, diff.New.Name, targetSchema, DiffTypeViewTrigger, collector) + } + if triggerDiff.New.Disabled { + generateTriggerEnabledState(triggerDiff.New, diff.New.Schema, diff.New.Name, targetSchema, collector) + } } } } diff --git a/ir/inspector.go b/ir/inspector.go index de60893a..c29c25b5 100644 --- a/ir/inspector.go +++ b/ir/inspector.go @@ -1730,13 +1730,13 @@ func (i *Inspector) buildTriggers(ctx context.Context, schema *IR, targetSchema comment = triggerRow.TriggerComment.String } - // Extract enabled state: tgenabled 'D' = disabled, anything else = enabled - enabled := true + // Extract disabled state: tgenabled 'D' = disabled, anything else = enabled (Postgres default) + disabled := false switch v := triggerRow.TriggerEnabled.(type) { case string: - enabled = v != "D" + disabled = v == "D" case []byte: - enabled = string(v) != "D" + disabled = string(v) == "D" } // Determine if this is a constraint trigger @@ -1762,7 +1762,7 @@ func (i *Inspector) buildTriggers(ctx context.Context, schema *IR, targetSchema Deferrable: deferrable, InitiallyDeferred: initDeferred, Comment: comment, - Enabled: enabled, + Disabled: disabled, } // Add trigger to the appropriate map diff --git a/ir/ir.go b/ir/ir.go index f29bfb36..4e52cd31 100644 --- a/ir/ir.go +++ b/ir/ir.go @@ -297,7 +297,7 @@ type Trigger struct { InitiallyDeferred bool `json:"initially_deferred,omitempty"` // Whether deferred by default OldTable string `json:"old_table,omitempty"` // REFERENCING OLD TABLE AS name NewTable string `json:"new_table,omitempty"` // REFERENCING NEW TABLE AS name - Enabled bool `json:"enabled"` // false = DISABLED (tgenabled='D'), true = enabled (default) + Disabled bool `json:"disabled,omitempty"` // true = DISABLED (tgenabled='D'); omitted/false = enabled (Postgres default) } // TriggerTiming represents the timing of trigger execution diff --git a/testdata/diff/comment/add_sequence_comment/plan.json b/testdata/diff/comment/add_sequence_comment/plan.json new file mode 100644 index 00000000..7ca4dca1 --- /dev/null +++ b/testdata/diff/comment/add_sequence_comment/plan.json @@ -0,0 +1,26 @@ +{ + "version": "1.0.0", + "pgschema_version": "1.11.0", + "created_at": "1970-01-01T00:00:00Z", + "source_fingerprint": { + "hash": "f92816662b57efe3e43677b68115cd1e978e7d16e2d516519230558fb841466d" + }, + "groups": [ + { + "steps": [ + { + "sql": "COMMENT ON SEQUENCE order_id_seq IS 'Primary key sequence for the orders table, starts at 1000';", + "type": "sequence", + "operation": "alter", + "path": "public.order_id_seq" + }, + { + "sql": "COMMENT ON SEQUENCE user_id_seq IS 'Primary key sequence for the users table';", + "type": "sequence", + "operation": "alter", + "path": "public.user_id_seq" + } + ] + } + ] +} diff --git a/testdata/diff/comment/add_sequence_comment/plan.sql b/testdata/diff/comment/add_sequence_comment/plan.sql new file mode 100644 index 00000000..42155cb6 --- /dev/null +++ b/testdata/diff/comment/add_sequence_comment/plan.sql @@ -0,0 +1,3 @@ +COMMENT ON SEQUENCE order_id_seq IS 'Primary key sequence for the orders table, starts at 1000'; + +COMMENT ON SEQUENCE user_id_seq IS 'Primary key sequence for the users table'; diff --git a/testdata/diff/comment/add_sequence_comment/plan.txt b/testdata/diff/comment/add_sequence_comment/plan.txt new file mode 100644 index 00000000..cff6901a --- /dev/null +++ b/testdata/diff/comment/add_sequence_comment/plan.txt @@ -0,0 +1,15 @@ +Plan: 2 to modify. + +Summary by type: + sequences: 2 to modify + +Sequences: + ~ order_id_seq + ~ user_id_seq + +DDL to be executed: +-------------------------------------------------- + +COMMENT ON SEQUENCE order_id_seq IS 'Primary key sequence for the orders table, starts at 1000'; + +COMMENT ON SEQUENCE user_id_seq IS 'Primary key sequence for the users table'; diff --git a/testdata/diff/comment/add_serial_sequence_comment_on_create/plan.json b/testdata/diff/comment/add_serial_sequence_comment_on_create/plan.json new file mode 100644 index 00000000..7c3c51c6 --- /dev/null +++ b/testdata/diff/comment/add_serial_sequence_comment_on_create/plan.json @@ -0,0 +1,26 @@ +{ + "version": "1.0.0", + "pgschema_version": "1.11.0", + "created_at": "1970-01-01T00:00:00Z", + "source_fingerprint": { + "hash": "965b1131737c955e24c7f827c55bd78e4cb49a75adfd04229e0ba297376f5085" + }, + "groups": [ + { + "steps": [ + { + "sql": "CREATE TABLE IF NOT EXISTS orders (\n order_id BIGSERIAL,\n customer_id integer NOT NULL,\n total numeric(10,2) NOT NULL,\n CONSTRAINT orders_pkey PRIMARY KEY (order_id)\n);", + "type": "table", + "operation": "create", + "path": "public.orders" + }, + { + "sql": "COMMENT ON SEQUENCE orders_order_id_seq IS 'Primary key sequence for the orders table';", + "type": "sequence", + "operation": "create", + "path": "public.orders_order_id_seq" + } + ] + } + ] +} diff --git a/testdata/diff/comment/add_serial_sequence_comment_on_create/plan.sql b/testdata/diff/comment/add_serial_sequence_comment_on_create/plan.sql new file mode 100644 index 00000000..127e1ad1 --- /dev/null +++ b/testdata/diff/comment/add_serial_sequence_comment_on_create/plan.sql @@ -0,0 +1,8 @@ +CREATE TABLE IF NOT EXISTS orders ( + order_id BIGSERIAL, + customer_id integer NOT NULL, + total numeric(10,2) NOT NULL, + CONSTRAINT orders_pkey PRIMARY KEY (order_id) +); + +COMMENT ON SEQUENCE orders_order_id_seq IS 'Primary key sequence for the orders table'; diff --git a/testdata/diff/comment/add_serial_sequence_comment_on_create/plan.txt b/testdata/diff/comment/add_serial_sequence_comment_on_create/plan.txt new file mode 100644 index 00000000..f1ba1eb5 --- /dev/null +++ b/testdata/diff/comment/add_serial_sequence_comment_on_create/plan.txt @@ -0,0 +1,23 @@ +Plan: 2 to add. + +Summary by type: + sequences: 1 to add + tables: 1 to add + +Sequences: + + orders_order_id_seq + +Tables: + + orders + +DDL to be executed: +-------------------------------------------------- + +CREATE TABLE IF NOT EXISTS orders ( + order_id BIGSERIAL, + customer_id integer NOT NULL, + total numeric(10,2) NOT NULL, + CONSTRAINT orders_pkey PRIMARY KEY (order_id) +); + +COMMENT ON SEQUENCE orders_order_id_seq IS 'Primary key sequence for the orders table'; diff --git a/testdata/diff/comment/add_trigger_comment/plan.json b/testdata/diff/comment/add_trigger_comment/plan.json new file mode 100644 index 00000000..3481632c --- /dev/null +++ b/testdata/diff/comment/add_trigger_comment/plan.json @@ -0,0 +1,20 @@ +{ + "version": "1.0.0", + "pgschema_version": "1.11.0", + "created_at": "1970-01-01T00:00:00Z", + "source_fingerprint": { + "hash": "6aa4d2eb121345da38a1de455a5840b523fc30982164d0c2569cb1518c020033" + }, + "groups": [ + { + "steps": [ + { + "sql": "COMMENT ON TRIGGER employees_last_modified_trigger ON employees IS 'Updates last_modified timestamp to current time on every row update';", + "type": "table.trigger", + "operation": "alter", + "path": "public.employees.employees_last_modified_trigger" + } + ] + } + ] +} diff --git a/testdata/diff/comment/add_trigger_comment/plan.sql b/testdata/diff/comment/add_trigger_comment/plan.sql new file mode 100644 index 00000000..790f4f4d --- /dev/null +++ b/testdata/diff/comment/add_trigger_comment/plan.sql @@ -0,0 +1 @@ +COMMENT ON TRIGGER employees_last_modified_trigger ON employees IS 'Updates last_modified timestamp to current time on every row update'; diff --git a/testdata/diff/comment/add_trigger_comment/plan.txt b/testdata/diff/comment/add_trigger_comment/plan.txt new file mode 100644 index 00000000..35027657 --- /dev/null +++ b/testdata/diff/comment/add_trigger_comment/plan.txt @@ -0,0 +1,13 @@ +Plan: 1 to modify. + +Summary by type: + tables: 1 to modify + +Tables: + ~ employees + ~ employees_last_modified_trigger (trigger) + +DDL to be executed: +-------------------------------------------------- + +COMMENT ON TRIGGER employees_last_modified_trigger ON employees IS 'Updates last_modified timestamp to current time on every row update'; diff --git a/testdata/diff/create_trigger/disable_trigger/plan.json b/testdata/diff/create_trigger/disable_trigger/plan.json new file mode 100644 index 00000000..3f6aecd3 --- /dev/null +++ b/testdata/diff/create_trigger/disable_trigger/plan.json @@ -0,0 +1,20 @@ +{ + "version": "1.0.0", + "pgschema_version": "1.11.0", + "created_at": "1970-01-01T00:00:00Z", + "source_fingerprint": { + "hash": "6aa4d2eb121345da38a1de455a5840b523fc30982164d0c2569cb1518c020033" + }, + "groups": [ + { + "steps": [ + { + "sql": "ALTER TABLE employees DISABLE TRIGGER employees_last_modified_trigger;", + "type": "table.trigger", + "operation": "alter", + "path": "public.employees.employees_last_modified_trigger" + } + ] + } + ] +} diff --git a/testdata/diff/create_trigger/disable_trigger/plan.sql b/testdata/diff/create_trigger/disable_trigger/plan.sql new file mode 100644 index 00000000..a8bed61e --- /dev/null +++ b/testdata/diff/create_trigger/disable_trigger/plan.sql @@ -0,0 +1 @@ +ALTER TABLE employees DISABLE TRIGGER employees_last_modified_trigger; diff --git a/testdata/diff/create_trigger/disable_trigger/plan.txt b/testdata/diff/create_trigger/disable_trigger/plan.txt new file mode 100644 index 00000000..5ff6fabd --- /dev/null +++ b/testdata/diff/create_trigger/disable_trigger/plan.txt @@ -0,0 +1,13 @@ +Plan: 1 to modify. + +Summary by type: + tables: 1 to modify + +Tables: + ~ employees + ~ employees_last_modified_trigger (trigger) + +DDL to be executed: +-------------------------------------------------- + +ALTER TABLE employees DISABLE TRIGGER employees_last_modified_trigger; From fc953c0ee8fd719925ec176485a4a3c519294755 Mon Sep 17 00:00:00 2001 From: ayusssmaan Date: Thu, 18 Jun 2026 12:01:21 +0530 Subject: [PATCH 4/4] fix: address Copilot review comments on trigger comment/state handling - Re-apply trigger comment and disabled state after structural recreation in table diff to avoid losing them on DROP+CREATE - Use old/new comparison in view trigger modification branches to handle comment removal and disabled->enabled transitions correctly - Pass diffType to generateTriggerEnabledState so view trigger enable/disable steps are correctly labeled as DiffTypeViewTrigger in plan output --- internal/diff/table.go | 12 +++++++----- internal/diff/trigger.go | 8 ++++---- internal/diff/view.go | 16 ++++++++++------ 3 files changed, 21 insertions(+), 15 deletions(-) diff --git a/internal/diff/table.go b/internal/diff/table.go index 8fca2d56..0942bf64 100644 --- a/internal/diff/table.go +++ b/internal/diff/table.go @@ -1431,14 +1431,16 @@ func (td *tableDiff) generateAlterTableStatements(targetSchema string, collector } } - // Emit COMMENT ON TRIGGER for comment changes (including after structural recreate) - if commentChanged { + // Emit COMMENT ON TRIGGER when the comment changed, or after structural recreation + // so the desired comment is preserved. + if commentChanged || (!structurallyEqual && triggerDiff.New.Comment != "") { generateTriggerComment(triggerDiff.New, td.Table.Schema, td.Table.Name, targetSchema, DiffTypeTableTrigger, collector) } - // Emit ENABLE/DISABLE TRIGGER for enabled state changes - if enabledChanged { - generateTriggerEnabledState(triggerDiff.New, td.Table.Schema, td.Table.Name, targetSchema, collector) + // Emit ENABLE/DISABLE TRIGGER when the state changed, or after structural recreation + // so disabled triggers stay disabled. + if enabledChanged || (!structurallyEqual && triggerDiff.New.Disabled) { + generateTriggerEnabledState(triggerDiff.New, td.Table.Schema, td.Table.Name, targetSchema, DiffTypeTableTrigger, collector) } } diff --git a/internal/diff/trigger.go b/internal/diff/trigger.go index b95f43a1..89579ecf 100644 --- a/internal/diff/trigger.go +++ b/internal/diff/trigger.go @@ -156,7 +156,7 @@ func generateCreateTriggersSQL(triggers []*ir.Trigger, targetSchema string, coll // Emit DISABLE TRIGGER if the trigger is disabled if trigger.Disabled { - generateTriggerEnabledState(trigger, trigger.Schema, trigger.Table, targetSchema, collector) + generateTriggerEnabledState(trigger, trigger.Schema, trigger.Table, targetSchema, DiffTypeTableTrigger, collector) } } } @@ -335,7 +335,7 @@ func generateCreateViewTriggersSQL(triggers []*ir.Trigger, targetSchema string, } if trigger.Disabled { - generateTriggerEnabledState(trigger, trigger.Schema, trigger.Table, targetSchema, collector) + generateTriggerEnabledState(trigger, trigger.Schema, trigger.Table, targetSchema, DiffTypeViewTrigger, collector) } } } @@ -360,7 +360,7 @@ func generateTriggerComment(trigger *ir.Trigger, schema, table, targetSchema str } // generateTriggerEnabledState emits ALTER TABLE DISABLE/ENABLE TRIGGER -func generateTriggerEnabledState(trigger *ir.Trigger, schema, table, targetSchema string, collector *diffCollector) { +func generateTriggerEnabledState(trigger *ir.Trigger, schema, table, targetSchema string, diffType DiffType, collector *diffCollector) { tableName := getTableNameWithSchema(schema, table, targetSchema) state := "ENABLE" if trigger.Disabled { @@ -368,7 +368,7 @@ func generateTriggerEnabledState(trigger *ir.Trigger, schema, table, targetSchem } sql := fmt.Sprintf("ALTER TABLE %s %s TRIGGER %s;", tableName, state, ir.QuoteIdentifier(trigger.Name)) context := &diffContext{ - Type: DiffTypeTableTrigger, + Type: diffType, Operation: DiffOperationAlter, Path: fmt.Sprintf("%s.%s.%s", schema, table, trigger.Name), Source: trigger, diff --git a/internal/diff/view.go b/internal/diff/view.go index 71768d4e..8efbdc85 100644 --- a/internal/diff/view.go +++ b/internal/diff/view.go @@ -435,11 +435,13 @@ func generateModifyViewsSQL(diffs []*viewDiff, targetSchema string, collector *d CanRunInTransaction: true, } collector.collect(createContext, createSQL) - if triggerDiff.New.Comment != "" { + commentChanged := triggerDiff.Old.Comment != triggerDiff.New.Comment + enabledChanged := triggerDiff.Old.Disabled != triggerDiff.New.Disabled + if commentChanged || triggerDiff.New.Comment != "" { generateTriggerComment(triggerDiff.New, diff.New.Schema, diff.New.Name, targetSchema, DiffTypeViewTrigger, collector) } - if triggerDiff.New.Disabled { - generateTriggerEnabledState(triggerDiff.New, diff.New.Schema, diff.New.Name, targetSchema, collector) + if enabledChanged || triggerDiff.New.Disabled { + generateTriggerEnabledState(triggerDiff.New, diff.New.Schema, diff.New.Name, targetSchema, DiffTypeViewTrigger, collector) } } else { sql := generateTriggerSQLWithMode(triggerDiff.New, targetSchema) @@ -451,11 +453,13 @@ func generateModifyViewsSQL(diffs []*viewDiff, targetSchema string, collector *d CanRunInTransaction: true, } collector.collect(context, sql) - if triggerDiff.New.Comment != "" { + commentChanged := triggerDiff.Old.Comment != triggerDiff.New.Comment + enabledChanged := triggerDiff.Old.Disabled != triggerDiff.New.Disabled + if commentChanged || triggerDiff.New.Comment != "" { generateTriggerComment(triggerDiff.New, diff.New.Schema, diff.New.Name, targetSchema, DiffTypeViewTrigger, collector) } - if triggerDiff.New.Disabled { - generateTriggerEnabledState(triggerDiff.New, diff.New.Schema, diff.New.Name, targetSchema, collector) + if enabledChanged || triggerDiff.New.Disabled { + generateTriggerEnabledState(triggerDiff.New, diff.New.Schema, diff.New.Name, targetSchema, DiffTypeViewTrigger, collector) } } }