fix: clean up leaked cluster-level roles in TestIgnorePrivileges

[tianzhou]

Summary

• TestIgnorePrivileges (added in PR feat: add support for privileges in .pgschemaignore (#339) #340) creates cluster-level PostgreSQL roles (app_reader, deploy_bot, admin_role) and ALTER DEFAULT PRIVILEGES rules in the sharedEmbeddedPG instance via the plan command
• Since Go runs tests alphabetically, these persist and contaminate TestPlanAndApply, causing unexpected GRANT statements in plan output for ~100 table/view/index/trigger tests
• Added cleanupSharedEmbeddedPG() that revokes default privileges, drops owned objects, and drops the leaked roles after the privilege test completes

Test plan

• TestIgnorePrivileges still passes
• TestPlanAndApply passes when run after TestIgnorePrivileges (previously failed)
• Full go test -v ./cmd passes (all 150+ integration tests)
• Full go test -v ./internal/diff passes

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR addresses two related concerns: it fixes a test isolation bug where TestIgnorePrivileges leaked cluster-level PostgreSQL roles into the shared embedded PG instance (contaminating subsequent tests like TestPlanAndApply), and it also bundles in support for rendering UPDATE OF column lists in trigger DDL (both detection from pg_get_triggerdef() output and SQL generation).

Changes:

• Added cleanupSharedEmbeddedPG() helper that explicitly revokes default privileges and drops leaked roles (app_reader, deploy_bot, admin_role) after TestIgnorePrivileges completes
• Added UpdateColumns []string field to the Trigger IR struct, with extraction from pg_get_triggerdef() output via extractUpdateColumnsFromTriggerDef, equality comparison in triggersEqual, and correct rendering in generateTriggerSQLWithMode
• Updated testdata/diff/create_trigger/ fixtures to include a new employees_salary_update_trigger (BEFORE UPDATE OF salary) test case and update the alter_trigger test to exercise the INSERT OR UPDATE OF salary event list

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
cmd/ignore_integration_test.go	Adds cleanupSharedEmbeddedPG() and calls it after the privilege subtests to remove leaked cluster-level roles
ir/ir.go	Adds UpdateColumns field to the Trigger struct
ir/inspector.go	Adds extractUpdateColumnsFromTriggerDef helper and populates UpdateColumns in buildTriggers
internal/diff/trigger.go	Adds UpdateColumns equality check and UPDATE OF col1, col2 rendering in SQL generation
testdata/diff/create_trigger/add_trigger/*	Adds employees_salary_update_trigger fixture for the new UPDATE OF trigger feature
testdata/diff/create_trigger/alter_trigger/*	Updates alter_trigger fixtures to reflect INSERT OR UPDATE OF salary event list

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[greptile-apps]

Greptile Summary

This PR fixes a test isolation bug where TestIgnorePrivileges leaked cluster-level PostgreSQL roles (app_reader, deploy_bot, admin_role) into sharedEmbeddedPG, contaminating TestPlanAndApply. It also adds full support for UPDATE OF column_list triggers — new UpdateColumns field on the Trigger IR struct, extraction from pg_get_triggerdef() output, diff/equality logic, and updated SQL generation — along with test fixtures covering both adding and altering column-specific update triggers.

Key changes:

• cleanupSharedEmbeddedPG() revokes default privileges and drops leaked roles after TestIgnorePrivileges runs; the cleanup sequence was validated end-to-end by the test plan.
• extractUpdateColumnsFromTriggerDef() correctly parses UPDATE OF col1, col2 from trigger definitions.
• triggersEqual() compares UpdateColumns in insertion order; since PostgreSQL treats UPDATE OF a, b and UPDATE OF b, a as identical, an order-only change in the schema file would produce an unnecessary CREATE OR REPLACE TRIGGER.
• generateTriggerSQLWithMode() correctly emits UPDATE OF col1, col2 in the event clause when UpdateColumns is non-empty.
• Test fixtures (add_trigger, alter_trigger) are updated to cover the new UPDATE OF syntax end-to-end.

Confidence Score: 4/5

• Safe to merge; the test isolation fix is correct and well-tested, and UPDATE OF support is logically sound for all real-world inputs.
• The PR correctly fixes the test contamination issue (validated by test plan: all 150+ integration tests pass). The UPDATE OF feature is implemented end-to-end with proper SQL generation and test fixtures. The one remaining issue is that trigger equality comparison is order-sensitive on UpdateColumns, which could cause unnecessary trigger recreations if users reorder columns in their schema files — this is a minor architectural rough edge but not a correctness regression.
• internal/diff/trigger.go: UpdateColumns comparison should be order-independent to match PostgreSQL semantics.

Sequence Diagram

sequenceDiagram
    participant Test as TestIgnorePrivileges
    participant PlanCmd as GeneratePlan
    participant EmbPG as sharedEmbeddedPG
    participant Cleanup as cleanupSharedEmbeddedPG

    Test->>PlanCmd: GeneratePlan(config, sharedEmbeddedPG)
    PlanCmd->>EmbPG: ApplySchema(ctx, "public", desiredState SQL)
    Note over EmbPG: CREATE ROLE app_reader/deploy_bot/admin_role<br/>ALTER DEFAULT PRIVILEGES GRANT … TO app_reader<br/>ALTER DEFAULT PRIVILEGES GRANT … TO deploy_bot<br/>(cluster-level — survive schema DROP)
    PlanCmd-->>Test: migrationPlan

    Test->>Cleanup: cleanupSharedEmbeddedPG(t)
    Cleanup->>EmbPG: ALTER DEFAULT PRIVILEGES REVOKE ALL ON TABLES FROM app_reader
    Cleanup->>EmbPG: ALTER DEFAULT PRIVILEGES REVOKE ALL ON TABLES FROM deploy_bot
    Cleanup->>EmbPG: REASSIGN OWNED BY app_reader/deploy_bot/admin_role TO testuser
    Cleanup->>EmbPG: DROP OWNED BY app_reader/deploy_bot/admin_role
    Cleanup->>EmbPG: DROP ROLE IF EXISTS app_reader/deploy_bot/admin_role
    Note over EmbPG: Cluster-level objects removed<br/>sharedEmbeddedPG now clean

    Note right of Test: TestPlanAndApply runs next<br/>No more spurious GRANT statements

Loading

_{Last reviewed commit: d51911b}

[greptile-apps]

Order-sensitive UpdateColumns comparison may cause spurious migrations

UpdateColumns are compared element-by-element in the order they appear. PostgreSQL treats UPDATE OF a, b and UPDATE OF b, a as semantically identical (the trigger fires when any listed column is updated), but this comparison would treat them as different and generate an unnecessary CREATE OR REPLACE TRIGGER. If a user ever reorders columns in their schema file, they'll see unexpected plan output.

Consider sorting both slices before comparing:

Suggested change

	// 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
	}
	}
	// Compare update columns (order-independent, as PostgreSQL treats them as a set)
	if len(old.UpdateColumns) != len(new.UpdateColumns) {
	return false
	}
	oldCols := append([]string(nil), old.UpdateColumns...)
	newCols := append([]string(nil), new.UpdateColumns...)
	sort.Strings(oldCols)
	sort.Strings(newCols)
	for i, col := range oldCols {
	if col != newCols[i] {
	return false
	}
	}

[tianzhou]

Not an issue here. Both desired and current state go through database inspection (pg_get_triggerdef()), which normalizes column order by attnum. pgschema doesn't parse raw SQL — both sides are always inspector-derived, so the column order is deterministic and consistent.

Adding a sort would be defensive but unnecessary given the architecture.