Merge pull request #114527 from rafiss/backport23.2-114477

release-23.2: sql: use role membership cache to check for role existence

pkg/bench/rttanalysis/testdata/benchmark_expectations

@@ -1,7 +1,7 @@
 exp,benchmark
-14,AlterRole/alter_role_with_1_option
-17,AlterRole/alter_role_with_2_options
-25,AlterRole/alter_role_with_3_options
+18,AlterRole/alter_role_with_1_option
+21,AlterRole/alter_role_with_2_options
+29,AlterRole/alter_role_with_3_options
 15,AlterTableAddCheckConstraint/alter_table_add_1_check_constraint
 15,AlterTableAddCheckConstraint/alter_table_add_2_check_constraints
 15,AlterTableAddCheckConstraint/alter_table_add_3_check_constraints
@@ -21,27 +21,27 @@ exp,benchmark
 15,AlterTableDropConstraint/alter_table_drop_1_check_constraint
 15,AlterTableDropConstraint/alter_table_drop_2_check_constraints
 15,AlterTableDropConstraint/alter_table_drop_3_check_constraints
-9,AlterTableSplit/alter_table_split_at_1_value
-12,AlterTableSplit/alter_table_split_at_2_values
-15,AlterTableSplit/alter_table_split_at_3_values
-8,AlterTableUnsplit/alter_table_unsplit_at_1_value
-10,AlterTableUnsplit/alter_table_unsplit_at_2_values
-12,AlterTableUnsplit/alter_table_unsplit_at_3_values
-6,Audit/select_from_an_audit_table
-20,CreateRole/create_role_with_1_option
-23,CreateRole/create_role_with_2_options
-26,CreateRole/create_role_with_3_options
-21,CreateRole/create_role_with_no_options
+8,AlterTableSplit/alter_table_split_at_1_value
+11,AlterTableSplit/alter_table_split_at_2_values
+14,AlterTableSplit/alter_table_split_at_3_values
+7,AlterTableUnsplit/alter_table_unsplit_at_1_value
+9,AlterTableUnsplit/alter_table_unsplit_at_2_values
+11,AlterTableUnsplit/alter_table_unsplit_at_3_values
+5,Audit/select_from_an_audit_table
+26,CreateRole/create_role_with_1_option
+29,CreateRole/create_role_with_2_options
+32,CreateRole/create_role_with_3_options
+27,CreateRole/create_role_with_no_options
 16,"Discard/DISCARD_ALL,_1_tables_in_1_db"
 23,"Discard/DISCARD_ALL,_2_tables_in_2_dbs"
 0,"Discard/DISCARD_ALL,_no_tables"
 17,DropDatabase/drop_database_0_tables
 18,DropDatabase/drop_database_1_table
 18,DropDatabase/drop_database_2_tables
 18,DropDatabase/drop_database_3_tables
-27,DropRole/drop_1_role
-36,DropRole/drop_2_roles
-45,DropRole/drop_3_roles
+34,DropRole/drop_1_role
+43,DropRole/drop_2_roles
+50-53,DropRole/drop_3_roles
 15,DropSequence/drop_1_sequence
 17,DropSequence/drop_2_sequences
 19,DropSequence/drop_3_sequences
@@ -52,64 +52,64 @@ exp,benchmark
 17,DropView/drop_2_views
 17,DropView/drop_3_views
 5,GenerateObjects/generate_1000_tables_-_this_test_should_use_the_same_number_of_RTTs_as_for_10_tables
-12,GenerateObjects/generate_100_tables_from_template
+11,GenerateObjects/generate_100_tables_from_template
 5,GenerateObjects/generate_10_tables
 16,GenerateObjects/generate_10x10_schemas_and_tables_in_existing_db
 5,GenerateObjects/generate_50000_tables
-13,Grant/grant_all_on_1_table
-17,Grant/grant_all_on_2_tables
-21,Grant/grant_all_on_3_tables
-16,GrantRole/grant_1_role
-21,GrantRole/grant_2_roles
-4,ORMQueries/activerecord_type_introspection_query
-1,ORMQueries/column_descriptions_json_agg
-5,ORMQueries/django_column_introspection_1_table
-5,ORMQueries/django_column_introspection_4_tables
-5,ORMQueries/django_column_introspection_8_tables
-7,ORMQueries/django_comment_introspection_with_comments
-7,ORMQueries/django_table_introspection_1_table
-7,ORMQueries/django_table_introspection_8_tables
+15,Grant/grant_all_on_1_table
+19,Grant/grant_all_on_2_tables
+23,Grant/grant_all_on_3_tables
+19,GrantRole/grant_1_role
+25,GrantRole/grant_2_roles
+3,ORMQueries/activerecord_type_introspection_query
+0,ORMQueries/column_descriptions_json_agg
+4,ORMQueries/django_column_introspection_1_table
+4,ORMQueries/django_column_introspection_4_tables
+4,ORMQueries/django_column_introspection_8_tables
+6,ORMQueries/django_comment_introspection_with_comments
+6,ORMQueries/django_table_introspection_1_table
+6,ORMQueries/django_table_introspection_8_tables
 0,ORMQueries/has_column_privilege_using_attnum
 0,ORMQueries/has_column_privilege_using_column_name
 0,ORMQueries/has_schema_privilege
 0,ORMQueries/has_sequence_privilege
 0,ORMQueries/has_table_privilege
-6,ORMQueries/hasura_column_descriptions
-13,ORMQueries/hasura_column_descriptions_8_tables
-6,ORMQueries/hasura_column_descriptions_modified
-5,ORMQueries/information_schema._pg_index_position
-5,ORMQueries/introspection_description_join
-6,ORMQueries/npgsql_fields
-6,ORMQueries/npgsql_types
-5,ORMQueries/pg_attribute
-5,ORMQueries/pg_class
-7,ORMQueries/pg_is_other_temp_schema
-7,ORMQueries/pg_is_other_temp_schema_multiple_times
-5,ORMQueries/pg_my_temp_schema
-5,ORMQueries/pg_my_temp_schema_multiple_times
-4,ORMQueries/pg_namespace
-5,ORMQueries/pg_type
-132,ORMQueries/prisma_column_descriptions
-4,ORMQueries/prisma_column_descriptions_updated
-13,Revoke/revoke_all_on_1_table
-17,Revoke/revoke_all_on_2_tables
-21,Revoke/revoke_all_on_3_tables
-15,RevokeRole/revoke_1_role
-18,RevokeRole/revoke_2_roles
-12,ShowGrants/grant_2_roles
-13,ShowGrants/grant_3_roles
-14,ShowGrants/grant_4_roles
+5,ORMQueries/hasura_column_descriptions
+12,ORMQueries/hasura_column_descriptions_8_tables
+5,ORMQueries/hasura_column_descriptions_modified
+4,ORMQueries/information_schema._pg_index_position
+4,ORMQueries/introspection_description_join
+5,ORMQueries/npgsql_fields
+5,ORMQueries/npgsql_types
+4,ORMQueries/pg_attribute
+4,ORMQueries/pg_class
+6,ORMQueries/pg_is_other_temp_schema
+6,ORMQueries/pg_is_other_temp_schema_multiple_times
+3,ORMQueries/pg_my_temp_schema
+3,ORMQueries/pg_my_temp_schema_multiple_times
+3,ORMQueries/pg_namespace
+4,ORMQueries/pg_type
+133,ORMQueries/prisma_column_descriptions
+3,ORMQueries/prisma_column_descriptions_updated
+15,Revoke/revoke_all_on_1_table
+19,Revoke/revoke_all_on_2_tables
+23,Revoke/revoke_all_on_3_tables
+17,RevokeRole/revoke_1_role
+21,RevokeRole/revoke_2_roles
+14,ShowGrants/grant_2_roles
+16,ShowGrants/grant_3_roles
+18,ShowGrants/grant_4_roles
 1,SystemDatabaseQueries/select_system.users_with_empty_database_Name
 1,SystemDatabaseQueries/select_system.users_with_schema_Name
 1,SystemDatabaseQueries/select_system.users_without_schema_Name
-21,Truncate/truncate_1_column_0_rows
-21,Truncate/truncate_1_column_1_row
-21,Truncate/truncate_1_column_2_rows
-21,Truncate/truncate_2_column_0_rows
-21,Truncate/truncate_2_column_1_rows
-21,Truncate/truncate_2_column_2_rows
-4,UDFResolution/select_from_udf
-2,VirtualTableQueries/select_crdb_internal.invalid_objects_with_1_fk
-2,VirtualTableQueries/select_crdb_internal.tables_with_1_fk
-10,VirtualTableQueries/virtual_table_cache_with_point_lookups
-16,VirtualTableQueries/virtual_table_cache_with_schema_change
+20,Truncate/truncate_1_column_0_rows
+20,Truncate/truncate_1_column_1_row
+20,Truncate/truncate_1_column_2_rows
+20,Truncate/truncate_2_column_0_rows
+20,Truncate/truncate_2_column_1_rows
+20,Truncate/truncate_2_column_2_rows
+3,UDFResolution/select_from_udf
+1,VirtualTableQueries/select_crdb_internal.invalid_objects_with_1_fk
+1,VirtualTableQueries/select_crdb_internal.tables_with_1_fk
+9,VirtualTableQueries/virtual_table_cache_with_point_lookups
+15,VirtualTableQueries/virtual_table_cache_with_schema_change

