sql: add aclitem type, implement acldefault and makeaclitem, enhance aclexplode

[rafiss]

Add a proper aclitem type (StringFamily with OID 1033, following the same pattern as name) and implement three PostgreSQL-compatible ACL functions:

• makeaclitem(grantee oid, grantor oid, privileges text, is_grantable bool) → aclitem: constructs an aclitem string from its components.
• acldefault(type text, ownerId oid) → aclitem[]: returns the hard-wired default ACL for a given object type, matching PostgreSQL's defaults for all 13 object type characters.
• aclexplode(aclitems) → setof record: previously a stub that always returned no rows. Now parses ACL strings and emits (grantor, grantee, privilege_type, is_grantable) rows. Supports both text[] and aclitem[] input.

These functions are needed for pg-compatibility with ORMs and tools that inspect PostgreSQL access control lists.

Resolves: #91070
informs: #20296

Release note (sql change): Added support for the aclitem type and the makeaclitem and acldefault built-in functions for PostgreSQL compatibility. The existing aclexplode function, which previously always returned no rows, now correctly parses ACL strings and returns the individual privilege grants they contain.

[trunk-io]

😎 Merged successfully - details.

[cockroach-teamcity]

This change is 

[fqazi]

@rafiss Great work, a couple of issues

@fqazi reviewed 4 files and all commit messages, and made 4 comments.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on rafiss and yuzefovich).

---

pkg/sql/privilege/privilege.go line 480 at r2 (raw file):

		}
		// Unterminated quote — caller will detect trailing content error.
		return i

I think the validation here is a bad and we need to return a special value. The following will pass incorrectly:

s="user=rw/"root"

---

pkg/sql/sem/builtins/generator_builtins.go line 188 at r2 (raw file):

		// Resolve role names to OIDs.
		granteeOid, err := resolveRoleOid(ctx, evalCtx, granteeStr)

This could be really slow for large arrays, can we may be do a batched resolution / have caching? I don't know the scale here, so just something to think about.

---

pkg/sql/sem/builtins/pg_builtins.go line 2519 at r2 (raw file):

				// Build the aclitem string: "grantee=privchars/grantor".
				var result strings.Builder
				result.WriteString(granteeName)

Think we need to escape the string, if for example it contains "user-1"

[fqazi]

@fqazi made 1 comment.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on rafiss and yuzefovich).

---

pkg/sql/privilege/privilege.go line 480 at r2 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

I think the validation here is a bad and we need to return a special value. The following will pass incorrectly:

s="user=rw/"root"

*we need to also return a special value for missing quotes

[fqazi]

@fqazi made 1 comment.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on rafiss and yuzefovich).

---

pkg/sql/privilege/privilege.go line 480 at r2 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

*we need to also return a special value for missing quotes

Ugh the example above should be user=rw/"root

[yuzefovich]

Nice work! I think there are a few spots where we missed the new type. Some are mentioned inline, additionaly probably strVal.ResolveAsType needs an adjustment.

@yuzefovich reviewed 18 files and all commit messages, and made 8 comments.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on fqazi and rafiss).

---

-- commits line 21 at r2:
Should we add support for array containment @> before closing the issue? (I.e. either add it in this PR, or leave a TODO and not close the issue, or file a new issue.)

---

pkg/sql/pgwire/pgwirebase/encoding.go line 979 at r2 (raw file):

		}
		return da.NewDName(tree.DString(bs)), nil
	case oidext.T_aclitem:

We also probably need to update the decoding logic in rowenc/keyside (1 spot) and rowenc/valueside (2 spots) packages.

---

pkg/sql/pgwire/pgwirebase/encoding.go line 983 at r2 (raw file):

			return nil, err
		}
		return tree.NewDACLItem(bs), nil

nit: we should extend tree.DatumAlloc for the new type.

---

pkg/sql/randgen/datum.go line 282 at r2 (raw file):

			// characters (=, /, ") that break the aclitem format.
			const alphanums = "abcdefghijklmnopqrstuvwxyz0123456789"
			granteeLen := rng.Intn(6)

nit: is 0 length ok?

---

