sql: extend ShowRoles AST with Options and Limit fields

[souravcrl]

Summary

• Extend the ShowRoles AST struct with Options *ShowUsersOptions and Limit *Limit fields, matching ShowUsers
• Add Format logic for the new fields so SHOW ROLES WITH ... round-trips correctly through the formatter

Since SHOW ROLES and SHOW USERS are interchangeable, the ShowRoles node should support the same provisioning filter clauses. This is the first of three parts to extend SHOW ROLES with the same options that SHOW USERS supports.

Part 1 of 3 — AST only; grammar and delegation follow in subsequent PRs.

Informs: CRDB-62959
Epic: none

🤖 Generated with Claude Code

[trunk-io]

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.

After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here

[cockroach-teamcity]

This change is 

[spilchen]

@spilchen reviewed 1 file and made 3 comments.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on souravcrl).

---

pkg/sql/delegate/delegate.go line 156 at r3 (raw file):

	case *tree.ShowUsers:
		return d.delegateShowRolesExtended(t)

there is a stale TODO comment in show_roles.go that I think can be removed now?

// TODO(sourav): Wire this into the delegate dispatch for ShowUsers;
// currently only exercised by unit tests.
func (d *delegator) delegateShowRolesExtended(n *tree.ShowUsers) (tree.Statement, error) {

---

pkg/sql/sem/tree/show.go line 1117 at r3 (raw file):

func (node *ShowRoles) Format(ctx *FmtCtx) {
	ctx.WriteString("SHOW ROLES")
	if node.Options != nil && !node.Options.IsDefault() {

do we need to extend the SHOW ROLES grammar to accept these new options? I didn't see that change in sql.y

I see this in the PR "Part 1 of 3 — AST only; grammar and delegation follow in subsequent PRs.". So, maybe this part was intentionally deferred. But I do see some grammar changes in here, so that part isn't 100% accurate. So, I think this comment still stands. The code in this Format function is untested and can generate SQL that's not parsable.

---

pkg/sql/parser/testdata/show line 653 at r1 (raw file):

parse
SHOW USERS WITH SOURCE = 'ldap:ldap.example.com'

do we want to test the SHOW ROLES variant in here too?

[spilchen]

I think some of my comments are addressed in part 2 and 3 PRs. I find the PR split very confusing. When I review a PR I expect it to be self contained. But things are leaking out and being tested in a follow-on PRs. My advice is to combine all 3 PRs in 1. I will review the whole thing.

[souravcrl]

I appreciate the feedback — and I completely understand the preference for self-contained PRs but I'm new to sql part of code and splitting the work into smaller, incremental PRs has been helping me build familiarity and work on smaller changes which helps me in building my confidence as I make careful and well thought out progress. In larger PRs actually I get overwhelmed with the amount of changes and I sometimes loose focus, so it is difficult for me.

@souravcrl made 4 comments.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on spilchen).

---

pkg/sql/delegate/delegate.go line 156 at r3 (raw file):

Previously, spilchen wrote…

there is a stale TODO comment in show_roles.go that I think can be removed now?

// TODO(sourav): Wire this into the delegate dispatch for ShowUsers;
// currently only exercised by unit tests.
func (d *delegator) delegateShowRolesExtended(n *tree.ShowUsers) (tree.Statement, error) {

done

---

pkg/sql/sem/tree/show.go line 1117 at r3 (raw file):

Previously, spilchen wrote…

do we need to extend the SHOW ROLES grammar to accept these new options? I didn't see that change in sql.y

I see this in the PR "Part 1 of 3 — AST only; grammar and delegation follow in subsequent PRs.". So, maybe this part was intentionally deferred. But I do see some grammar changes in here, so that part isn't 100% accurate. So, I think this comment still stands. The code in this Format function is untested and can generate SQL that's not parsable.

Good point. I've added unit tests for the ShowRoles.Format implementation in this PR — see TestShowRolesFormat in
https://github.com/cockroachdb/cockroach/pull/168022/files/56867b361973d449a1c8b4c83e03960701bb4ac4#diff-9559ecbf9ce12fd06476e211456b68c89af1ea58accdc37f540318504ee08d50. The tests cover all
combinations: no options, source only, last login before only, both options, with limit, and nil options. The grammar changes that make the formatted output actually parsable are in #168024, which
extends show_roles_stmt in sql.y to accept opt_with_show_users_options opt_limit_clause. Until that grammar PR lands, the Format output is indeed not round-trippable through the parser — but the AST and formatting are correct and fully tested at the unit level here.

---

pkg/sql/parser/testdata/show line 653 at r1 (raw file):

Previously, spilchen wrote…

do we want to test the SHOW ROLES variant in here too?

Yes — parser tests for the SHOW ROLES extended syntax are added in #168024, which is where the grammar is wired. That PR adds parse tests for SHOW ROLES WITH SOURCE = ..., SHOW ROLES WITH
LAST LOGIN BEFORE ..., combined options, SHOW ROLES LIMIT ..., and duplicate option error cases for both SHOW ROLES and SHOW USERS. It wouldn't be possible to add those parse tests here since the grammar doesn't accept the new clauses yet at this point in the stack.

[spilchen]

I'm still confused by the split of this PR. The PR does 3 things:

1. grammar
2. delegation
3. AST extension

But only completes the first two for SHOW USERS. The third commit extends ShowRoles at the AST level but stops there, leaving the grammar and delegation untouched. I feel like the third commit should not have been included unless all 3 things were included. I think the PR would be cleaner if the PR was just the SHOW USERS change (commits 1 and 2), with ShowRoles extension done in a separate PR that wires everything up together.

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

---

pkg/sql/sem/tree/show_users_test.go line 93 at r6 (raw file):

}

func TestShowRolesFormat(t *testing.T) {

this does test the format code, but it's not the normal way we do it in the parser. It should be done through pkg/sql/parser/testdata, which requires full end-to-end flow.

[souravcrl]

I think there is some confusion here. The grammar and delegation are unrelated to the PR, they are part of a different PR #166416. This is a stacked PR, that's why those commits are showing up. The commit to review here is based on 56867b3.

@souravcrl made 2 comments.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on spilchen).

---

pkg/sql/sem/tree/show_users_test.go line 93 at r6 (raw file):

Previously, spilchen wrote…

this does test the format code, but it's not the normal way we do it in the parser. It should be done through pkg/sql/parser/testdata, which requires full end-to-end flow.

I was following other tests in the package like the pkg/sql/sem/tree/schema_helpers_test.go test. I have added parser tests also but they are integrated with the parser code logic.

[spilchen]

Ah ok. Yes, I am very confused with this review. The PR description says this is part 1 of 3. So, I thought this was the first PR to review. Also, there isn't any links to the other PRs. So, I need some guidance on what you would like me to review in what order. Perhaps get Claude to sort through them and come up with a concise review order summary.

[souravcrl]

I have updated the issue reference. It is targeting show roles as against show users which #166416 is doing. Both of them are related as the delegate logic is common and hence these 3 are based on the former. The 3 PRs are numbered in the order only -168022 followed by 23 and 24.

@souravcrl made 1 comment.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on spilchen).

[spilchen]

The dependency between the various PRs should be clear in the PR description. It shouldn't be buried as a comment. I asked Claude to sort this out and I pasted it below. Is it accurate? If so, I may just review the 4th PR (#168024) as it has everything in it? Part of the problem I had with reviewing is I would make a comment only to be told its fixed in another PRs. So, they aren't truly standalone PRs and should probably go in as one anyway.

PR Review Order (dependency chain)

All 4 PRs are by @souravcrl, adding provisioning filter clauses (WITH SOURCE = ..., LAST LOGIN BEFORE ..., LIMIT) to SHOW USERS and SHOW ROLES.

1. #166416 — sql: add parser grammar and wire up for SHOW USERS clauses

• Status: REVIEW_REQUIRED
• Branch: show-users-enhance → master
• Commits to review: 2
  1. Parser grammar for SHOW USERS provisioning clauses in sql.y
  2. Delegation wiring + e2e logic tests
• Summary: The foundation — complete end-to-end implementation for SHOW USERS extended syntax. References issue provisioning, sql: support provisioning clauses in show users view #150604.

2. #168022 — sql: extend ShowRoles AST with Options and Limit fields (Part 1/3)

• Status: CHANGES_REQUESTED (by @spilchen)
• Commits to review: 1 new commit (on top of sql: add parser grammar and wire up for SHOW USERS clauses #166416's 2)
  1. Adds Options and Limit fields to ShowRoles AST struct + Format logic
• Summary: AST-only extension. No grammar or delegation for SHOW ROLES yet. @spilchen's concerns:
  Format produces unparsable SQL, unit test doesn't follow standard parser testdata pattern, stale TODO.

3. #168023 — sql: wire SHOW ROLES extended options into delegate dispatch (Part 2/3)

• Status: REVIEW_REQUIRED
• Commits to review: 1 new commit (on top of sql: extend ShowRoles AST with Options and Limit fields #168022's 3)
  1. Refactors delegateShowRolesExtended to accept (options, limit) instead of *tree.ShowUsers,
     serving both SHOW USERS and SHOW ROLES. Removes stale TODO.
• Summary: Delegation/execution wiring. After this, the delegate layer can handle extended SHOW ROLES, but the parser still can't parse the extended syntax.

4. #168024 — sql: add parser grammar and e2e tests for SHOW ROLES provisioning clauses (Part 3/3)

• Status: REVIEW_REQUIRED
• Commits to review: 1 new commit (on top of sql: wire SHOW ROLES extended options into delegate dispatch #168023's 4)
  1. Extends sql.y grammar for SHOW ROLES [WITH <options>] [LIMIT <n>] + parser tests + e2e logic
     tests + BNF docs
• Summary: Completes the feature — grammar, parser round-trip tests, and e2e tests. This is where
  Format output becomes parsable.

[cockroach-teamcity]

⚪ Sysbench [SQL, 3node, oltp_read_write]

Metric	Old Commit	New Commit	Delta	Note
⚪ sec/op	10.60m ±1%	10.44m ±2%	~	p=0.061 n=15
⚪ allocs/op	8.152k ±1%	8.169k ±2%	~	p=0.044 n=15

Reproduce

benchdiff binaries:

mkdir -p benchdiff/7345331/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/7345331b92de3252f74732afe155816d930940e0/bin/pkg_sql_tests benchdiff/7345331/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/7345331/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/a41e86d/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/a41e86dbb2bd18447a4ccf69d6c5f887827cd7d7/bin/pkg_sql_tests benchdiff/a41e86d/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/a41e86d/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests

benchdiff command:

# NB: for best (most stable) results, also add a suitable `--benchtime` that
# results in ~1s to ~5s of benchmark runs. For example, if ops average ~3ms, a
# benchtime of `1000x` is appropriate.
#
# Some benchmarks (in particular BenchmarkSysbench) output additional memory
# profiles covering only the execution (excluding the setup/teardown) - those
# should be preferred for analysis since they more closely correspond to what's
# reported as B/op and alloc/op.
benchdiff --run=^BenchmarkSysbench/SQL/3node/oltp_read_write$ --old=a41e86d --new=7345331 --memprofile ./pkg/sql/tests

⚪ Sysbench [KV, 3node, oltp_read_only]

Metric	Old Commit	New Commit	Delta	Note
⚪ sec/op	3.209m ±1%	3.250m ±1%	~	p=0.061 n=15
⚪ allocs/op	2.101k ±0%	2.101k ±0%	~	p=0.323 n=15

Reproduce

benchdiff binaries:

mkdir -p benchdiff/7345331/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/7345331b92de3252f74732afe155816d930940e0/bin/pkg_sql_tests benchdiff/7345331/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/7345331/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/a41e86d/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/a41e86dbb2bd18447a4ccf69d6c5f887827cd7d7/bin/pkg_sql_tests benchdiff/a41e86d/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/a41e86d/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests

benchdiff command:

# NB: for best (most stable) results, also add a suitable `--benchtime` that
# results in ~1s to ~5s of benchmark runs. For example, if ops average ~3ms, a
# benchtime of `1000x` is appropriate.
#
# Some benchmarks (in particular BenchmarkSysbench) output additional memory
# profiles covering only the execution (excluding the setup/teardown) - those
# should be preferred for analysis since they more closely correspond to what's
# reported as B/op and alloc/op.
benchdiff --run=^BenchmarkSysbench/KV/3node/oltp_read_only$ --old=a41e86d --new=7345331 --memprofile ./pkg/sql/tests

🔴 Sysbench [KV, 3node, oltp_write_only]

Metric	Old Commit	New Commit	Delta	Note
🔴 sec/op	3.116m ±2%	3.176m ±1%	+1.94%	p=0.003 n=15
⚪ allocs/op	4.221k ±0%	4.221k ±0%	~	p=0.766 n=15

Reproduce

benchdiff binaries:

mkdir -p benchdiff/7345331/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/7345331b92de3252f74732afe155816d930940e0/bin/pkg_sql_tests benchdiff/7345331/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/7345331/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/a41e86d/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/a41e86dbb2bd18447a4ccf69d6c5f887827cd7d7/bin/pkg_sql_tests benchdiff/a41e86d/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/a41e86d/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests

benchdiff command:

# NB: for best (most stable) results, also add a suitable `--benchtime` that
# results in ~1s to ~5s of benchmark runs. For example, if ops average ~3ms, a
# benchtime of `1000x` is appropriate.
#
# Some benchmarks (in particular BenchmarkSysbench) output additional memory
# profiles covering only the execution (excluding the setup/teardown) - those
# should be preferred for analysis since they more closely correspond to what's
# reported as B/op and alloc/op.
benchdiff --run=^BenchmarkSysbench/KV/3node/oltp_write_only$ --old=a41e86d --new=7345331 --memprofile ./pkg/sql/tests

Artifacts

download:

mkdir -p new
gcloud storage cp gs://cockroach-microbench-ci/artifacts/7345331b92de3252f74732afe155816d930940e0/24661122328-1/\* new/
mkdir -p old
gcloud storage cp gs://cockroach-microbench-ci/artifacts/a41e86dbb2bd18447a4ccf69d6c5f887827cd7d7/24661122328-1/\* old/

built with commit: 7345331b92de3252f74732afe155816d930940e0