Merge #58671

58671: sql: fix performance regression in user authn r=otan a=rafiss

The authn code needs to query system.users and system.role_options.
These queries are run by the internal executor, which has a current DB
of "". This causes the name to be resolved as "".system.users and
"".system.role_options. The lookup for the "" DB always fails, but that
result is not cached, so the lookup occurs on every authn attempt. There
is fallback logic that then looks up the correct name.

Now we specify the fully-qualified 3-part name for these two queries.

This is a low-risk change that can be backported. A more robust fix that
will prevent this class of mistaken lookups will follow later, but probably
can't be backported.

Release note (bug fix): The user authentication flow no longer performs
extraneous name lookups. This performance regression was present since
v20.2.

Co-authored-by: Rafi Shamim <[email protected]>

pkg/bench/ddl_analysis/BUILD.bazel

@@ -26,6 +26,7 @@ go_test(
         "drop_bench_test.go",
         "grant_revoke_bench_test.go",
         "grant_revoke_role_bench_test.go",
+        "system_bench_test.go",
         "truncate_bench_test.go",
         "validate_benchmark_data_test.go",
         "virtual_table_bench_test.go",

pkg/bench/ddl_analysis/ddl_analysis_bench.go

@@ -100,14 +100,6 @@ func RunRoundTripBenchmark(b *testing.B, tests []RoundTripBenchTestCase) {
 // counting each "txn coordinator send" operation.
 func countKvBatchRequestsInRecording(r tracing.Recording) int {
 	root := r[0]
-
-	// Find the topmost "flow" span to start traversing from.
-	for _, sp := range r {
-		if sp.ParentSpanID == root.SpanID && sp.Operation == "flow" {
-			return countKvBatchRequestsInSpan(r, sp)
-		}
-	}
-
 	return countKvBatchRequestsInSpan(r, root)
 }
 

pkg/bench/ddl_analysis/system_bench_test.go

@@ -0,0 +1,41 @@
+// Copyright 2021 The Cockroach Authors.
+//
+// Use of this software is governed by the Business Source License
+// included in the file licenses/BSL.txt.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+package bench
+
+import "testing"
+
+func BenchmarkSystemDatabaseQueries(b *testing.B) {
+	tests := []RoundTripBenchTestCase{
+		// This query performs 1 extra lookup since the executor first tries to
+		// lookup the name `current_db.system.users`.
+		{
+			name: "select system.users without schema name",
+			stmt: `SELECT username, "hashedPassword" FROM system.users WHERE username = 'root'`,
+		},
+		// This query performs 4 extra lookup since the executor tries to
+		// lookup the name `"".system.users`. Since the "" database doesn't exist,
+		// it also falls back to looking up that database name in the deprecated
+		// namespace table.
+		{
+			name:  "select system.users with empty database name",
+			setup: `SET sql_safe_updates = false; USE "";`,
+			stmt:  `SELECT username, "hashedPassword"  FROM system.users WHERE username = 'root'`,
+		},
+		// This query performs 2 lookups: getting the descriptor ID by name, then
+		// fetching the system table descriptor.
+		{
+			name: "select system.users with schema name",
+			stmt: `SELECT username, "hashedPassword" FROM system.public.users WHERE username = 'root'`,
+		},
+	}
+
+	RunRoundTripBenchmark(b, tests)
+}

pkg/bench/ddl_analysis/testdata/benchmark_expectations

@@ -65,3 +65,6 @@ min	max	benchmark
 46	46	Truncate/truncate_2_column_2_rows
 8	8	VirtualTableQueries/select_crdb_internal.invalid_objects_with_1_fk
 1	1	VirtualTableQueries/select_crdb_internal.tables_with_1_fk
+3	3	SystemDatabaseQueries/select_system.users_without_schema_name
+6	6	SystemDatabaseQueries/select_system.users_with_empty_database_name
+2	2	SystemDatabaseQueries/select_system.users_with_schema_name

pkg/sql/user.go

@@ -111,7 +111,8 @@ func retrieveUserAndPassword(
 
 	// Perform the lookup with a timeout.
 	err = runFn(func(ctx context.Context) error {
-		const getHashedPassword = `SELECT "hashedPassword" FROM system.users ` +
+		// Use fully qualified table name to avoid looking up "".system.users.
+		const getHashedPassword = `SELECT "hashedPassword" FROM system.public.users ` +
 			`WHERE username=$1`
 		values, err := ie.QueryRowEx(
 			ctx, "get-hashed-pwd", nil, /* txn */
@@ -131,7 +132,8 @@ func retrieveUserAndPassword(
 			return nil
 		}
 
-		getLoginDependencies := `SELECT option, value FROM system.role_options ` +
+		// Use fully qualified table name to avoid looking up "".system.role_options.
+		getLoginDependencies := `SELECT option, value FROM system.public.role_options ` +
 			`WHERE username=$1 AND option IN ('NOLOGIN', 'VALID UNTIL')`
 
 		loginDependencies, err := ie.QueryEx(