Skip to content

Conversation

@asonawalla
Copy link
Contributor

Summary

  • Fixes infinite plan loop when revoking/granting default privileges owned by a role other than the current user
  • Always includes FOR ROLE <owner> in generated ALTER DEFAULT PRIVILEGES DDL

Problem

When pgschema generated DDL for default privileges owned by a different role, it omitted the FOR ROLE clause:

-- What pgschema generated (wrong):
ALTER DEFAULT PRIVILEGES IN SCHEMA public REVOKE SELECT ON TABLES FROM app_user;

-- What PostgreSQL needs:
ALTER DEFAULT PRIVILEGES FOR ROLE other_role IN SCHEMA public REVOKE SELECT ON TABLES FROM app_user;

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

  1. Add OwnerRole field to DefaultPrivilege struct
  2. Extract defaclrole from pg_default_acl in the inspector query
  3. Include owner role in diff comparison key (privileges with different owners are different)
  4. Always generate DDL with FOR ROLE clause

Test plan

  • Updated existing default_privilege tests to expect FOR ROLE in output
  • Converted drop_privilege test to cross-role scenario (privileges owned by test_admin, revoked by testuser)
  • All 8 default_privilege tests pass

🤖 Generated with Claude Code

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>
@tianzhou tianzhou requested review from Copilot and tianzhou January 12, 2026 03:50
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>
@tianzhou
Copy link
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

Copy link
Contributor

Copilot AI left a 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 OwnerRole field to the DefaultPrivilege struct and updated all related logic to track and use the owner role
  • Modified SQL queries to extract defaclrole from pg_default_acl and 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.

@asonawalla
Copy link
Contributor Author

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

Yep, that was the missing piece - thanks!

Copy link
Contributor

@tianzhou tianzhou left a 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.

@tianzhou tianzhou merged commit f0c6cc3 into pgplex:main Jan 12, 2026
1 check passed
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants