Merge #58677

58677: sql/catalog: allow caching of system.users and role_options r=ajwerner a=rafiss These tables are used during authentication, so are in a hot path. Caching them will avoid KV roundtrips. Smaller scope of #35804 Release note: None Co-authored-by: Rafi Shamim <[email protected]>
cockroachdb · Jan 14, 2021 · eba887a · eba887a
2 parents b22e903 + 148d58f
commit eba887a
Show file tree

Hide file tree

Showing 4 changed files with 14 additions and 8 deletions.
diff --git a/pkg/bench/ddl_analysis/system_bench_test.go b/pkg/bench/ddl_analysis/system_bench_test.go
@@ -14,8 +14,8 @@ import "testing"
 
 func BenchmarkSystemDatabaseQueries(b *testing.B) {
 	tests := []RoundTripBenchTestCase{
-		// This query performs 2 lookups: getting the descriptor ID by name, then
-		// fetching the system table descriptor.
+		// This query performs 1-2 lookups: getting the descriptor ID by name, then
+		// fetching the system table descriptor. The descriptor is then cached.
 		{
 			name: "select system.users with schema name",
 			stmt: `SELECT username, "hashedPassword" FROM system.public.users WHERE username = 'root'`,
@@ -26,7 +26,7 @@ func BenchmarkSystemDatabaseQueries(b *testing.B) {
 			name: "select system.users without schema name",
 			stmt: `SELECT username, "hashedPassword" FROM system.users WHERE username = 'root'`,
 		},
-		// This query performs 0 extra lookup since the name resolution logic does
+		// This query performs 0 extra lookups since the name resolution logic does
 		// not try to resolve `"".system.users` and instead resolves
 		//`system.public.users` right away.
 		{

diff --git a/pkg/bench/ddl_analysis/testdata/benchmark_expectations b/pkg/bench/ddl_analysis/testdata/benchmark_expectations
@@ -65,6 +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
-2	2	SystemDatabaseQueries/select_system.users_with_empty_database_name
-2	2	SystemDatabaseQueries/select_system.users_with_schema_name
+2	2	SystemDatabaseQueries/select_system.users_without_schema_name
+1	1	SystemDatabaseQueries/select_system.users_with_empty_database_name
+1	1	SystemDatabaseQueries/select_system.users_with_schema_name
diff --git a/pkg/sql/catalog/descs/collection.go b/pkg/sql/catalog/descs/collection.go
@@ -457,11 +457,15 @@ func (tc *Collection) getObjectByName(
 	// But doing so turned problematic and the tests pass only by also
 	// disabling caching of system.eventlog, system.rangelog, and
 	// system.users. For now we're sticking to disabling caching of
-	// all system descriptors except the role-members-desc.
+	// all system descriptors except role_members, role_options, and users
+	// (i.e., the ones used during authn/authz flows).
 	// TODO (lucy): Reevaluate the above. We have many more system tables now and
 	// should be able to lease most of them.
+	isAllowedSystemTable := objectName == systemschema.RoleMembersTable.Name ||
+		objectName == systemschema.RoleOptionsTable.Name ||
+		objectName == systemschema.UsersTable.Name
 	avoidCache := flags.AvoidCached || mutable || lease.TestingTableLeasesAreDisabled() ||
-		(catalogName == systemschema.SystemDatabaseName && objectName != systemschema.RoleMembersTable.Name)
+		(catalogName == systemschema.SystemDatabaseName && !isAllowedSystemTable)
 	if avoidCache {
 		return tc.getDescriptorFromStore(
 			ctx, txn, tc.codec(), dbID, schemaID, objectName, mutable)

diff --git a/pkg/sql/logictest/testdata/logic_test/crdb_internal_tenant b/pkg/sql/logictest/testdata/logic_test/crdb_internal_tenant
@@ -9,7 +9,9 @@ query IT
 SELECT node_id, name FROM crdb_internal.leases ORDER BY name
 ----
 0  role_members
+0  role_options
 0  test
+0  users
 
 query error database "crdb_internal" does not exist
 ALTER DATABASE crdb_internal RENAME TO not_crdb_internal