fix: stub roles for ALTER DEFAULT PRIVILEGES in external plan DB

[tianzhou]

Summary

• External plan DB rejects ALTER DEFAULT PRIVILEGES FOR ROLE <role> with "permission denied" (SQLSTATE 42501) because the plan user is not a member of the referenced role
• Fix: parse role names from desired SQL via ExtractDefaultPrivilegeRoles, create stub roles + grant membership to plan user before applying schema, clean up in Stop()
• Added ExtractDefaultPrivilegeRoles parser with 5 test cases (single, quoted, multiple/deduped, no match, case insensitive)

Fixes #553

Test plan

• Unit tests for ExtractDefaultPrivilegeRoles (5 cases, all pass)
• go build ./... passes
• go vet ./... passes
• @schema-codex review

🤖 Generated with Claude Code

[greptile-apps]

Greptile Summary

The PR extracts owners from ALTER DEFAULT PRIVILEGES statements and creates temporary role memberships so desired SQL can run against an external plan database.

• Adds default-privilege role extraction and unit coverage.
• Creates and grants stub roles before applying desired SQL.
• Attempts to remove recorded roles during external database shutdown.

Confidence Score: 2/5

The PR is not safe to merge until stub-role ownership and cleanup are corrected and all supported default-privilege role forms are extracted.

The changed cleanup can delete or retain privileges for persistent cluster roles, while valid FOR USER and multi-role statements continue to fail in the external planning path.

Files Needing Attention: internal/postgres/external.go, internal/postgres/fk_refs.go

Security Review

The cleanup bookkeeping crosses an authorization ownership boundary: it can attempt to delete pre-existing cluster roles, retain newly granted memberships when deletion fails, or leak roles and memberships after partial setup failure.

Important Files Changed

Filename	Overview
internal/postgres/external.go	Adds external-plan role setup and cleanup, but cleanup does not track ownership or partial progress and can mutate persistent cluster authorization state.
internal/postgres/fk_refs.go	Adds role extraction but omits PostgreSQL-supported FOR USER and comma-separated role-list forms.
internal/postgres/fk_refs_test.go	Covers basic single-role extraction and deduplication but lacks valid alternate-keyword and multi-role grammar cases.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  SQL[Desired schema SQL] --> Parse[Extract default-privilege roles]
  Parse --> Create[Create role or accept duplicate]
  Create --> Grant[Grant role to plan user]
  Grant --> Apply[Apply desired schema]
  Apply --> Stop[Stop external provider]
  Stop --> Drop[Drop every recorded role]
  Create -. pre-existing role .-> Drop
  Grant -. failed drop leaves membership .-> Persist[Persistent authorization change]
  Create -. later setup failure before recording .-> Leak[Role or membership leak]

Loading

_{Reviews (1): Last reviewed commit: "fix: stub roles for ALTER DEFAULT PRIVIL..." | Re-trigger Greptile}

[greptile-apps]

Cleanup loses role ownership state

If a referenced role already exists in the persistent plan database, this code records it for cleanup and Stop attempts to delete a cluster role that this run did not create; when deletion fails because the role has dependencies, the newly granted membership remains because cleanup never revokes it. A setup error on a later role also returns before ed.stubRoles is assigned, so roles and memberships created earlier in the loop persist after Stop.

How this was verified: The duplicate-role path is suppressed, cleanup drops every recorded name, and no membership revocation or incremental creation tracking exists.

Knowledge Base Used: Postgres Embedded/External Validation (internal/postgres)

[greptile-apps]

Valid role forms remain unstubbed

When desired SQL uses PostgreSQL's valid FOR USER role form or a comma-separated FOR ROLE role1, role2 list, the extractor either skips the clause or captures only its first role. The external plan database therefore executes the original statement without creating and granting every referenced role, causing planning to fail with a missing-role or permission error.

Knowledge Base Used: Postgres Embedded/External Validation (internal/postgres)

[Copilot]

Pull request overview

