Skip to content

Commit

Permalink
sql: avoid checking if a role exists when setting to the current role
Browse files Browse the repository at this point in the history
This is an uncached round-trip. It makes `DISCARD` expensive.
In cockroachdb#86485 we implemented
`SET SESSION AUTHORIZATION DEFAULT`, this added an extra sql query to
`DISCARD ALL` to check if the role we'd become exists. This is almost
certainly unintentional in the case where the role we'd become is the
current session role. I think this just happened because of code
consolidation.

We now no longer check if the current session role exists. This removes
the last round-trip from DISCARD ALL.

Release note (performance improvement): In 22.2, logic was added to make
`SET SESSION AUTHORIZATION DEFAULT` not a no-op. This implementation used
more general code for setting the role for a session which made sure that
the role exists. The check for whether a role exists is currently uncached.
We don't need to check if the role we already are exists. This improves the
performance of `DISCARD ALL` in addition to `SET SESSION AUTHORIZATION
DEFAULT`.
  • Loading branch information
ajwerner committed Jan 25, 2023
1 parent 73befd9 commit 74487fc
Show file tree
Hide file tree
Showing 2 changed files with 4 additions and 4 deletions.
6 changes: 3 additions & 3 deletions pkg/bench/rttanalysis/testdata/benchmark_expectations
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,9 @@ exp,benchmark
15,CreateRole/create_role_with_2_options
18,CreateRole/create_role_with_3_options
13,CreateRole/create_role_with_no_options
18,"Discard/DISCARD_ALL,_1_tables_in_1_db"
21,"Discard/DISCARD_ALL,_2_tables_in_2_dbs"
1,"Discard/DISCARD_ALL,_no_tables"
12,"Discard/DISCARD_ALL,_1_tables_in_1_db"
15,"Discard/DISCARD_ALL,_2_tables_in_2_dbs"
0,"Discard/DISCARD_ALL,_no_tables"
10,DropDatabase/drop_database_0_tables
11,DropDatabase/drop_database_1_table
11,DropDatabase/drop_database_2_tables
Expand Down
2 changes: 1 addition & 1 deletion pkg/sql/user.go
Original file line number Diff line number Diff line change
Expand Up @@ -546,7 +546,7 @@ func (p *planner) setRole(ctx context.Context, local bool, s username.SQLUsernam
sessionUser := p.SessionData().SessionUser()
becomeUser := sessionUser
// Check the role exists - if so, populate becomeUser.
if !s.IsNoneRole() {
if !s.IsNoneRole() && s != sessionUser {
becomeUser = s

exists, err := p.RoleExists(ctx, becomeUser)
Expand Down

0 comments on commit 74487fc

Please sign in to comment.