From d6bd72ba236b3d7337b8599d595de0bc11643596 Mon Sep 17 00:00:00 2001
From: Rafi Shamim <rafi@cockroachlabs.com>
Date: Fri, 8 Jan 2021 15:00:28 -0500
Subject: [PATCH] sql: fix performance regression in user authn

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.

To show that this fix is important, new benchmarks are added to the
bench/ddl_analysis tests.

Release note (bug fix): The user authentication flow no longer performs
extraneous name lookups. This performance regression was present since
v20.2.
---
 pkg/bench/ddl_analysis/ddl_analysis_bench.go |  8 ----
 pkg/bench/ddl_analysis/system_bench_test.go  | 41 ++++++++++++++++++++
 pkg/sql/user.go                              |  6 ++-
 3 files changed, 45 insertions(+), 10 deletions(-)
 create mode 100644 pkg/bench/ddl_analysis/system_bench_test.go

diff --git a/pkg/bench/ddl_analysis/ddl_analysis_bench.go b/pkg/bench/ddl_analysis/ddl_analysis_bench.go
index 9cc5a257d224..88bb311f6586 100644
--- a/pkg/bench/ddl_analysis/ddl_analysis_bench.go
+++ b/pkg/bench/ddl_analysis/ddl_analysis_bench.go
@@ -102,14 +102,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)
 }
 
diff --git a/pkg/bench/ddl_analysis/system_bench_test.go b/pkg/bench/ddl_analysis/system_bench_test.go
new file mode 100644
index 000000000000..45c1d9e472c4
--- /dev/null
+++ b/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)
+}
diff --git a/pkg/sql/user.go b/pkg/sql/user.go
index f96c24791ea2..7718d10e5a66 100644
--- a/pkg/sql/user.go
+++ b/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(