pkg/ccl/streamingccl/streamproducer/replication_manager_test.go

@@ -70,6 +70,7 @@ func TestReplicationManagerRequiresReplicationPrivilege(t *testing.T) {
 
 	tDB.Exec(t, "CREATE ROLE somebody")
 	tDB.Exec(t, "GRANT SYSTEM REPLICATION TO somebody")
+	tDB.Exec(t, "CREATE ROLE anybody")
 
 	for _, tc := range []struct {
 		user         string
@@ -79,12 +80,14 @@ func TestReplicationManagerRequiresReplicationPrivilege(t *testing.T) {
 		{user: "admin", expErr: "", isEnterprise: true},
 		{user: "root", expErr: "", isEnterprise: true},
 		{user: "somebody", expErr: "", isEnterprise: true},
-		{user: "nobody", expErr: "user nobody does not have REPLICATION system privilege", isEnterprise: true},
+		{user: "anybody", expErr: "user anybody does not have REPLICATION system privilege", isEnterprise: true},
+		{user: "nobody", expErr: `role/user "nobody" does not exist`, isEnterprise: true},
 
 		{user: "admin", expErr: "use of REPLICATION requires an enterprise license", isEnterprise: false},
 		{user: "root", expErr: " use of REPLICATION requires an enterprise license", isEnterprise: false},
 		{user: "somebody", expErr: "use of REPLICATION requires an enterprise license", isEnterprise: false},
-		{user: "nobody", expErr: "user nobody does not have REPLICATION system privilege", isEnterprise: false},
+		{user: "anybody", expErr: "user anybody does not have REPLICATION system privilege", isEnterprise: false},
+		{user: "nobody", expErr: `role/user "nobody" does not exist`, isEnterprise: false},
 	} {
 		t.Run(fmt.Sprintf("%s/ent=%t", tc.user, tc.isEnterprise), func(t *testing.T) {
 			if tc.isEnterprise {

pkg/server/application_api/sql_stats_test.go

@@ -1113,7 +1113,7 @@ func TestUnprivilegedUserResetIndexUsageStats(t *testing.T) {
 	defer s.Stopper().Stop(ctx)
 
 	sqlConn := sqlutils.MakeSQLRunner(conn)
-	sqlConn.Exec(t, "CREATE USER nonAdminUser")
+	sqlConn.Exec(t, "CREATE USER non_admin_user")
 
 	ie := s.InternalExecutor().(*sql.InternalExecutor)
 
@@ -1122,7 +1122,7 @@ func TestUnprivilegedUserResetIndexUsageStats(t *testing.T) {
 		"test-reset-index-usage-stats-as-non-admin-user",
 		nil, /* txn */
 		sessiondata.InternalExecutorOverride{
-			User: username.MakeSQLUsernameFromPreNormalizedString("nonAdminUser"),
+			User: username.MakeSQLUsernameFromPreNormalizedString("non_admin_user"),
 		},
 		"SELECT crdb_internal.reset_index_usage_stats()",
 	)

pkg/sql/authorization.go

@@ -188,10 +188,8 @@ func (p *planner) HasPrivilege(
 			// lookup in common cases (e.g., internal executor usages).
 			return true, nil
 		}
-		if exists, err := p.RoleExists(ctx, user); err != nil {
-			return false, err
-		} else if !exists {
-			return false, pgerror.Newf(pgcode.UndefinedObject, "role %s was concurrently dropped", user)
+		if err := p.CheckRoleExists(ctx, user); err != nil {
+			return false, pgerror.Wrapf(err, pgcode.UndefinedObject, "role %s was concurrently dropped", user)
 		}
 		return true, nil
 	}
@@ -482,6 +480,9 @@ func (p *planner) UserHasAdminRole(ctx context.Context, user username.SQLUsernam
 	if user.IsAdminRole() || user.IsRootUser() || user.IsNodeUser() {
 		return true, nil
 	}
+	if user.IsPublicRole() {
+		return false, nil
+	}
 
 	// Expand role memberships.
 	memberOf, err := p.MemberOfWithAdminOption(ctx, user)
@@ -666,6 +667,12 @@ var useSingleQueryForRoleMembershipCache = settings.RegisterBoolSetting(
 func resolveMemberOfWithAdminOption(
 	ctx context.Context, member username.SQLUsername, txn isql.Txn, singleQuery bool,
 ) (map[username.SQLUsername]bool, error) {
+	roleExists, err := RoleExists(ctx, txn, member)
+	if err != nil {
+		return nil, err
+	} else if !roleExists {
+		return nil, sqlerrors.NewUndefinedUserError(member)
+	}
 	ret := map[username.SQLUsername]bool{}
 	if singleQuery {
 		type membership struct {
@@ -908,13 +915,9 @@ func (p *planner) checkCanAlterToNewOwner(
 	ctx context.Context, desc catalog.MutableDescriptor, newOwner username.SQLUsername,
 ) error {
 	// Make sure the newOwner exists.
-	roleExists, err := p.RoleExists(ctx, newOwner)
-	if err != nil {
+	if err := p.CheckRoleExists(ctx, newOwner); err != nil {
 		return err
 	}
-	if !roleExists {
-		return sqlerrors.NewUndefinedUserError(newOwner)
-	}
 
 	// If the user is a superuser, skip privilege checks.
 	hasAdmin, err := p.HasAdminRole(ctx)

pkg/sql/conn_executor.go

@@ -1562,11 +1562,6 @@ type connExecutor struct {
 		// in a transaction.
 		hasAdminRoleCache HasAdminRoleCache
 
-		// roleExistsCache is a cache of role existence checks. This is used because
-		// role existence checks are made when checking privileges. Only positive
-		// values are cached.
-		roleExistsCache map[username.SQLUsername]struct{}
-
 		// createdSequences keeps track of sequences created in the current transaction.
 		// The map key is the sequence descpb.ID.
 		createdSequences map[descpb.ID]struct{}
@@ -1978,7 +1973,6 @@ func (ex *connExecutor) resetExtraTxnState(ctx context.Context, ev txnEvent, pay
 	ex.extraTxnState.numDDL = 0
 	ex.extraTxnState.firstStmtExecuted = false
 	ex.extraTxnState.hasAdminRoleCache = HasAdminRoleCache{}
-	ex.extraTxnState.roleExistsCache = make(map[username.SQLUsername]struct{})
 	ex.extraTxnState.createdSequences = nil
 
 	if ex.extraTxnState.fromOuterTxn {
@@ -3625,7 +3619,6 @@ func (ex *connExecutor) resetEvalCtx(evalCtx *extendedEvalContext, txn *kv.Txn,
 	evalCtx.SkipNormalize = false
 	evalCtx.SchemaChangerState = ex.extraTxnState.schemaChangerState
 	evalCtx.DescIDGenerator = ex.getDescIDGenerator()
-	evalCtx.RoleExistsCache = ex.extraTxnState.roleExistsCache
 
 	// See resetPlanner for more context on setting the maximum timestamp for
 	// AOST read retries.

pkg/sql/create_role.go

@@ -189,6 +189,11 @@ func (n *CreateRoleNode) startExec(params runParams) error {
 			return err
 		}
 	}
+	// Bump role membership table version to force a refresh of role membership
+	// cache.
+	if err := params.p.BumpRoleMembershipTableVersion(params.ctx); err != nil {
+		return err
+	}
 
 	return params.p.logEvent(params.ctx,
 		0, /* no target */

pkg/sql/delegate/BUILD.bazel

@@ -62,7 +62,6 @@ go_library(
         "//pkg/sql/sem/eval",
         "//pkg/sql/sem/tree",
         "//pkg/sql/sessiondatapb",
-        "//pkg/sql/sqlerrors",
         "//pkg/sql/sqltelemetry",
         "//pkg/sql/syntheticprivilege",
         "//pkg/util/errorutil/unimplemented",

pkg/sql/delegate/show_grants.go

@@ -23,7 +23,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/opt/cat"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/catconstants"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
-	"github.com/cockroachdb/cockroach/pkg/sql/sqlerrors"
 	"github.com/cockroachdb/cockroach/pkg/util/intsets"
 )
 
@@ -410,14 +409,10 @@ ORDER BY
 		// are used with `public`.
 		userExists := user.IsPublicRole()
 		if !userExists {
-			userExists, err = d.catalog.RoleExists(d.ctx, user)
-			if err != nil {
+			if err := d.catalog.CheckRoleExists(d.ctx, user); err != nil {
 				return nil, err
 			}
 		}
-		if !userExists {
-			return nil, sqlerrors.NewUndefinedUserError(user)
-		}
 	}
 
 	return d.parse(query)