fix: include FOR ROLE in ALTER DEFAULT PRIVILEGES DDL#236
Merged
tianzhou merged 3 commits intopgplex:mainfrom Jan 12, 2026
Merged
fix: include FOR ROLE in ALTER DEFAULT PRIVILEGES DDL#236tianzhou merged 3 commits intopgplex:mainfrom
tianzhou merged 3 commits intopgplex:mainfrom
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>
Contributor
|
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 |
Contributor
There was a problem hiding this comment.
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.
Contributor
Author
Yep, that was the missing piece - thanks! |
tianzhou
approved these changes
Jan 12, 2026
Contributor
tianzhou
left a comment
There was a problem hiding this comment.
LGTM. Thanks for the fix.
alecthomas
pushed a commit
to alecthomas/pgschema
that referenced
this pull request
Jan 26, 2026
* 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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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