Skip to content

sql/privilege: clean up and increase compatibility of new acl functions#166185

Open
rafiss wants to merge 8 commits intocockroachdb:masterfrom
rafiss:aclitem-cleanup
Open

sql/privilege: clean up and increase compatibility of new acl functions#166185
rafiss wants to merge 8 commits intocockroachdb:masterfrom
rafiss:aclitem-cleanup

Conversation

@rafiss
Copy link
Collaborator

@rafiss rafiss commented Mar 19, 2026

Review note: Please review each commit individually to make this easy to review.
See the Claude Code plan here.


sql/privilege: sort ACL characters by privilege bit position

Replace the hardcoded orderedPrivs list with a sort by Kind value,
which corresponds to the privilege bit position. This produces a
deterministic ACL character ordering and also fixes a pre-existing
bug where make([]string, len(privileges)) created zero-value
entries before appending.

sql/privilege: remove redundant ValidACLChars constant

Use ACLCharToPrivName map lookup instead of maintaining a duplicate
list of valid ACL characters.

sql: update acldefault to reflect actual default privileges

Update the acldefault builtin to return privilege defaults that match
what CockroachDB actually assigns to newly created objects:

  • Include admin and root roles with ALL privileges, matching
    NewCustomSuperuserPrivilegeDescriptor behavior.
  • If the owner is distinct from admin/root, include an owner entry too.
  • Adjust privilege chars per object type to match CRDB's defaults
    (e.g. tables get "admrtw" not "arwdDxtm").
  • Add parameter ('p') defaults with SET and ALTER SYSTEM privileges.
  • Do not mark implicit grant options with '*' in the ACL string.
    PostgreSQL documents that "the owner's implicit grant options are not
    marked in the access privileges display", so we follow suit for
    admin, root, and the owner.

Add TestAclDefaultPublicPrivileges to verify that the public role's
privileges from acldefault match the actual defaults on newly created
objects.

sql: populate ACL columns in pg_catalog tables

Populate the relacl, nspacl, datacl, proacl, and typacl columns in
pg_catalog tables using a new privilegeDescriptorToACLArray helper.

Only privileges with a PostgreSQL ACL character equivalent are included;
CockroachDB-specific privileges (BACKUP, CHANGEFEED, ZONECONFIG, etc.)
are skipped.

To match PostgreSQL behavior:

  • Return NULL when an object's privileges match its defaults, since
    pg_dump compares ACL columns against acldefault() to decide whether
    to emit GRANT/REVOKE statements.
  • Strip grant option '*' markers for admin, root, and the owner,
    since their grant options are implicit (as in PostgreSQL for the
    owner/superuser).
  • Update acldefault() to use a shared DefaultACLItems function,
    ensuring consistency between ACL column output and acldefault().

Virtual tables and schemas continue to return NULL ACLs.

sql/privilege: add ACLItem type for structured aclitem construction

Add a new ACLItem struct that encapsulates PostgreSQL-compatible aclitem
construction (grantee=privchars/grantor format). This replaces ad-hoc
fmt.Sprintf-based ACL string building with a structured type that:

  • Implements redact.SafeFormatter (role names are PII, privilege chars
    are safe)
  • Provides NewACLItem constructor and ParseACLItem parser with
    round-trip consistency
  • Moves DefaultACLItems to return []ACLItem instead of []string
  • Adds IsDefaultACL for order-independent default privilege comparison
  • Removes ValidateACLItemString (superseded by ParseACLItem) and the
    now-unused skipACLIdentifier helper

sql: migrate aclitem callers to use ACLItem type

Update all callers that previously constructed aclitem strings via
fmt.Sprintf to use the structured ACLItem type:

  • NewDACLItemFromDString: use ParseACLItem for validation
  • acldefault builtin: use DefaultACLItems ([]ACLItem) for
    descriptor-backed types; use NewACLItem for non-descriptor types
    (except PARAMETER which needs raw ACL chars for SET/ALTER SYSTEM)
  • privilegeDescriptorToACLArray: use NewACLItem with ALL expansion
    and IsDefaultACL for default detection
  • createDefACLItem: accept username.SQLUsername, use NewACLItem

The aclexplode builtin retains manual char iteration because ACL chars
's' (SET) and 'A' (ALTER SYSTEM) have no privilege.Kind equivalent.

sql: accept privilege.ACLItem in tree.NewDACLItem