pkg/sql/sem/eval/cast.go line 519 at r2 (raw file):

				return tree.NewDName(s), nil
			}
			if t.Oid() == oidext.T_aclitem {

We also need to add a similar block to vec_to_datum_tmpl.go. We also need to adjust toString method in cast_gen_util.go.

---

pkg/sql/sem/tree/datum.go line 5837 at r2 (raw file):

// initialized from a string in the format "grantee=privchars/grantor".
func NewDACLItem(d string) Datum {
	return wrapWithOid(NewDString(d), oidext.T_aclitem)

nit: update the last paragraph of the comment on DOidWrapper to include new type.

---

pkg/sql/logictest/testdata/logic_test/pg_builtins line 12 at r2 (raw file):

# aclexplode with an invalid ACL string produces no rows (graceful skip).
query T

nit: there is query empty 😉

[rafiss]

i looked into adding an aclitem case to ResolveAsType. that would bypass ValidateACLItemString (which is in the privilege package and can't be imported from tree). The cast path in eval/cast.go already handles validation correctly, and string literals like '...'::aclitem go through that path. So it doesn't seem that any change needed here.

@rafiss made 11 comments and resolved 4 discussions.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on fqazi and yuzefovich).

---

-- commits line 21 at r2:

Previously, yuzefovich (Yahor Yuzefovich) wrote…

Should we add support for array containment @> before closing the issue? (I.e. either add it in this PR, or leave a TODO and not close the issue, or file a new issue.)

this actually worked already since aclitem inherits DString comparison via DOidWrapper, so the generic ArrayContains logic handles it. i added logic tests covering @>

---

pkg/sql/pgwire/pgwirebase/encoding.go line 979 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

We also probably need to update the decoding logic in rowenc/keyside (1 spot) and rowenc/valueside (2 spots) packages.

Done.

---

pkg/sql/pgwire/pgwirebase/encoding.go line 983 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: we should extend tree.DatumAlloc for the new type.

Done

---

pkg/sql/privilege/privilege.go line 480 at r2 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

Ugh the example above should be user=rw/"root

Done. skipACLIdentifier now returns -1 for unterminated quotes, and both call sites in ValidateACLItemString check for it and return a proper error. Added unit tests in TestValidateACLItemString and logic tests for both grantee and grantor positions.

---

pkg/sql/randgen/datum.go line 282 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: is 0 length ok?

Done; i added 1 +

---

pkg/sql/sem/builtins/generator_builtins.go line 188 at r2 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

This could be really slow for large arrays, can we may be do a batched resolution / have caching? I don't know the scale here, so just something to think about.

Done; great point. I don't think this is function would ever be used in a high throughput situation, but it could be used in a big query. I added a roleOidCache map local to the newAclexplodeGenerator call so repeated role names across aclitem entries don't issue duplicate queries.

---

pkg/sql/sem/builtins/pg_builtins.go line 2519 at r2 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

Think we need to escape the string, if for example it contains "user-1"

Done. Added QuoteACLIdentifier in the privilege package and use it in both makeaclitem and acldefault output. Also updated aclexplode to use ExtractACLIdentifier for proper unquoting when parsing. I added tests for it.

---

pkg/sql/sem/eval/cast.go line 519 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

We also need to add a similar block to vec_to_datum_tmpl.go. We also need to adjust toString method in cast_gen_util.go.

Done.

---

pkg/sql/sem/tree/datum.go line 5837 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: update the last paragraph of the comment on DOidWrapper to include new type.

Done

---

pkg/sql/logictest/testdata/logic_test/pg_builtins line 12 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: there is query empty 😉

done; TIL

[blathers-crl]

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.}

[yuzefovich]

Ack. Execution parts LGTM, will defer to Faizan for approval.

@yuzefovich reviewed 11 files and all commit messages, made 4 comments, and resolved 1 discussion.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on fqazi and rafiss).

---

-- commits line 21 at r2:

Previously, rafiss (Rafi Shamim) wrote…

this actually worked already since aclitem inherits DString comparison via DOidWrapper, so the generic ArrayContains logic handles it. i added logic tests covering @>

Nice!

---

pkg/sql/pgwire/pgwirebase/encoding.go line 979 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

Done.

One more spot in valueside.UnmarshalLegacy.

---

pkg/sql/colexec/colexecbase/BUILD.bazel line 43 at r3 (raw file):

        "@com_github_cockroachdb_errors//:errors",
        "@com_github_cockroachdb_redact//:redact",  # keep
        "//pkg/sql/oidext",  # keep

nit: out of order.

[rafiss]

@rafiss made 2 comments and resolved 1 discussion.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on fqazi and yuzefovich).

---

pkg/sql/colexec/colexecbase/BUILD.bazel line 43 at r3 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: out of order.

done

---

pkg/sql/pgwire/pgwirebase/encoding.go line 979 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

One more spot in valueside.UnmarshalLegacy.

done

[github-actions]

AI Review: Potential Issue(s) Detected

Missing privilege character mappings for 's' (SET) and 'A' (ALTER SYSTEM) in ACLCharToPrivName and PrivNameToACLChar maps (pkg/sql/privilege/privilege.go:371-403) cause:

1. Silent data loss in aclexplode: Characters not found in ACLCharToPrivName are silently skipped (generator_builtins.go:232), so aclexplode(acldefault('p', oid)) returns zero privilege rows despite the aclitem containing valid 's' and 'A' characters.
2. makeaclitem rejects SET/ALTER SYSTEM: PrivNameToACLChar lacks the reverse mappings, so makeaclitem(..., 'SET', ...) returns an "unrecognized privilege type" error.
3. Broken round-trip: The acldefault ↔ aclexplode contract is violated for PARAMETER ('p') objects.

Fix: Add 's': "SET" and 'A': "ALTER SYSTEM" to ACLCharToPrivName, and "SET": 's' and "ALTER SYSTEM": 'A' to PrivNameToACLChar.

Inline comments have been added to the relevant lines.

View full analysis

If helpful: add O-AI-Review-Real-Issue-Found label.
If not helpful: add O-AI-Review-Not-Helpful label.

[fqazi]

@fqazi reviewed 34 files and all commit messages, made 1 comment, and resolved 2 discussions.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on rafiss and yuzefovich).

[rafiss]

/trunk merge

TFTR!