Skip to content

Commit 4fdaff4

Browse files
authored
feat: add support for trigger comments, trigger enabled/disabled state, and sequence comments (#479)
* 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) * 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. * fix: address Greptile review findings and add missing plan test files 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 * 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
1 parent de0db1b commit 4fdaff4

32 files changed

Lines changed: 485 additions & 51 deletions

File tree

internal/diff/diff.go

Lines changed: 25 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -288,6 +288,7 @@ type ddlDiff struct {
288288
droppedTypes []*ir.Type
289289
modifiedTypes []*typeDiff
290290
addedSequences []*ir.Sequence
291+
addedSerialSeqComments []*ir.Sequence // SERIAL-owned sequences skipped from addedSequences but with comments to emit
291292
droppedSequences []*ir.Sequence
292293
modifiedSequences []*sequenceDiff
293294
addedDefaultPrivileges []*ir.DefaultPrivilege
@@ -478,6 +479,7 @@ func GenerateMigration(oldIR, newIR *ir.IR, targetSchema string) []Diff {
478479
droppedTypes: []*ir.Type{},
479480
modifiedTypes: []*typeDiff{},
480481
addedSequences: []*ir.Sequence{},
482+
addedSerialSeqComments: []*ir.Sequence{},
481483
droppedSequences: []*ir.Sequence{},
482484
modifiedSequences: []*sequenceDiff{},
483485
addedDefaultPrivileges: []*ir.DefaultPrivilege{},
@@ -1041,6 +1043,11 @@ func GenerateMigration(oldIR, newIR *ir.IR, targetSchema string) []Diff {
10411043
// (created by SERIAL in CREATE TABLE). If the column already exists,
10421044
// we need to create the sequence explicitly for ALTER COLUMN to use.
10431045
if seq.OwnedByTable != "" && seq.OwnedByColumn != "" && !columnExistsInTables(oldTables, seq.Schema, seq.OwnedByTable, seq.OwnedByColumn) {
1046+
// Sequence is created implicitly by CREATE TABLE (SERIAL). Emit its
1047+
// comment separately after all tables are created.
1048+
if seq.Comment != "" {
1049+
diff.addedSerialSeqComments = append(diff.addedSerialSeqComments, seq)
1050+
}
10441051
continue
10451052
}
10461053
diff.addedSequences = append(diff.addedSequences, seq)
@@ -1064,12 +1071,20 @@ func GenerateMigration(oldIR, newIR *ir.IR, targetSchema string) []Diff {
10641071
for _, key := range seqKeys {
10651072
newSeq := newSequences[key]
10661073
if oldSeq, exists := oldSequences[key]; exists {
1067-
// Skip sequences owned by table columns (created by SERIAL)
1068-
if (oldSeq.OwnedByTable != "" && oldSeq.OwnedByColumn != "") ||
1069-
(newSeq.OwnedByTable != "" && newSeq.OwnedByColumn != "") {
1074+
// Skip sequences owned by table columns (created by SERIAL) for structural changes,
1075+
// but allow comment-only changes through so COMMENT ON SEQUENCE can be deployed.
1076+
isOwned := (oldSeq.OwnedByTable != "" && oldSeq.OwnedByColumn != "") ||
1077+
(newSeq.OwnedByTable != "" && newSeq.OwnedByColumn != "")
1078+
if isOwned {
1079+
if oldSeq.Comment != newSeq.Comment {
1080+
diff.modifiedSequences = append(diff.modifiedSequences, &sequenceDiff{
1081+
Old: oldSeq,
1082+
New: newSeq,
1083+
})
1084+
}
10701085
continue
10711086
}
1072-
if !sequencesEqual(oldSeq, newSeq) {
1087+
if !sequencesEqual(oldSeq, newSeq) || oldSeq.Comment != newSeq.Comment {
10731088
diff.modifiedSequences = append(diff.modifiedSequences, &sequenceDiff{
10741089
Old: oldSeq,
10751090
New: newSeq,
@@ -1818,6 +1833,12 @@ func (d *ddlDiff) generateCreateSQL(targetSchema string, collector *diffCollecto
18181833
// Create tables WITH function/domain dependencies (now that functions and deferred domains exist)
18191834
deferredPolicies2, deferredConstraints2 := generateCreateTablesSQL(tablesWithDeps, targetSchema, collector, existingTables, shouldDeferPolicy, d.suppressedInlineFKs)
18201835

1836+
// Emit COMMENT ON SEQUENCE for sequences created implicitly via CREATE TABLE (SERIAL/BIGSERIAL).
1837+
// These were skipped from addedSequences but their comments must still be deployed.
1838+
for _, seq := range d.addedSerialSeqComments {
1839+
generateSequenceComment(seq, targetSchema, DiffOperationCreate, collector)
1840+
}
1841+
18211842
// Add deferred foreign key constraints from BOTH batches AFTER all tables are created
18221843
// This ensures FK references to tables in the second batch (function-dependent tables) work correctly
18231844
allDeferredConstraints := append(deferredConstraints1, deferredConstraints2...)

internal/diff/sequence.go

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,9 +31,33 @@ func generateCreateSequencesSQL(sequences []*ir.Sequence, targetSchema string, c
3131
}
3232

3333
collector.collect(context, sql)
34+
35+
// Emit COMMENT ON SEQUENCE if present
36+
if seq.Comment != "" {
37+
generateSequenceComment(seq, targetSchema, DiffOperationCreate, collector)
38+
}
3439
}
3540
}
3641

42+
// generateSequenceComment emits a COMMENT ON SEQUENCE statement
43+
func generateSequenceComment(seq *ir.Sequence, targetSchema string, operation DiffOperation, collector *diffCollector) {
44+
seqName := qualifyEntityName(seq.Schema, seq.Name, targetSchema)
45+
var sql string
46+
if seq.Comment == "" {
47+
sql = fmt.Sprintf("COMMENT ON SEQUENCE %s IS NULL;", seqName)
48+
} else {
49+
sql = fmt.Sprintf("COMMENT ON SEQUENCE %s IS %s;", seqName, quoteString(seq.Comment))
50+
}
51+
context := &diffContext{
52+
Type: DiffTypeSequence,
53+
Operation: operation,
54+
Path: fmt.Sprintf("%s.%s", seq.Schema, seq.Name),
55+
Source: seq,
56+
CanRunInTransaction: true,
57+
}
58+
collector.collect(context, sql)
59+
}
60+
3761
// generateDropSequencesSQL generates DROP SEQUENCE statements
3862
func generateDropSequencesSQL(sequences []*ir.Sequence, targetSchema string, collector *diffCollector) {
3963
// Process sequences in reverse order (already sorted)
@@ -71,6 +95,11 @@ func generateModifySequencesSQL(diffs []*sequenceDiff, targetSchema string, coll
7195

7296
collector.collect(context, stmt)
7397
}
98+
99+
// Emit COMMENT ON SEQUENCE if comment changed
100+
if diff.Old.Comment != diff.New.Comment {
101+
generateSequenceComment(diff.New, targetSchema, DiffOperationAlter, collector)
102+
}
74103
}
75104
}
76105

internal/diff/table.go

Lines changed: 56 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -98,10 +98,13 @@ func diffTriggers(oldTable, newTable *ir.Table, diff *tableDiff) {
9898
}
9999
}
100100

101-
// Find modified triggers
101+
// Find modified triggers (structural changes, comment-only, or enabled-state-only)
102102
for name, newTrigger := range newTriggers {
103103
if oldTrigger, exists := oldTriggers[name]; exists {
104-
if !triggersEqual(oldTrigger, newTrigger) {
104+
structurallyEqual := triggersEqual(oldTrigger, newTrigger)
105+
commentChanged := oldTrigger.Comment != newTrigger.Comment
106+
enabledChanged := oldTrigger.Disabled != newTrigger.Disabled
107+
if !structurallyEqual || commentChanged || enabledChanged {
105108
diff.ModifiedTriggers = append(diff.ModifiedTriggers, &triggerDiff{
106109
Old: oldTrigger,
107110
New: newTrigger,
@@ -1383,43 +1386,61 @@ func (td *tableDiff) generateAlterTableStatements(targetSchema string, collector
13831386

13841387
// Modify triggers - already sorted by the Diff operation
13851388
for _, triggerDiff := range td.ModifiedTriggers {
1386-
// Constraint triggers don't support CREATE OR REPLACE, so we need to DROP and CREATE
1387-
if triggerDiff.New.IsConstraint {
1388-
tableName := getTableNameWithSchema(td.Table.Schema, td.Table.Name, targetSchema)
1389+
structurallyEqual := triggersEqual(triggerDiff.Old, triggerDiff.New)
1390+
commentChanged := triggerDiff.Old.Comment != triggerDiff.New.Comment
1391+
enabledChanged := triggerDiff.Old.Disabled != triggerDiff.New.Disabled
1392+
1393+
if !structurallyEqual {
1394+
// Constraint triggers don't support CREATE OR REPLACE, so we need to DROP and CREATE
1395+
if triggerDiff.New.IsConstraint {
1396+
tableName := getTableNameWithSchema(td.Table.Schema, td.Table.Name, targetSchema)
1397+
1398+
// Step 1: DROP the old trigger
1399+
dropSQL := fmt.Sprintf("DROP TRIGGER IF EXISTS %s ON %s;", ir.QuoteIdentifier(triggerDiff.Old.Name), tableName)
1400+
dropContext := &diffContext{
1401+
Type: DiffTypeTableTrigger,
1402+
Operation: DiffOperationDrop,
1403+
Path: fmt.Sprintf("%s.%s.%s", td.Table.Schema, td.Table.Name, triggerDiff.Old.Name),
1404+
Source: triggerDiff.Old,
1405+
CanRunInTransaction: true,
1406+
}
1407+
collector.collect(dropContext, dropSQL)
13891408

1390-
// Step 1: DROP the old trigger
1391-
dropSQL := fmt.Sprintf("DROP TRIGGER IF EXISTS %s ON %s;", ir.QuoteIdentifier(triggerDiff.Old.Name), tableName)
1392-
dropContext := &diffContext{
1393-
Type: DiffTypeTableTrigger,
1394-
Operation: DiffOperationDrop,
1395-
Path: fmt.Sprintf("%s.%s.%s", td.Table.Schema, td.Table.Name, triggerDiff.Old.Name),
1396-
Source: triggerDiff.Old,
1397-
CanRunInTransaction: true,
1398-
}
1399-
collector.collect(dropContext, dropSQL)
1409+
// Step 2: CREATE the new constraint trigger
1410+
createSQL := generateTriggerSQLWithMode(triggerDiff.New, targetSchema)
1411+
createContext := &diffContext{
1412+
Type: DiffTypeTableTrigger,
1413+
Operation: DiffOperationCreate,
1414+
Path: fmt.Sprintf("%s.%s.%s", td.Table.Schema, td.Table.Name, triggerDiff.New.Name),
1415+
Source: triggerDiff.New,
1416+
CanRunInTransaction: true,
1417+
}
1418+
collector.collect(createContext, createSQL)
1419+
} else {
1420+
// Use CREATE OR REPLACE for regular triggers
1421+
sql := generateTriggerSQLWithMode(triggerDiff.New, targetSchema)
14001422

1401-
// Step 2: CREATE the new constraint trigger
1402-
createSQL := generateTriggerSQLWithMode(triggerDiff.New, targetSchema)
1403-
createContext := &diffContext{
1404-
Type: DiffTypeTableTrigger,
1405-
Operation: DiffOperationCreate,
1406-
Path: fmt.Sprintf("%s.%s.%s", td.Table.Schema, td.Table.Name, triggerDiff.New.Name),
1407-
Source: triggerDiff.New,
1408-
CanRunInTransaction: true,
1423+
context := &diffContext{
1424+
Type: DiffTypeTableTrigger,
1425+
Operation: DiffOperationAlter,
1426+
Path: fmt.Sprintf("%s.%s.%s", td.Table.Schema, td.Table.Name, triggerDiff.New.Name),
1427+
Source: triggerDiff,
1428+
CanRunInTransaction: true,
1429+
}
1430+
collector.collect(context, sql)
14091431
}
1410-
collector.collect(createContext, createSQL)
1411-
} else {
1412-
// Use CREATE OR REPLACE for regular triggers
1413-
sql := generateTriggerSQLWithMode(triggerDiff.New, targetSchema)
1432+
}
14141433

1415-
context := &diffContext{
1416-
Type: DiffTypeTableTrigger,
1417-
Operation: DiffOperationAlter,
1418-
Path: fmt.Sprintf("%s.%s.%s", td.Table.Schema, td.Table.Name, triggerDiff.New.Name),
1419-
Source: triggerDiff,
1420-
CanRunInTransaction: true,
1421-
}
1422-
collector.collect(context, sql)
1434+
// Emit COMMENT ON TRIGGER when the comment changed, or after structural recreation
1435+
// so the desired comment is preserved.
1436+
if commentChanged || (!structurallyEqual && triggerDiff.New.Comment != "") {
1437+
generateTriggerComment(triggerDiff.New, td.Table.Schema, td.Table.Name, targetSchema, DiffTypeTableTrigger, collector)
1438+
}
1439+
1440+
// Emit ENABLE/DISABLE TRIGGER when the state changed, or after structural recreation
1441+
// so disabled triggers stay disabled.
1442+
if enabledChanged || (!structurallyEqual && triggerDiff.New.Disabled) {
1443+
generateTriggerEnabledState(triggerDiff.New, td.Table.Schema, td.Table.Name, targetSchema, DiffTypeTableTrigger, collector)
14231444
}
14241445
}
14251446

internal/diff/trigger.go

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,16 @@ func generateCreateTriggersSQL(triggers []*ir.Trigger, targetSchema string, coll
148148
}
149149

150150
collector.collect(context, sql)
151+
152+
// Emit COMMENT ON TRIGGER if present
153+
if trigger.Comment != "" {
154+
generateTriggerComment(trigger, trigger.Schema, trigger.Table, targetSchema, DiffTypeTableTrigger, collector)
155+
}
156+
157+
// Emit DISABLE TRIGGER if the trigger is disabled
158+
if trigger.Disabled {
159+
generateTriggerEnabledState(trigger, trigger.Schema, trigger.Table, targetSchema, DiffTypeTableTrigger, collector)
160+
}
151161
}
152162
}
153163

@@ -319,7 +329,50 @@ func generateCreateViewTriggersSQL(triggers []*ir.Trigger, targetSchema string,
319329
}
320330

321331
collector.collect(context, sql)
332+
333+
if trigger.Comment != "" {
334+
generateTriggerComment(trigger, trigger.Schema, trigger.Table, targetSchema, DiffTypeViewTrigger, collector)
335+
}
336+
337+
if trigger.Disabled {
338+
generateTriggerEnabledState(trigger, trigger.Schema, trigger.Table, targetSchema, DiffTypeViewTrigger, collector)
339+
}
322340
}
323341
}
324342

343+
// generateTriggerComment emits a COMMENT ON TRIGGER statement
344+
func generateTriggerComment(trigger *ir.Trigger, schema, table, targetSchema string, diffType DiffType, collector *diffCollector) {
345+
tableName := getTableNameWithSchema(schema, table, targetSchema)
346+
var sql string
347+
if trigger.Comment == "" {
348+
sql = fmt.Sprintf("COMMENT ON TRIGGER %s ON %s IS NULL;", ir.QuoteIdentifier(trigger.Name), tableName)
349+
} else {
350+
sql = fmt.Sprintf("COMMENT ON TRIGGER %s ON %s IS %s;", ir.QuoteIdentifier(trigger.Name), tableName, quoteString(trigger.Comment))
351+
}
352+
context := &diffContext{
353+
Type: diffType,
354+
Operation: DiffOperationAlter,
355+
Path: fmt.Sprintf("%s.%s.%s", schema, table, trigger.Name),
356+
Source: trigger,
357+
CanRunInTransaction: true,
358+
}
359+
collector.collect(context, sql)
360+
}
325361

362+
// generateTriggerEnabledState emits ALTER TABLE DISABLE/ENABLE TRIGGER
363+
func generateTriggerEnabledState(trigger *ir.Trigger, schema, table, targetSchema string, diffType DiffType, collector *diffCollector) {
364+
tableName := getTableNameWithSchema(schema, table, targetSchema)
365+
state := "ENABLE"
366+
if trigger.Disabled {
367+
state = "DISABLE"
368+
}
369+
sql := fmt.Sprintf("ALTER TABLE %s %s TRIGGER %s;", tableName, state, ir.QuoteIdentifier(trigger.Name))
370+
context := &diffContext{
371+
Type: diffType,
372+
Operation: DiffOperationAlter,
373+
Path: fmt.Sprintf("%s.%s.%s", schema, table, trigger.Name),
374+
Source: trigger,
375+
CanRunInTransaction: true,
376+
}
377+
collector.collect(context, sql)
378+
}

internal/diff/view.go

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -435,6 +435,14 @@ func generateModifyViewsSQL(diffs []*viewDiff, targetSchema string, collector *d
435435
CanRunInTransaction: true,
436436
}
437437
collector.collect(createContext, createSQL)
438+
commentChanged := triggerDiff.Old.Comment != triggerDiff.New.Comment
439+
enabledChanged := triggerDiff.Old.Disabled != triggerDiff.New.Disabled
440+
if commentChanged || triggerDiff.New.Comment != "" {
441+
generateTriggerComment(triggerDiff.New, diff.New.Schema, diff.New.Name, targetSchema, DiffTypeViewTrigger, collector)
442+
}
443+
if enabledChanged || triggerDiff.New.Disabled {
444+
generateTriggerEnabledState(triggerDiff.New, diff.New.Schema, diff.New.Name, targetSchema, DiffTypeViewTrigger, collector)
445+
}
438446
} else {
439447
sql := generateTriggerSQLWithMode(triggerDiff.New, targetSchema)
440448
context := &diffContext{
@@ -445,6 +453,14 @@ func generateModifyViewsSQL(diffs []*viewDiff, targetSchema string, collector *d
445453
CanRunInTransaction: true,
446454
}
447455
collector.collect(context, sql)
456+
commentChanged := triggerDiff.Old.Comment != triggerDiff.New.Comment
457+
enabledChanged := triggerDiff.Old.Disabled != triggerDiff.New.Disabled
458+
if commentChanged || triggerDiff.New.Comment != "" {
459+
generateTriggerComment(triggerDiff.New, diff.New.Schema, diff.New.Name, targetSchema, DiffTypeViewTrigger, collector)
460+
}
461+
if enabledChanged || triggerDiff.New.Disabled {
462+
generateTriggerEnabledState(triggerDiff.New, diff.New.Schema, diff.New.Name, targetSchema, DiffTypeViewTrigger, collector)
463+
}
448464
}
449465
}
450466
}

ir/inspector.go

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -881,13 +881,19 @@ func (i *Inspector) buildSequences(ctx context.Context, schema *IR, targetSchema
881881
}
882882
}
883883

884+
seqComment := ""
885+
if seq.SequenceComment.Valid {
886+
seqComment = seq.SequenceComment.String
887+
}
888+
884889
sequence := &Sequence{
885890
Schema: schemaName,
886891
Name: sequenceName,
887892
DataType: dataType,
888893
StartValue: seq.StartValue.Int64,
889894
Increment: seq.Increment.Int64,
890895
CycleOption: seq.CycleOption.Bool,
896+
Comment: seqComment,
891897
}
892898

893899
// Set default values if not valid
@@ -1725,6 +1731,15 @@ func (i *Inspector) buildTriggers(ctx context.Context, schema *IR, targetSchema
17251731
comment = triggerRow.TriggerComment.String
17261732
}
17271733

1734+
// Extract disabled state: tgenabled 'D' = disabled, anything else = enabled (Postgres default)
1735+
disabled := false
1736+
switch v := triggerRow.TriggerEnabled.(type) {
1737+
case string:
1738+
disabled = v == "D"
1739+
case []byte:
1740+
disabled = string(v) == "D"
1741+
}
1742+
17281743
// Determine if this is a constraint trigger
17291744
oid, ok := triggerRow.TriggerConstraintOid.(int64)
17301745
isConstraint := ok && oid != 0
@@ -1748,6 +1763,7 @@ func (i *Inspector) buildTriggers(ctx context.Context, schema *IR, targetSchema
17481763
Deferrable: deferrable,
17491764
InitiallyDeferred: initDeferred,
17501765
Comment: comment,
1766+
Disabled: disabled,
17511767
}
17521768

17531769
// Add trigger to the appropriate map

ir/ir.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -298,6 +298,7 @@ type Trigger struct {
298298
InitiallyDeferred bool `json:"initially_deferred,omitempty"` // Whether deferred by default
299299
OldTable string `json:"old_table,omitempty"` // REFERENCING OLD TABLE AS name
300300
NewTable string `json:"new_table,omitempty"` // REFERENCING NEW TABLE AS name
301+
Disabled bool `json:"disabled,omitempty"` // true = DISABLED (tgenabled='D'); omitted/false = enabled (Postgres default)
301302
}
302303

303304
// TriggerTiming represents the timing of trigger execution

0 commit comments

Comments
 (0)