This PR addresses failures when generating plans against an external plan database where ALTER DEFAULT PRIVILEGES FOR ROLE <role> can be rejected with SQLSTATE 42501 if the plan user is not a member of the referenced role (issue #553). It introduces role extraction from desired SQL and attempts to provision role stubs/memberships so the desired schema can be applied in the plan DB.

Changes:

• Added ExtractDefaultPrivilegeRoles SQL scanner to find roles referenced by ALTER DEFAULT PRIVILEGES ... FOR ROLE ....
• Updated external plan DB schema-apply flow to create stub roles and grant membership to the plan user, then clean up in Stop().
• Added unit tests for role extraction.

Reviewed changes

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

File	Description
internal/postgres/fk_refs.go	Adds ExtractDefaultPrivilegeRoles SQL-walking parser to extract roles from desired SQL.
internal/postgres/fk_refs_test.go	Adds unit tests covering basic role extraction cases.
internal/postgres/external.go	Creates/grants stub roles before applying desired SQL to external plan DB; attempts cleanup on shutdown.

Suppressed comments (2)

internal/postgres/external.go:182

• ApplySchema() currently records all referenced roles in ed.stubRoles and Stop() drops them later, which can delete real roles. It also relies on matching the error text "already a member" and will fail when the referenced role equals the plan username (GRANT role TO role errors). Track (1) roles that were created as stubs and (2) memberships that were newly granted, so Stop() can safely revert only what this run changed.

	// Only roles that do not already exist are created and tracked for cleanup.
	candidateRoles := ExtractDefaultPrivilegeRoles(schemaAgnosticSQL)
	for _, role := range candidateRoles {
		var exists bool
		if err := conn.QueryRowContext(ctx, "SELECT EXISTS(SELECT 1 FROM pg_catalog.pg_roles WHERE rolname = $1)", role).Scan(&exists); err != nil {

internal/postgres/external.go:220

• Stop() drops roles but does not revoke any memberships it granted, and if the roles existed beforehand this can either (a) incorrectly delete real roles or (b) leave the plan user with extra memberships after the command completes. Cleanup should revoke only the memberships added by ApplySchema and drop only roles that were created as stubs.

// Errors during cleanup are logged but don't cause failures.
func (ed *ExternalDatabase) Stop() error {
	// Drop the temporary schema (best effort - don't fail if this errors)
	if ed.db != nil && ed.tempSchema != "" {

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[tianzhou]

Addressed all review findings across 3 commits since the initial submission:

Greptile/Copilot reviewed the first commit (dacf6a9) — the following commits fix the issues they flagged:

Finding	Status	Commit
P0: Stop() drops pre-existing roles	Fixed — only roles this run created are tracked/dropped	22c05f0
P0: GRANT on pre-existing role mutates cluster state	Fixed — returns actionable error if role exists but plan user isn't a member	8477a8a
P1: FOR USER synonym not handled	Fixed — parser now accepts both FOR ROLE and FOR USER	edcd530
P1: Comma-separated role lists not handled	Fixed — parser now extracts all roles from FOR ROLE r1, r2, r3	edcd530
Copilot: IN SCHEMA before FOR ROLE ordering	Not applicable — PostgreSQL grammar requires FOR ROLE before IN SCHEMA	N/A
Copilot: GRANT role TO role self-grant	Not applicable — ExtractDefaultPrivilegeRoles extracts the definer role, not the plan user; they won't be the same	N/A

Test coverage: 8 cases for ExtractDefaultPrivilegeRoles (single, quoted, deduped, no match, case insensitive, FOR USER, comma-separated, comma + FOR USER). All pass.

[Copilot]

Pull request overview

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

Suppressed comments (4)

internal/postgres/fk_refs.go:229

• RoleSpec in ALTER DEFAULT PRIVILEGES FOR ROLE/USER can be CURRENT_USER, SESSION_USER, or CURRENT_ROLE (see internal/gram.y), but this extractor will treat those keywords as literal role names (e.g. "current_user") and attempt to create/grant/drop stub roles unnecessarily (and potentially fail if the plan user lacks CREATEROLE). Consider explicitly ignoring these keyword role specs, and also rejecting dotted/qualified names since role specs aren’t schema-qualified.

				_, role, next, ok := parseQualifiedName(code, i)
				if !ok {
					i = next
					break

internal/postgres/fk_refs_test.go:216

• The tests don’t cover FOR ROLE CURRENT_USER / SESSION_USER / CURRENT_ROLE, which are valid RoleSpec values and should not result in stub role creation. Adding a test case for these keywords will prevent regressions.

			name: "case insensitive keywords",
			sql:  `alter default privileges for role myuser in schema public grant select on tables to reader;`,
			want: []string{"myuser"},
		},
		{
			name: "FOR USER synonym",
			sql:  `ALTER DEFAULT PRIVILEGES FOR USER myuser IN SCHEMA public GRANT SELECT ON TABLES TO reader;`,
			want: []string{"myuser"},
		},

internal/postgres/external.go:191

• The membership check conflates query errors with a non-member result (err != nil || !isMember), which can hide the real failure mode (e.g. permission issues running pg_has_role). Handle the query error separately and keep the actionable "not a member" message only for the !isMember case.

		if exists {
			var isMember bool
			if err := conn.QueryRowContext(ctx, "SELECT pg_has_role($1, $2, 'MEMBER')", ed.username, role).Scan(&isMember); err != nil || !isMember {
				return fmt.Errorf("role %q already exists in the plan database and the plan user %q is not a member of it; grant membership manually or use a plan user that is already a member of that role", role, ed.username)
			}

internal/postgres/external.go:230

• Cleanup currently uses DROP ROLE IF EXISTS ... without CASCADE. If the desired SQL includes ALTER DEFAULT PRIVILEGES FOR ROLE <stub> without IN SCHEMA, PostgreSQL can create default ACL entries owned by that role, and DROP ROLE may fail under RESTRICT. Using CASCADE improves best-effort cleanup and avoids accumulating stub roles in the plan DB.

			_, _ = ed.db.ExecContext(ctx, fmt.Sprintf("DROP ROLE IF EXISTS %s", quoteIdent(role)))