Change tree.NewDACLItem to accept a privilege.ACLItem instead of a
string. This eliminates the string→parse→validate round-trip at each
call site where a structured ACLItem is already available.

Callers that still need to construct an aclitem datum from a raw string
(pgwire decoding, casts, random datum generation, string literal
resolution) now use tree.NewDACLItemFromDString.

Also change createDefACLItem in pg_catalog.go to return
privilege.ACLItem instead of string, since all callers immediately
pass it to tree.NewDACLItem.


informs #20296

rafiss and others added 3 commits March 19, 2026 05:32
Replace the hardcoded orderedPrivs list with a sort by Kind value,
which corresponds to the privilege bit position. This produces a
deterministic ACL character ordering and also fixes a pre-existing
bug where `make([]string, len(privileges))` created zero-value
entries before appending.

Epic: none

Release note: None

Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
Use ACLCharToPrivName map lookup instead of maintaining a duplicate
list of valid ACL characters.

Epic: none

Release note: None

Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
Update the acldefault builtin to return privilege defaults that match
what CockroachDB actually assigns to newly created objects:

- Include admin and root roles with ALL privileges, matching
  NewCustomSuperuserPrivilegeDescriptor behavior.
- If the owner is distinct from admin/root, include an owner entry too.
- Adjust privilege chars per object type to match CRDB's defaults
  (e.g. tables get "admrtw" not "arwdDxtm").
- Add parameter ('p') defaults with SET and ALTER SYSTEM privileges.
- Do not mark implicit grant options with '*' in the ACL string.
  PostgreSQL documents that "the owner's implicit grant options are not
  marked in the access privileges display", so we follow suit for
  admin, root, and the owner.

Add TestAclDefaultPublicPrivileges to verify that the public role's
privileges from acldefault match the actual defaults on newly created
objects.

Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
@trunk-io
Copy link
Contributor

trunk-io bot commented Mar 19, 2026

Merging to master in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

Populate the relacl, nspacl, datacl, proacl, and typacl columns in
pg_catalog tables using a new privilegeDescriptorToACLArray helper.

Only privileges with a PostgreSQL ACL character equivalent are included;
CockroachDB-specific privileges (BACKUP, CHANGEFEED, ZONECONFIG, etc.)
are skipped.

To match PostgreSQL behavior:

- Return NULL when an object's privileges match its defaults, since
  pg_dump compares ACL columns against acldefault() to decide whether
  to emit GRANT/REVOKE statements.
- Strip grant option '*' markers for admin, root, and the owner,
  since their grant options are implicit (as in PostgreSQL for the
  owner/superuser).
- Update acldefault() to use a shared DefaultACLItems function,
  ensuring consistency between ACL column output and acldefault().

Virtual tables and schemas continue to return NULL ACLs.

Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
@rafiss rafiss marked this pull request as ready for review March 19, 2026 18:31
@rafiss rafiss requested review from a team as code owners March 19, 2026 18:31
@rafiss rafiss requested a review from fqazi March 19, 2026 18:31
Add a new ACLItem struct that encapsulates PostgreSQL-compatible aclitem
construction (grantee=privchars/grantor format). This replaces ad-hoc
fmt.Sprintf-based ACL string building with a structured type that:

- Implements redact.SafeFormatter (role names are PII, privilege chars
  are safe)
- Provides NewACLItem constructor and ParseACLItem parser with
  round-trip consistency
- Moves DefaultACLItems to return []ACLItem instead of []string
- Adds IsDefaultACL for order-independent default privilege comparison
- Removes ValidateACLItemString (superseded by ParseACLItem) and the
  now-unused skipACLIdentifier helper

Release note: None

Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
@rafiss rafiss requested a review from a team as a code owner March 19, 2026 19:45
@rafiss rafiss requested review from ZhouXing19 and removed request for a team March 19, 2026 19:45
@blathers-crl
Copy link

blathers-crl bot commented Mar 19, 2026

Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@github-actions

This comment was marked as resolved.

@github-actions github-actions bot added the o-AI-Review-Potential-Issue-Detected AI reviewer found potential issue. Never assign manually—auto-applied by GH action only. label Mar 19, 2026
@rafiss rafiss added the O-AI-Review-Real-Issue-Found AI reviewer found real issue label Mar 19, 2026
rafiss and others added 3 commits March 19, 2026 19:35
Update all callers that previously constructed aclitem strings via
fmt.Sprintf to use the structured ACLItem type:

