Skip to content

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

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

sql/privilege: clean up and increase compatibility of new acl functions#166185
rafiss wants to merge 7 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
rafiss and others added 3 commits March 19, 2026 14:34
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>
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>
@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.

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