Merge #114451 #114477 #114496 #114498

114451: opt: fix minor bug in optsteps[web] and exploretrace r=mgartner a=mgartner

This commit fixes a minor bug in the `optsteps`, `optstepsweb`, and
`exploretrace` commands that prevented some transformations from being
displayed. For example, the bug hid the problematic transformation that
was fixed by #114394. See the code comment for more details.

Epic: None

Release note: None


114477: sql: use role membership cache to check for role existence r=rafiss a=rafiss

Previously, the logic to check if a role existed would only cache that result for the duration of the current transaction. This behavior was chosen so that if a different session concurrently dropped that role, we the next time the current session started a transaction, it would check again if the role still existed. This was correct, but came at the cost of system table lookups at least once per-transaction.

Now the existence of a role is cached by making use of the role membership cache. The role membership cache similarly needs to be aware of changes made to role memberships that are performed by other sessions. Rather than looking up the memberships in each transaction, it has logic that causes the cache to be invalidated any time role memberships change (i.e., DROP ROLE, GRANT ROLE, or REVOKE ROLE). Now we check for the existence of a role before loading role memberships into the cache. As long as the cache does not get invalidated, we know that the role still exists.

No release note since this performance problem only appeared in alpha builds.

informs #114472

Release note: None

114496: github-pull-request-make: use longer test timeout, test fewer tests r=jlinder a=rickystewart

Try choosing only 4 tests to test instead of 5. Also increase the individual per-test timeout from 75% of duration to 90%.

Epic: none
Release note: None

114498: ui: fix sql activity app filter on internal queries r=xinhaoz a=xinhaoz

This commit fixes a bug when using the app filter on the statements sql activity page. Previously when the internal app name prefix, '$ internal' was selected from the dropdown, internal queries would not show up because we were performing an exact match on the application name and selected applications. This patch fixes this by matching on the prefix of the app name with the internal app name prefix when it is selected. In addition, internal queries are now only shown when the internal app name prefix is selected from the app filter dropdown. This reduces noise from internal queries and also matches the behaviour in the transactions page.

Epic: none
Fixes: #114461

https://www.loom.com/share/8fd77d2448ea4793a3ab225a46faa85e

Co-authored-by: Marcus Gartner <[email protected]>
Co-authored-by: Rafi Shamim <[email protected]>
Co-authored-by: Ricky Stewart <[email protected]>
Co-authored-by: Xin Hao Zhang <[email protected]>

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
-43-46,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,65 +52,65 @@ 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/asyncpg_types
-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/asyncpg_types
+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/cmd/github-pull-request-make/main.go

@@ -173,8 +173,8 @@ func chooseFiveTestsPerPackage(pkgs map[string]pkg) map[string]pkg {
 	croppedPkgs := make(map[string]pkg)
 	for pkgName, tests := range pkgs {
 		randomOrderTests := scrambleTestOrder(tests)
-		cropIdx := 5
-		if len(randomOrderTests) < 5 {
+		cropIdx := 4
+		if len(randomOrderTests) < cropIdx {
 			cropIdx = len(randomOrderTests)
 		}
 		croppedPkgs[pkgName] = makePkg(randomOrderTests[:cropIdx])
@@ -289,7 +289,7 @@ func main() {
 			}
 			// Use a timeout shorter than the duration so that hanging tests don't
 			// get a free pass.
-			timeout := (3 * duration) / 4
+			timeout := (9 * duration) / 10
 
 			// The stress -p flag defaults to the number of CPUs, which is too
 			// aggressive on big machines and can cause tests to fail. Under nightly

pkg/server/application_api/sql_stats_test.go

@@ -1117,7 +1117,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)
 
@@ -1126,7 +1126,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

@@ -1560,11 +1560,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{}
@@ -1976,7 +1971,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 {
@@ -3619,7 +3613,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

@@ -61,7 +61,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)