sql: add SHOW DEFAULT SESSION VARIABLES FOR ROLE command

[jasminejsun]

Epic: CRDB-26921
Issue: #117500

Release note (sql change): Previously, if a database administrator wanted to know which session variables they had set by default for a role they would need to switch into that user or scan a system table manually. This could be more time consuming or difficult to us. To address this, this patch adds a new command: SHOW VARIABLES FOR ROLE, which allows the database administrator to easily view the default values for a session variables applied to a given user.

[blathers-crl]

It looks like your PR touches SQL parser code but doesn't add or edit parser tests. Please make sure you add or edit parser tests if you edit the parser.

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

[cockroach-teamcity]

All committers have signed the CLA.

[cockroach-teamcity]

This change is 

[fqazi]

Great work, just a few minor things!

Reviewed 16 of 16 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jasminejsun)

---

pkg/sql/delegate/show_variables_for_role.go line 26 at r1 (raw file):

) (tree.Statement, error) {
	getVariablesForRoleQuery := `SELECT split_part(split_part(unnest(settings), ',', 1), '=', 1) as session_variables,
		split_part(split_part(unnest(settings), ',', 1), '=', 2) as default_values

I worry this might break if we have things with commands or equal signs within the settings themselves. i.e.

Imagine if someone did something stupid like:
'alter role demo set search_path='a='';

---

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

SHOW VARIABLES FOR ROLE ALL -- fully parenthesized
SHOW VARIABLES FOR ROLE ALL -- literals removed
SHOW VARIABLES FOR ROLE ALL -- identifiers removed

One more test here try using a parametrized query...i.e. ROLE $1

[jasminejsun]

pkg/sql/delegate/show_variables_for_role.go line 26 at r1 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

I worry this might break if we have things with commands or equal signs within the settings themselves. i.e.

Imagine if someone did something stupid like:
'alter role demo set search_path='a='';

Good point!
Is it safe to assume that the session variable name will never include an equal sign, and just split on the first equal sign in the string?

[jasminejsun]

pkg/sql/delegate/show_variables_for_role.go line 26 at r1 (raw file):

Previously, jasminejsun (Jasmine Sun) wrote…

Good point!
Is it safe to assume that the session variable name will never include an equal sign, and just split on the first equal sign in the string?

Should take care of commands where they're setting values with equal signs in them now!

[fqazi]

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jasminejsun)

---

-- commits line 4 at r2:
Add a release note describing the functionality and follow the format from the wiki

[jasminejsun]

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

Previously, fqazi (Faizan Qazi) wrote…

One more test here try using a parametrized query...i.e. ROLE $1

Non-applicables since role_spec utilizes identifiers rather than expressions! 😄

[jasminejsun]

-- commits line 4 at r2:

Previously, fqazi (Faizan Qazi) wrote…

Add a release note describing the functionality and follow the format from the wiki

Done!

[fqazi]

Just a few more minor nits, once those are sorted its ready to merge.

Reviewed 1 of 1 files at r3.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @jasminejsun)

---

-- commits line 2 at r3:
Instead of going low level lets focus on why this was needed / what it solves i.e.:

Previously, if a database administrator wanted to know which session variables they had set by default for a role they would need to switch into that user or scan a system table manually. This could be more time consuming or difficult to us. To address this, this patch adds a new command: SHOW VARIABLES FOR ROLE, which allows the database administrator to easily view the default values for a session variables applied to a given user.

---

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

)

// delegateShowVariablesForRole implements SHOW VARIABLES FOR ROLE <name> which returns all the default session values

Nit: session variables for a user.

[rafiss]

this is nice work! i have a few comments that i think we should address before merging

i saw that SHOW VARIABLES FOR ROLE was the syntax that @dikshant suggested in CRDB-26921. However, I don't think this is that intuitive -- we don't use the word "variable" like this anywhere else in our product. Can we change the command to something even more explicit, like: SHOW DEFAULT SESSION VARIABLES FOR ROLE ... ?

the rest of my comments are inline

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @fqazi and @jasminejsun)

---

pkg/sql/delegate/show_variables_for_role.go line 28 at r4 (raw file):

	getVariablesForRoleQuery := `SELECT split_part(setting, '=', 1) AS session_variables,
		substring(setting FROM position('=' in setting) + 1) AS default_values
		FROM system.database_role_settings,

implementing the query this way, with a lookup on system.database_role_settings, has a downside. it means that you need to have privileges on the system table, which is something that normally only admins have. instead, i think we should make the query look at pg_catalog.pg_db_role_setting, which does not require additional privileges.

then, at the beginning of this function, we should have the specific privilege checks we need in place. let's make the required privileges be the same as the ones needed to run ALTER ROLE ... SET; that is, running this SHOW command requires at least one of CREATEROLE, MODIFYCLUSTERSETTING, or MODIFYSQLCLUSTERSETTING. you can see that they are checked in this part of the code:

cockroach/pkg/sql/alter_role.go

Lines 301 to 316 in df6457a

	canAlterRoleSet, err := p.HasGlobalPrivilegeOrRoleOption(ctx, privilege.CREATEROLE)
	if err != nil {
	return nil, err
	}
	if !canAlterRoleSet {
	canAlterRoleSet, err = p.HasGlobalPrivilegeOrRoleOption(ctx, privilege.MODIFYCLUSTERSETTING)
	if err != nil {
	return nil, err
	}
	}
	if !canAlterRoleSet {
	canAlterRoleSet, err = p.HasGlobalPrivilegeOrRoleOption(ctx, privilege.MODIFYSQLCLUSTERSETTING)
	if err != nil {
	return nil, err
	}
	}

here is an example of how a different SHOW command does privilege checking:

cockroach/pkg/sql/delegate/show_all_cluster_settings.go

Lines 29 to 50 in df6457a

	hasView := false
	if err := d.catalog.CheckPrivilege(d.ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.MODIFYCLUSTERSETTING); err == nil {
	hasModify = true
	hasSqlModify = true
	hasView = true
	} else if pgerror.GetPGCode(err) != pgcode.InsufficientPrivilege {
	return nil, err
	}
	if !hasSqlModify {
	if err := d.catalog.CheckPrivilege(d.ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.MODIFYSQLCLUSTERSETTING); err == nil {
	hasSqlModify = true
	hasView = true
	} else if pgerror.GetPGCode(err) != pgcode.InsufficientPrivilege {
	return nil, err
	}
	}
	if !hasView {
	if err := d.catalog.CheckPrivilege(d.ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.VIEWCLUSTERSETTING); err == nil {
	hasView = true
	} else if pgerror.GetPGCode(err) != pgcode.InsufficientPrivilege {
	return nil, err
	}

finally, let's make sure that the logictests cover the privilege checking too

---

pkg/sql/logictest/testdata/logic_test/alter_role_set line 303 at r4 (raw file):

statement ok
ALTER ROLE ALL SET application_name = 'c';

can we add more tests for the following:

ALTER ROLE roach IN DATABASE defaultdb SET ...

ALTER ROLE ALL IN DATABASE defaultdb SET ...

as you can see, some of the defaults are per-database. so i think it would be important for the SHOW output to indicate if a given default is applicable to all databases, or a specific database. i think maybe there should be a third column, named something like database_name, and if that's NULL it means the default applies to all databases

[dikshant]

+1 on SHOW DEFAULT SESSION VARIABLES FOR ROLE ...

[rafiss]

this looks great! really nice work coming up with the delegate SQL query

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @fqazi and @jasminejsun)

[fqazi]

Reviewed 16 of 18 files at r5, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @jasminejsun)

---

-- commits line 11 at r5:
This should be the body of the commit message. Release note can simply just say you added SHOW VARIABLES FOR ROLE with a brief description of the feature.

[jasminejsun]

bors r+

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)