- NewDACLItemFromDString: use ParseACLItem for validation
- acldefault builtin: use DefaultACLItems ([]ACLItem) for
  descriptor-backed types; use NewACLItem for non-descriptor types
  (except PARAMETER which needs raw ACL chars for SET/ALTER SYSTEM)
- privilegeDescriptorToACLArray: use NewACLItem with ALL expansion
  and IsDefaultACL for default detection
- createDefACLItem: accept username.SQLUsername, use NewACLItem

The aclexplode builtin retains manual char iteration because ACL chars
's' (SET) and 'A' (ALTER SYSTEM) have no privilege.Kind equivalent.

Release note: None

Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
Change `tree.NewDACLItem` to accept a `privilege.ACLItem` instead of a
string. This eliminates the string→parse→validate round-trip at each
call site where a structured ACLItem is already available.

Callers that still need to construct an aclitem datum from a raw string
(pgwire decoding, casts, random datum generation, string literal
resolution) now use `tree.NewDACLItemFromDString`.

Also change `createDefACLItem` in pg_catalog.go to return
`privilege.ACLItem` instead of `string`, since all callers immediately
pass it to `tree.NewDACLItem`.

Release note: None

Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
When the owner of a database object is a non-root, non-admin user,
their privileges are not explicitly stored in the privilege descriptor
(they are implicit). This caused privilegeDescriptorToACLArray to omit
the owner's entry from the ACL array, leading to two problems:

1. The owner's privilege entry was missing from relacl/nspacl output,
   unlike PostgreSQL which always includes the owner.
2. IsDefaultACL always returned false for non-root-owned objects
   (length mismatch: 2 actual vs 3 expected), so default privileges
   returned a non-NULL ACL array instead of NULL.

Fix this by checking whether the owner appears in privDesc.Users after
the main loop, and if not, adding an entry with the owner's implicit
ALL privileges.

Release note: None

Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
Copy link
Contributor

@eric-alton eric-alton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eric-alton reviewed 21 files and all commit messages, and made 6 comments.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on fqazi, rafiss, and ZhouXing19).


-- commits line 2 at r1:
Outside of the bug fix, the motivation behind this change is unclear. What benefits do we gain from having deterministic ACL ordering?


pkg/sql/privilege/kind.go line 82 at r9 (raw file):

	// by acldefault, aclexplode, and makeaclitem. They have no
	// corresponding CockroachDB privilege and must never be used in
	// bitmask operations (Mask/ToBitField/ListFromBitField).

Can we guard against this in any way? For example, maybe one common code path which performs bitmask operations on these Kind values can check that the value does not exceed largestKind.


pkg/sql/sem/builtins/pg_builtins.go line 2630 at r3 (raw file):

				// (matching PostgreSQL behavior for owner defaults).
				if def.ownerPrivs != "" {
					item := quotedAdmin + "=" + def.ownerPrivs + "/" + quotedOwner

What is the reason for this ordering, admin before public? (admin, public, root, owner)


pkg/sql/pg_catalog.go line 1748 at r4 (raw file):

// Only privileges that have a PostgreSQL ACL character equivalent are
// included; CockroachDB-specific privileges (BACKUP, CHANGEFEED, etc.)
// are silently skipped by ListToACL.

If our goal is to have pg_dump output CockroachDB-compatible DCL, should these CDB privileges be skipped?


pkg/sql/pg_catalog.go line 1795 at r4 (raw file):

			return nil, err
		}
		aclItems = append(aclItems, fmt.Sprintf("%s=%s/%s", grantee, acl, quotedOwner))

Is the owner being treated as the grantor here? Is that correct, or does it matter?


pkg/sql/logictest/testdata/logic_test/pg_catalog line 5681 at r4 (raw file):


# Verify that ACL columns return NULL when privileges match defaults
# (matching PostgreSQL behavior), and non-NULL after modification.

Not all your acl types follow this test pattern. nspacl, datacl, and typacl are all missing a test for when a we modify the default privileges to go from expected NULL to non-NULL output (and back again if the change is reverted).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

o-AI-Review-Potential-Issue-Detected AI reviewer found potential issue. Never assign manually—auto-applied by GH action only. O-AI-Review-Real-Issue-Found AI reviewer found real issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants