-
Notifications
You must be signed in to change notification settings - Fork 29
fix: include FOR ROLE in ALTER DEFAULT PRIVILEGES DDL #236
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: include FOR ROLE in ALTER DEFAULT PRIVILEGES DDL #236
Conversation
When generating DDL for default privileges owned by a role other than the current user, pgschema was omitting the FOR ROLE clause. This caused PostgreSQL to apply the statement to the current user's defaults instead of the owning role's, resulting in an infinite plan loop where the same changes would be detected repeatedly. Changes: - Add OwnerRole field to DefaultPrivilege struct in IR - Update pg_default_acl query to extract defaclrole (owning role) - Include owner role in diff comparison key - Generate DDL with FOR ROLE clause: ALTER DEFAULT PRIVILEGES FOR ROLE <owner> IN SCHEMA ... - Add cross-role test case (drop_privilege) to verify the fix The fix always includes FOR ROLE in generated DDL, which is explicit and correct regardless of which user runs the migration. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The integration tests (TestPlanAndApply) use plan.sql, plan.txt, and plan.json files as expected output, not the diff.sql files used by unit tests. This updates all default_privilege integration test expectations to include FOR ROLE in the generated DDL. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Pls check the failed test. You may need to regenerate the plan as well PGSCHEMA_TEST_FILTER="default_privilege/" go test -v ./cmd -run TestPlanAndApply --generate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes an infinite plan loop bug when managing default privileges owned by roles other than the current user. The issue occurred because the generated DDL omitted the FOR ROLE clause, causing PostgreSQL to apply changes to the wrong role's privileges.
Changes:
- Added
OwnerRolefield to theDefaultPrivilegestruct and updated all related logic to track and use the owner role - Modified SQL queries to extract
defaclrolefrompg_default_acland include it in the result set - Updated all DDL generation to always include
FOR ROLE <owner>in ALTER DEFAULT PRIVILEGES statements
Reviewed changes
Copilot reviewed 40 out of 40 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| ir/queries/queries.sql | Added owner_role extraction via pg_get_userbyid(defaclrole) to GetDefaultPrivilegesForSchema query |
| ir/queries/queries.sql.go | Auto-generated code reflecting the query changes with new OwnerRole field |
| ir/ir.go | Added OwnerRole field to DefaultPrivilege struct and updated GetObjectName to include owner in key |
| ir/inspector.go | Updated privilege grouping to include owner_role in key and sorting logic |
| internal/diff/diff.go | Updated diff key generation to include owner_role for proper matching between old/new privileges |
| internal/diff/default_privilege.go | Updated all DDL generation functions to include FOR ROLE clause; updated equality check to compare owner roles |
| testdata/diff/default_privilege/*/plan.txt | Updated test expectations to include FOR ROLE testuser/test_admin in DDL output |
| testdata/diff/default_privilege/*/plan.sql | Updated test expectations to include FOR ROLE clause |
| testdata/diff/default_privilege/*/plan.json | Updated test expectations with new path format including owner_role |
| testdata/diff/default_privilege/*/diff.sql | Updated expected diff output to include FOR ROLE clause |
| testdata/diff/default_privilege/drop_privilege/old.sql | Enhanced test to use cross-role scenario with test_admin owning privileges |
| testdata/diff/default_privilege/drop_privilege/new.sql | Enhanced test setup to support cross-role testing |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Yep, that was the missing piece - thanks! |
tianzhou
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for the fix.
* fix: include FOR ROLE in ALTER DEFAULT PRIVILEGES DDL When generating DDL for default privileges owned by a role other than the current user, pgschema was omitting the FOR ROLE clause. This caused PostgreSQL to apply the statement to the current user's defaults instead of the owning role's, resulting in an infinite plan loop where the same changes would be detected repeatedly. Changes: - Add OwnerRole field to DefaultPrivilege struct in IR - Update pg_default_acl query to extract defaclrole (owning role) - Include owner role in diff comparison key - Generate DDL with FOR ROLE clause: ALTER DEFAULT PRIVILEGES FOR ROLE <owner> IN SCHEMA ... - Add cross-role test case (drop_privilege) to verify the fix The fix always includes FOR ROLE in generated DDL, which is explicit and correct regardless of which user runs the migration. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: update integration test expectations for FOR ROLE clause The integration tests (TestPlanAndApply) use plan.sql, plan.txt, and plan.json files as expected output, not the diff.sql files used by unit tests. This updates all default_privilege integration test expectations to include FOR ROLE in the generated DDL. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * run TestPlanAndApply -generate --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
Summary
FOR ROLE <owner>in generatedALTER DEFAULT PRIVILEGESDDLProblem
When pgschema generated DDL for default privileges owned by a different role, it omitted the
FOR ROLEclause:Without
FOR ROLE, PostgreSQL applies the statement to the current user's default privileges, not the owning role's. The statement executes "successfully" but does nothing, causing pgschema to detect the same changes on the next plan.Solution
OwnerRolefield toDefaultPrivilegestructdefaclrolefrompg_default_aclin the inspector queryFOR ROLEclauseTest plan
FOR ROLEin outputdrop_privilegetest to cross-role scenario (privileges owned bytest_admin, revoked bytestuser)🤖 Generated with Claude Code