feat: add support for trigger UPDATE OF columns (#342)#344
Conversation
Triggers with column-specific UPDATE events (e.g., UPDATE OF email) were losing the column specification during inspection, causing incorrect migration plans that would fire triggers on all updates instead of only on specified column changes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes schema inspection and diff generation for PostgreSQL triggers that use column-specific UPDATE events (UPDATE OF col1, col2), ensuring the column list is preserved and emitted in generated DDL.
Changes:
- Added
UpdateColumnsto the trigger IR and included it in trigger equality checks. - Extracted
UPDATE OF ...columns frompg_get_triggerdef()output during inspection. - Updated trigger SQL generation and updated trigger diff test fixtures to include
UPDATE OF salary.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
ir/ir.go |
Extends Trigger IR with UpdateColumns for column-specific UPDATE triggers. |
ir/inspector.go |
Extracts UPDATE OF columns from pg_get_triggerdef() and populates IR. |
internal/diff/trigger.go |
Compares UpdateColumns and generates UPDATE OF ... in trigger DDL. |
testdata/diff/create_trigger/** |
Updates expected plans and SQL fixtures to cover UPDATE OF salary behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ir/inspector.go
Outdated
|
|
||
| // Extract UPDATE OF columns from trigger definition | ||
| var updateColumns []string | ||
| if triggerRow.TriggerDefinition.Valid { |
There was a problem hiding this comment.
buildTriggers extracts UpdateColumns from the full trigger definition regardless of whether the trigger actually has an UPDATE event. If the definition contains the substring UPDATE OF later (e.g., inside a WHEN expression string literal), extractUpdateColumnsFromTriggerDef may return non-nil columns and cause spurious diffs even for INSERT/DELETE-only triggers. Consider only calling extractUpdateColumnsFromTriggerDef when events includes TriggerEventUpdate (decoded from tgtype).
| if triggerRow.TriggerDefinition.Valid { | |
| if triggerRow.TriggerDefinition.Valid && (events&TriggerEventUpdate) != 0 { |
| // Find " ON " which terminates the event/column list | ||
| onIdx := strings.Index(upper[start:], " ON ") | ||
| if onIdx == -1 { | ||
| return nil | ||
| } | ||
|
|
||
| // Extract the column list substring | ||
| colListStr := strings.TrimSpace(triggerDef[start : start+onIdx]) | ||
|
|
||
| // Handle potential " OR " separating additional events after the columns | ||
| // e.g., "UPDATE OF col1, col2 OR DELETE ON ..." | ||
| // We need to check if there's an OR followed by another event keyword | ||
| eventKeywords := []string{"INSERT", "UPDATE", "DELETE", "TRUNCATE"} | ||
| parts := strings.Split(colListStr, " OR ") | ||
| // Only keep the first part (column list); the rest would be other events | ||
| if len(parts) > 1 { | ||
| // Check if subsequent parts are event keywords | ||
| for i := 1; i < len(parts); i++ { | ||
| trimmed := strings.TrimSpace(strings.ToUpper(parts[i])) | ||
| isEvent := false | ||
| for _, kw := range eventKeywords { | ||
| if trimmed == kw || strings.HasPrefix(trimmed, kw+" ") { | ||
| isEvent = true | ||
| break | ||
| } | ||
| } | ||
| if isEvent { | ||
| colListStr = strings.TrimSpace(parts[0]) | ||
| break | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Split by comma and trim each column name | ||
| rawCols := strings.Split(colListStr, ",") | ||
| var columns []string | ||
| for _, col := range rawCols { | ||
| col = strings.TrimSpace(col) | ||
| if col != "" { |
There was a problem hiding this comment.
extractUpdateColumnsFromTriggerDef relies on naive substring searches/splits (e.g., locating the end via " ON " and splitting on commas). This breaks for valid trigger defs where UPDATE columns are quoted identifiers containing ON or , (both allowed inside double-quoted identifiers), and it can also be confused by string literals containing UPDATE OF ... ON .... Consider extracting update-column attnums from pg_trigger.tgattr (joining pg_attribute for names) instead of parsing pg_get_triggerdef(), or implement a small tokenizer that respects quotes/literals when scanning for ON and commas.
Greptile SummaryThis PR fixes a correctness bug where column-specific
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["pg_get_triggerdef() output"] --> B["extractUpdateColumnsFromTriggerDef()"]
B --> C{Contains\n'UPDATE OF '?}
C -- No --> D["return nil\n(no column filter)"]
C -- Yes --> E["Find ' ON ' boundary"]
E --> F["Extract colListStr\n(original case)"]
F --> G{Contains\n' OR '?}
G -- Yes --> H["Split by ' OR '\nCheck for event keywords"]
H --> I["Trim to first part\n(column list only)"]
G -- No --> J["Split by ','"]
I --> J
J --> K["Trim each column name"]
K --> L["UpdateColumns []string\nstored in Trigger IR"]
L --> M["triggersEqual()"]
M --> N{len match?}
N -- No --> O["return false\n→ ALTER TRIGGER"]
N -- Yes --> P["Order-sensitive\nindex comparison"]
P --> Q{All equal?}
Q -- No --> O
Q -- Yes --> R["continue checks..."]
L --> S["generateTriggerSQLWithMode()"]
S --> T{UpdateColumns\nnon-empty?}
T -- Yes --> U["'UPDATE OF col1, col2'"]
T -- No --> V["'UPDATE'"]
U --> W["Final SQL emitted"]
V --> W
Last reviewed commit: 00bd4ca |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| parts := strings.Split(colListStr, " OR ") | ||
| // Only keep the first part (column list); the rest would be other events | ||
| if len(parts) > 1 { | ||
| // Check if subsequent parts are event keywords | ||
| for i := 1; i < len(parts); i++ { | ||
| trimmed := strings.TrimSpace(strings.ToUpper(parts[i])) | ||
| isEvent := false | ||
| for _, kw := range eventKeywords { | ||
| if trimmed == kw || strings.HasPrefix(trimmed, kw+" ") { | ||
| isEvent = true | ||
| break | ||
| } | ||
| } | ||
| if isEvent { | ||
| colListStr = strings.TrimSpace(parts[0]) | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
OR-split uses original case, not the uppercased copy
colListStr is sliced from the original triggerDef (mixed case), but the split is done with " OR " which is an uppercase literal. Since pg_get_triggerdef() always returns SQL keywords in uppercase this works today, but the same function already computes upper for exactly this kind of case-insensitive matching. Using the raw string means any non-standard or future lowercase representation would silently skip the trim step and leave event keywords like delete in the column list.
| parts := strings.Split(colListStr, " OR ") | |
| // Only keep the first part (column list); the rest would be other events | |
| if len(parts) > 1 { | |
| // Check if subsequent parts are event keywords | |
| for i := 1; i < len(parts); i++ { | |
| trimmed := strings.TrimSpace(strings.ToUpper(parts[i])) | |
| isEvent := false | |
| for _, kw := range eventKeywords { | |
| if trimmed == kw || strings.HasPrefix(trimmed, kw+" ") { | |
| isEvent = true | |
| break | |
| } | |
| } | |
| if isEvent { | |
| colListStr = strings.TrimSpace(parts[0]) | |
| break | |
| } | |
| } | |
| parts := strings.Split(strings.ToUpper(colListStr), " OR ") | |
| // Only keep the first part (column list); the rest would be other events | |
| if len(parts) > 1 { | |
| // Check if subsequent parts are event keywords | |
| for i := 1; i < len(parts); i++ { | |
| trimmed := strings.TrimSpace(parts[i]) | |
| isEvent := false | |
| for _, kw := range eventKeywords { | |
| if trimmed == kw || strings.HasPrefix(trimmed, kw+" ") { | |
| isEvent = true | |
| break | |
| } | |
| } | |
| if isEvent { | |
| // Slice colListStr at the corresponding byte offset instead | |
| orIdx := strings.Index(strings.ToUpper(colListStr), " OR "+trimmed) | |
| if orIdx != -1 { | |
| colListStr = strings.TrimSpace(colListStr[:orIdx]) | |
| } | |
| break | |
| } | |
| } | |
| } |
| // Compare update columns | ||
| if len(old.UpdateColumns) != len(new.UpdateColumns) { | ||
| return false | ||
| } | ||
| for i, col := range old.UpdateColumns { | ||
| if col != new.UpdateColumns[i] { | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
Order-sensitive UpdateColumns comparison may produce spurious diffs
normalizeTriggerEvents sorts events into a canonical order to prevent false positives when the same set of events is listed differently in two IRs. UpdateColumns has no analogous normalization: if the same columns are listed in a different order (e.g., UPDATE OF a, b vs UPDATE OF b, a) — both semantically identical in PostgreSQL — triggersEqual returns false and an unnecessary ALTER TRIGGER is emitted.
Consider either sorting UpdateColumns in normalizeTrigger (same file as normalizeTriggerEvents), or doing a set-comparison here:
// Compare update columns (order-independent)
if len(old.UpdateColumns) != len(new.UpdateColumns) {
return false
}
oldCols := make(map[string]struct{}, len(old.UpdateColumns))
for _, col := range old.UpdateColumns {
oldCols[col] = struct{}{}
}
for _, col := range new.UpdateColumns {
if _, ok := oldCols[col]; !ok {
return false
}
}Only extract UPDATE OF columns when the trigger actually has an UPDATE event, preventing false positives if the substring appears elsewhere. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
UPDATE OF col1, col2) were losing the column specification during schema inspection, causingpgschema planto generate incorrect DDL (UPDATEinstead ofUPDATE OF email)UpdateColumnsfield to the Trigger IR struct, extraction frompg_get_triggerdef()output in the inspector, and proper SQL generation in the diff engineFixes #342
Test plan
add_triggertest case with a column-specificUPDATE OF salarytriggeralter_triggertest case to useUPDATE OF salaryPGSCHEMA_TEST_FILTER="create_trigger/" go test -v ./internal/diff -run TestDiffFromFilesPGSCHEMA_TEST_FILTER="create_trigger/" go test -v ./cmd -run TestPlanAndApply🤖 Generated with Claude Code