From 9be6d5a0e1610b3744ddf91aaf855f5470ee842c Mon Sep 17 00:00:00 2001 From: Michael Butler Date: Wed, 22 Nov 2023 11:19:06 -0700 Subject: [PATCH 1/3] streamingccl: only return lag replanning error if lagging node has not advanced Previously, the frontier processor would return a lag replanning error if it detected a lagging node and after the hwm had advanced during the flow. This implies the frontier processor could replan as soon as a lagging node finished its catchup scan and bumped the hwm, but was still far behind the other nodes, as we observed in #114706. Ideally, the frontier processor should not throw this replanning error because the lagging node is making progress and because replanning can cause repeated work. This patch prevents this scenario by teaching the frontier processor to only throw a replanning error if: - the hwm has advanced in the flow - two consecutive lagging node checks detected a lagging node and the hwm has not advanced during those two checks. Informs #114706 Release note: none --- .../stream_ingestion_frontier_processor.go | 26 ++++++- ...tream_ingestion_frontier_processor_test.go | 75 +++++++++++++++++++ pkg/cmd/roachtest/tests/cluster_to_cluster.go | 2 +- 3 files changed, 101 insertions(+), 2 deletions(-) diff --git a/pkg/ccl/streamingccl/streamingest/stream_ingestion_frontier_processor.go b/pkg/ccl/streamingccl/streamingest/stream_ingestion_frontier_processor.go index 37edce01081f..76f83e278069 100644 --- a/pkg/ccl/streamingccl/streamingest/stream_ingestion_frontier_processor.go +++ b/pkg/ccl/streamingccl/streamingest/stream_ingestion_frontier_processor.go @@ -78,6 +78,10 @@ type streamIngestionFrontier struct { partitionProgress map[string]jobspb.StreamIngestionProgress_PartitionProgress lastNodeLagCheck time.Time + + // replicatedTimeAtLastPositiveLagNodeCheck records the replicated time the + // last time the lagging node checker detected a lagging node. + replicatedTimeAtLastPositiveLagNodeCheck hlc.Timestamp } var _ execinfra.Processor = &streamIngestionFrontier{} @@ -553,9 +557,29 @@ func (sf *streamIngestionFrontier) maybeCheckForLaggingNodes() error { }() executionDetails := constructSpanFrontierExecutionDetailsWithFrontier(sf.spec.PartitionSpecs, sf.frontier) log.VEvent(ctx, 2, "checking for lagging nodes") - return checkLaggingNodes( + err := checkLaggingNodes( ctx, executionDetails, maxLag, ) + return sf.handleLaggingNodeError(ctx, err) +} + +func (sf *streamIngestionFrontier) handleLaggingNodeError(ctx context.Context, err error) error { + switch { + case err == nil: + sf.replicatedTimeAtLastPositiveLagNodeCheck = hlc.Timestamp{} + log.VEvent(ctx, 2, "no lagging nodes after check") + return nil + case !errors.Is(err, ErrNodeLagging): + return err + case sf.replicatedTimeAtLastPositiveLagNodeCheck.Less(sf.persistedReplicatedTime): + sf.replicatedTimeAtLastPositiveLagNodeCheck = sf.persistedReplicatedTime + log.Infof(ctx, "detected a lagging node: %s. Don't forward error because replicated time at last check %s is less than current replicated time %s", err, sf.replicatedTimeAtLastPositiveLagNodeCheck, sf.persistedReplicatedTime) + return nil + case sf.replicatedTimeAtLastPositiveLagNodeCheck.Equal(sf.persistedReplicatedTime): + return errors.Wrapf(err, "hwm has not advanced from %s", sf.persistedReplicatedTime) + default: + return errors.Wrapf(err, "unable to handle replanning error with replicated time %s and last node lag check replicated time %s", sf.persistedReplicatedTime, sf.replicatedTimeAtLastPositiveLagNodeCheck) + } } diff --git a/pkg/ccl/streamingccl/streamingest/stream_ingestion_frontier_processor_test.go b/pkg/ccl/streamingccl/streamingest/stream_ingestion_frontier_processor_test.go index 754300aaa703..209efe2d0b24 100644 --- a/pkg/ccl/streamingccl/streamingest/stream_ingestion_frontier_processor_test.go +++ b/pkg/ccl/streamingccl/streamingest/stream_ingestion_frontier_processor_test.go @@ -152,3 +152,78 @@ func TestHeartbeatLoop(t *testing.T) { } } } + +func TestLaggingNodeErrorHandler(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + + ctx := context.Background() + + type testCase struct { + name string + replicatedTime int64 + previousReplicatedTimeOnLag int64 + inputLagErr error + + expectedNewReplicatedTimeOnLag int64 + expectedErrMsg string + } + + for _, tc := range []testCase{ + { + name: "no more lag", + previousReplicatedTimeOnLag: 1, + expectedNewReplicatedTimeOnLag: 0, + }, + { + name: "new lag", + previousReplicatedTimeOnLag: 0, + replicatedTime: 1, + expectedNewReplicatedTimeOnLag: 1, + inputLagErr: ErrNodeLagging, + }, + { + name: "repeated lag, no hwm advance", + previousReplicatedTimeOnLag: 1, + replicatedTime: 1, + expectedNewReplicatedTimeOnLag: 1, + inputLagErr: ErrNodeLagging, + expectedErrMsg: ErrNodeLagging.Error(), + }, + { + name: "repeated lag, with hwm advance", + previousReplicatedTimeOnLag: 1, + replicatedTime: 2, + expectedNewReplicatedTimeOnLag: 2, + inputLagErr: ErrNodeLagging, + }, + { + name: "non lag error", + inputLagErr: errors.New("unexpected"), + expectedErrMsg: "unexpected", + }, + { + name: "unhandlable lag error", + previousReplicatedTimeOnLag: 2, + replicatedTime: 1, + expectedNewReplicatedTimeOnLag: 2, + inputLagErr: ErrNodeLagging, + expectedErrMsg: "unable to handle replanning", + }, + } { + tc := tc + t.Run(tc.name, func(t *testing.T) { + sf := streamIngestionFrontier{ + persistedReplicatedTime: hlc.Timestamp{WallTime: tc.replicatedTime}, + replicatedTimeAtLastPositiveLagNodeCheck: hlc.Timestamp{WallTime: tc.previousReplicatedTimeOnLag}, + } + err := sf.handleLaggingNodeError(ctx, tc.inputLagErr) + if tc.expectedErrMsg == "" { + require.NoError(t, err) + } else { + require.ErrorContains(t, err, tc.expectedErrMsg) + } + require.Equal(t, hlc.Timestamp{WallTime: tc.expectedNewReplicatedTimeOnLag}, sf.replicatedTimeAtLastPositiveLagNodeCheck) + }) + } +} diff --git a/pkg/cmd/roachtest/tests/cluster_to_cluster.go b/pkg/cmd/roachtest/tests/cluster_to_cluster.go index b5e334d5097c..83d3b7ad328d 100644 --- a/pkg/cmd/roachtest/tests/cluster_to_cluster.go +++ b/pkg/cmd/roachtest/tests/cluster_to_cluster.go @@ -1752,7 +1752,7 @@ func destClusterSettings(t test.Test, db *sqlutils.SQLRunner, additionalDuration db.ExecMultiple(t, `SET CLUSTER SETTING cross_cluster_replication.enabled = true;`, `SET CLUSTER SETTING kv.rangefeed.enabled = true;`, `SET CLUSTER SETTING stream_replication.replan_flow_threshold = 0.1;`, - `SET CLUSTER SETTING physical_replication.consumer.node_lag_replanning_threshold = '10m';`) + `SET CLUSTER SETTING physical_replication.consumer.node_lag_replanning_threshold = '5m';`) if additionalDuration != 0 { replanFrequency := additionalDuration / 2 From 47e22e1e028c65165697bea59ccfb2be21cd95fa Mon Sep 17 00:00:00 2001 From: Rafi Shamim Date: Fri, 17 Nov 2023 10:41:12 -0500 Subject: [PATCH 2/3] server: remove admin checks from server-v2 These are now replaced by checks for the VIEWCLUSTERMETADATA privilege. The equivalent change was already made on the v1 server. Release note: None --- pkg/ccl/changefeedccl/cdceval/doc.go | 1 - .../testdata/logic_test/crdb_internal_tenant | 17 ------- pkg/server/api_v2.go | 31 ++++++++--- pkg/server/authserver/BUILD.bazel | 2 + pkg/server/authserver/api_v2.go | 11 ++-- pkg/server/authserver/api_v2_auth.go | 51 ++++++++----------- pkg/server/privchecker/BUILD.bazel | 3 -- pkg/server/privchecker/privchecker.go | 28 ++-------- pkg/sql/internal_test.go | 47 ----------------- .../testdata/logic_test/crdb_internal | 16 ------ pkg/sql/logictest/testdata/logic_test/role | 6 --- pkg/sql/sem/builtins/builtins.go | 25 --------- pkg/sql/sem/builtins/fixed_oids.go | 1 - 13 files changed, 56 insertions(+), 183 deletions(-) diff --git a/pkg/ccl/changefeedccl/cdceval/doc.go b/pkg/ccl/changefeedccl/cdceval/doc.go index 202adb54cfc0..bd89795c41fb 100644 --- a/pkg/ccl/changefeedccl/cdceval/doc.go +++ b/pkg/ccl/changefeedccl/cdceval/doc.go @@ -134,7 +134,6 @@ functions along with the number of arguments and the return type of the overload crdb_internal.get_namespace_id | 2 | int8 crdb_internal.get_namespace_id | 3 | int8 crdb_internal.get_zone_config | 1 | bytea - crdb_internal.is_admin | 0 | bool crdb_internal.locality_value | 1 | text crdb_internal.node_id | 0 | int8 crdb_internal.num_geo_inverted_index_entries | 3 | int8 diff --git a/pkg/ccl/logictestccl/testdata/logic_test/crdb_internal_tenant b/pkg/ccl/logictestccl/testdata/logic_test/crdb_internal_tenant index 52236f38f988..483619c8a4e3 100644 --- a/pkg/ccl/logictestccl/testdata/logic_test/crdb_internal_tenant +++ b/pkg/ccl/logictestccl/testdata/logic_test/crdb_internal_tenant @@ -572,23 +572,6 @@ SELECT FROM table41834; - -subtest builtin_is_admin - -user root - -query B -SELECT crdb_internal.is_admin() ----- -true - -user testuser - -query B -SELECT crdb_internal.is_admin() ----- -false - user root # Test the crdb_internal.create_type_statements table. diff --git a/pkg/server/api_v2.go b/pkg/server/api_v2.go index 754b580a3a2e..fdda0e133763 100644 --- a/pkg/server/api_v2.go +++ b/pkg/server/api_v2.go @@ -40,12 +40,14 @@ import ( "strconv" "github.com/cockroachdb/cockroach/pkg/kv" + "github.com/cockroachdb/cockroach/pkg/security/username" "github.com/cockroachdb/cockroach/pkg/server/apiconstants" "github.com/cockroachdb/cockroach/pkg/server/apiutil" "github.com/cockroachdb/cockroach/pkg/server/authserver" "github.com/cockroachdb/cockroach/pkg/server/serverpb" "github.com/cockroachdb/cockroach/pkg/server/srverrors" "github.com/cockroachdb/cockroach/pkg/server/telemetry" + "github.com/cockroachdb/cockroach/pkg/sql" "github.com/cockroachdb/cockroach/pkg/util/httputil" "github.com/cockroachdb/cockroach/pkg/util/metric" "github.com/gorilla/mux" @@ -169,16 +171,16 @@ func registerRoutes( {"logout/", a.authServer.ServeHTTP, false /* requiresAuth */, authserver.RegularRole, false}, // Directly register other endpoints in the api server. - {"sessions/", a.listSessions, true /* requiresAuth */, authserver.AdminRole, false}, - {"nodes/", systemRoutes.listNodes, true, authserver.AdminRole, false}, + {"sessions/", a.listSessions, true /* requiresAuth */, authserver.ViewClusterMetadataRole, false}, + {"nodes/", systemRoutes.listNodes, true, authserver.ViewClusterMetadataRole, false}, // Any endpoint returning range information requires an admin user. This is because range start/end keys // are sensitive info. - {"nodes/{node_id}/ranges/", systemRoutes.listNodeRanges, true, authserver.AdminRole, false}, - {"ranges/hot/", a.listHotRanges, true, authserver.AdminRole, false}, - {"ranges/{range_id:[0-9]+}/", a.listRange, true, authserver.AdminRole, false}, + {"nodes/{node_id}/ranges/", systemRoutes.listNodeRanges, true, authserver.ViewClusterMetadataRole, false}, + {"ranges/hot/", a.listHotRanges, true, authserver.ViewClusterMetadataRole, false}, + {"ranges/{range_id:[0-9]+}/", a.listRange, true, authserver.ViewClusterMetadataRole, false}, {"health/", systemRoutes.health, false, authserver.RegularRole, false}, {"users/", a.listUsers, true, authserver.RegularRole, false}, - {"events/", a.listEvents, true, authserver.AdminRole, false}, + {"events/", a.listEvents, true, authserver.ViewClusterMetadataRole, false}, {"databases/", a.listDatabases, true, authserver.RegularRole, false}, {"databases/{database_name:[\\w.]+}/", a.databaseDetails, true, authserver.RegularRole, false}, {"databases/{database_name:[\\w.]+}/grants/", a.databaseGrants, true, authserver.RegularRole, false}, @@ -203,10 +205,25 @@ func registerRoutes( http.Error(w, "Not Available on Tenants", http.StatusNotImplemented) })) } + + // Tell the authz server how to connect to SQL. + authzAccessorFactory := func(ctx context.Context, opName string) (sql.AuthorizationAccessor, func()) { + txn := a.db.NewTxn(ctx, opName) + p, cleanup := sql.NewInternalPlanner( + opName, + txn, + username.RootUserName(), + &sql.MemoryMetrics{}, + a.sqlServer.execCfg, + sql.NewInternalSessionData(ctx, a.sqlServer.execCfg.Settings, opName), + ) + return p.(sql.AuthorizationAccessor), cleanup + } + if route.requiresAuth { a.mux.Handle(apiconstants.APIV2Path+route.url, authMux) if route.role != authserver.RegularRole { - handler = authserver.NewRoleAuthzMux(a.sqlServer.internalExecutor, route.role, handler) + handler = authserver.NewRoleAuthzMux(authzAccessorFactory, route.role, handler) } innerMux.Handle(apiconstants.APIV2Path+route.url, handler) } else { diff --git a/pkg/server/authserver/BUILD.bazel b/pkg/server/authserver/BUILD.bazel index 3759f2a4901a..53cc66af0c48 100644 --- a/pkg/server/authserver/BUILD.bazel +++ b/pkg/server/authserver/BUILD.bazel @@ -26,8 +26,10 @@ go_library( "//pkg/settings/cluster", "//pkg/sql", "//pkg/sql/isql", + "//pkg/sql/privilege", "//pkg/sql/sem/tree", "//pkg/sql/sessiondata", + "//pkg/sql/syntheticprivilege", "//pkg/sql/types", "//pkg/ui", "//pkg/util/grpcutil", diff --git a/pkg/server/authserver/api_v2.go b/pkg/server/authserver/api_v2.go index 25e86d0fa091..1361e8b6bb58 100644 --- a/pkg/server/authserver/api_v2.go +++ b/pkg/server/authserver/api_v2.go @@ -15,7 +15,6 @@ import ( "net/http" "github.com/cockroachdb/cockroach/pkg/base" - "github.com/cockroachdb/cockroach/pkg/sql/isql" ) type ServerV2 interface { @@ -61,10 +60,12 @@ func NewV2Mux(s ServerV2, inner http.Handler, allowAnonymous bool) AuthV2Mux { } // NewRoleAuthzMux creates a new RoleAuthzMux. -func NewRoleAuthzMux(ie isql.Executor, role APIRole, inner http.Handler) RoleAuthzMux { +func NewRoleAuthzMux( + authzAccessorFactory authzAccessorFactory, role APIRole, inner http.Handler, +) RoleAuthzMux { return &roleAuthorizationMux{ - ie: ie, - role: role, - inner: inner, + authzAccessorFactory: authzAccessorFactory, + role: role, + inner: inner, } } diff --git a/pkg/server/authserver/api_v2_auth.go b/pkg/server/authserver/api_v2_auth.go index cf6292ffc107..04dd60439003 100644 --- a/pkg/server/authserver/api_v2_auth.go +++ b/pkg/server/authserver/api_v2_auth.go @@ -19,9 +19,10 @@ import ( "github.com/cockroachdb/cockroach/pkg/server/apiutil" "github.com/cockroachdb/cockroach/pkg/server/serverpb" "github.com/cockroachdb/cockroach/pkg/server/srverrors" - "github.com/cockroachdb/cockroach/pkg/sql/isql" - "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" + "github.com/cockroachdb/cockroach/pkg/sql" + "github.com/cockroachdb/cockroach/pkg/sql/privilege" "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" + "github.com/cockroachdb/cockroach/pkg/sql/syntheticprivilege" "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/cockroach/pkg/util/protoutil" "github.com/cockroachdb/errors" @@ -344,22 +345,21 @@ type APIRole int const ( // RegularRole is the default role for an APIv2 endpoint. RegularRole APIRole = iota - // AdminRole is the role for an APIv2 endpoint that requires - // admin privileges. - AdminRole - // SuperUserRole is the role for an APIv2 endpoint that requires - // superuser privileges. - SuperUserRole + // ViewClusterMetadataRole is the role for an APIv2 endpoint that requires + // VIEWCLUSTERMETADATA privileges. + ViewClusterMetadataRole ) +type authzAccessorFactory func(ctx context.Context, opName string) (_ sql.AuthorizationAccessor, cleanup func()) + // roleAuthorizationMux enforces a role (eg. type of user) for an arbitrary // inner mux. Meant to be used under authenticationV2Mux. If the logged-in user // is not at least of `role` type, an HTTP 403 forbidden error is returned. // Otherwise, the request is passed onto the inner http.Handler. type roleAuthorizationMux struct { - ie isql.Executor - role APIRole - inner http.Handler + authzAccessorFactory authzAccessorFactory + role APIRole + inner http.Handler } func (r *roleAuthorizationMux) getRoleForUser( @@ -367,29 +367,20 @@ func (r *roleAuthorizationMux) getRoleForUser( ) (APIRole, error) { if user.IsRootUser() { // Shortcut. - return SuperUserRole, nil + return ViewClusterMetadataRole, nil } - row, err := r.ie.QueryRowEx( - ctx, "check-is-admin", nil, /* txn */ - sessiondata.InternalExecutorOverride{User: user}, - "SELECT crdb_internal.is_admin()") + + authzAccessor, cleanup := r.authzAccessorFactory(ctx, "check-privilege") + defer cleanup() + + hasPriv, err := authzAccessor.HasPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.VIEWCLUSTERMETADATA, user) if err != nil { return RegularRole, err + } else if hasPriv { + return ViewClusterMetadataRole, nil + } else { + return RegularRole, nil } - if row == nil { - return RegularRole, errors.AssertionFailedf("hasAdminRole: expected 1 row, got 0") - } - if len(row) != 1 { - return RegularRole, errors.AssertionFailedf("hasAdminRole: expected 1 column, got %d", len(row)) - } - dbDatum, ok := tree.AsDBool(row[0]) - if !ok { - return RegularRole, errors.AssertionFailedf("hasAdminRole: expected bool, got %T", row[0]) - } - if dbDatum { - return AdminRole, nil - } - return RegularRole, nil } func (r *roleAuthorizationMux) ServeHTTP(w http.ResponseWriter, req *http.Request) { diff --git a/pkg/server/privchecker/BUILD.bazel b/pkg/server/privchecker/BUILD.bazel index 87dd0fe9d1ac..a20c73eb4336 100644 --- a/pkg/server/privchecker/BUILD.bazel +++ b/pkg/server/privchecker/BUILD.bazel @@ -17,10 +17,7 @@ go_library( "//pkg/sql/isql", "//pkg/sql/privilege", "//pkg/sql/roleoption", - "//pkg/sql/sem/tree", - "//pkg/sql/sessiondata", "//pkg/sql/syntheticprivilege", - "@com_github_cockroachdb_errors//:errors", "@org_golang_google_grpc//codes", "@org_golang_google_grpc//status", ], diff --git a/pkg/server/privchecker/privchecker.go b/pkg/server/privchecker/privchecker.go index 63f132f96369..18b1be35be4a 100644 --- a/pkg/server/privchecker/privchecker.go +++ b/pkg/server/privchecker/privchecker.go @@ -21,10 +21,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/isql" "github.com/cockroachdb/cockroach/pkg/sql/privilege" "github.com/cockroachdb/cockroach/pkg/sql/roleoption" - "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" - "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" "github.com/cockroachdb/cockroach/pkg/sql/syntheticprivilege" - "github.com/cockroachdb/errors" "google.golang.org/grpc/codes" grpcstatus "google.golang.org/grpc/status" ) @@ -247,28 +244,9 @@ func (c *adminPrivilegeChecker) GetUserAndRole( func (c *adminPrivilegeChecker) HasAdminRole( ctx context.Context, user username.SQLUsername, ) (bool, error) { - if user.IsRootUser() { - // Shortcut. - return true, nil - } - row, err := c.ie.QueryRowEx( - ctx, "check-is-admin", nil, /* txn */ - sessiondata.InternalExecutorOverride{User: user}, - "SELECT crdb_internal.is_admin()") - if err != nil { - return false, err - } - if row == nil { - return false, errors.AssertionFailedf("hasAdminRole: expected 1 row, got 0") - } - if len(row) != 1 { - return false, errors.AssertionFailedf("hasAdminRole: expected 1 column, got %d", len(row)) - } - dbDatum, ok := tree.AsDBool(row[0]) - if !ok { - return false, errors.AssertionFailedf("hasAdminRole: expected bool, got %T", row[0]) - } - return bool(dbDatum), nil + aa, cleanup := c.makeAuthzAccessor("check-admin-role") + defer cleanup() + return aa.UserHasAdminRole(ctx, user) } // HasRoleOption is part of the SQLPrivilegeChecker interface. diff --git a/pkg/sql/internal_test.go b/pkg/sql/internal_test.go index 62259dd46f95..50419c17b8ba 100644 --- a/pkg/sql/internal_test.go +++ b/pkg/sql/internal_test.go @@ -200,53 +200,6 @@ func TestInternalStmtFingerprintLimit(t *testing.T) { require.NoError(t, err) } -func TestQueryIsAdminWithNoTxn(t *testing.T) { - defer leaktest.AfterTest(t)() - defer log.Scope(t).Close(t) - - ctx := context.Background() - params, _ := createTestServerParams() - s, db, _ := serverutils.StartServer(t, params) - defer s.Stopper().Stop(ctx) - - if _, err := db.Exec("create user testuser"); err != nil { - t.Fatal(err) - } - - ie := s.InternalExecutor().(*sql.InternalExecutor) - - testData := []struct { - user username.SQLUsername - expAdmin bool - }{ - {username.NodeUserName(), true}, - {username.RootUserName(), true}, - {username.TestUserName(), false}, - } - - for _, tc := range testData { - t.Run(tc.user.Normalized(), func(t *testing.T) { - row, cols, err := ie.QueryRowExWithCols(ctx, "test", nil, /* txn */ - sessiondata.InternalExecutorOverride{User: tc.user}, - "SELECT crdb_internal.is_admin()") - if err != nil { - t.Fatal(err) - } - if row == nil || len(cols) != 1 { - numRows := 0 - if row != nil { - numRows = 1 - } - t.Fatalf("unexpected result shape %d, %d", numRows, len(cols)) - } - isAdmin := bool(*row[0].(*tree.DBool)) - if isAdmin != tc.expAdmin { - t.Fatalf("expected %q admin %v, got %v", tc.user, tc.expAdmin, isAdmin) - } - }) - } -} - func TestSessionBoundInternalExecutor(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) diff --git a/pkg/sql/logictest/testdata/logic_test/crdb_internal b/pkg/sql/logictest/testdata/logic_test/crdb_internal index 8fc5b1e8086a..4ec854f40f32 100644 --- a/pkg/sql/logictest/testdata/logic_test/crdb_internal +++ b/pkg/sql/logictest/testdata/logic_test/crdb_internal @@ -1009,22 +1009,6 @@ user testuser query error crdb_internal.compact_engine_span\(\): insufficient privilege SELECT crdb_internal.compact_engine_span(1, 1, decode('c08989', 'hex'), decode('c0898a', 'hex')) -subtest builtin_is_admin - -user root - -query B -SELECT crdb_internal.is_admin() ----- -true - -user testuser - -query B -SELECT crdb_internal.is_admin() ----- -false - user root # Test the crdb_internal.create_type_statements table. diff --git a/pkg/sql/logictest/testdata/logic_test/role b/pkg/sql/logictest/testdata/logic_test/role index 6eecba64d0ef..6694322a5a3f 100644 --- a/pkg/sql/logictest/testdata/logic_test/role +++ b/pkg/sql/logictest/testdata/logic_test/role @@ -257,12 +257,6 @@ user root statement ok GRANT admin TO testuser -# Verify that is_admin reports the right value. -query B -SELECT crdb_internal.is_admin() ----- -true - # Dropping users/roles deletes all their memberships. query TTBOO colnames,rowsort SELECT * FROM system.role_members diff --git a/pkg/sql/sem/builtins/builtins.go b/pkg/sql/sem/builtins/builtins.go index 89c17861b632..f7326e7ccf14 100644 --- a/pkg/sql/sem/builtins/builtins.go +++ b/pkg/sql/sem/builtins/builtins.go @@ -6285,31 +6285,6 @@ SELECT }, ), - // Returns true iff the current user has admin role. - // Note: it would be a privacy leak to extend this to check arbitrary usernames. - "crdb_internal.is_admin": makeBuiltin( - tree.FunctionProperties{ - Category: builtinconstants.CategorySystemInfo, - DistsqlBlocklist: true, - }, - tree.Overload{ - Types: tree.ParamTypes{}, - ReturnType: tree.FixedReturnType(types.Bool), - Fn: func(ctx context.Context, evalCtx *eval.Context, _ tree.Datums) (tree.Datum, error) { - if evalCtx.SessionAccessor == nil { - return nil, errors.AssertionFailedf("session accessor not set") - } - isAdmin, err := evalCtx.SessionAccessor.HasAdminRole(ctx) - if err != nil { - return nil, err - } - return tree.MakeDBool(tree.DBool(isAdmin)), nil - }, - Info: "Retrieves the current user's admin status.", - Volatility: volatility.Stable, - }, - ), - "crdb_internal.assignment_cast": makeBuiltin( tree.FunctionProperties{ Category: builtinconstants.CategorySystemInfo, diff --git a/pkg/sql/sem/builtins/fixed_oids.go b/pkg/sql/sem/builtins/fixed_oids.go index 2691bf2c7190..a9cb1ae51e4e 100644 --- a/pkg/sql/sem/builtins/fixed_oids.go +++ b/pkg/sql/sem/builtins/fixed_oids.go @@ -1316,7 +1316,6 @@ var builtinOidsArray = []string{ 1336: `crdb_internal.num_inverted_index_entries(val: jsonb, version: int) -> int`, 1337: `crdb_internal.num_inverted_index_entries(val: string, version: int) -> int`, 1338: `crdb_internal.num_inverted_index_entries(val: anyelement[], version: int) -> int`, - 1339: `crdb_internal.is_admin() -> bool`, 1341: `crdb_internal.assignment_cast(val: anyelement, type: anyelement) -> anyelement`, 1342: `crdb_internal.round_decimal_values(val: decimal, scale: int) -> decimal`, 1343: `crdb_internal.round_decimal_values(val: decimal[], scale: int) -> decimal[]`, From 1fb0eacf5ffc618ffdb8a2e672fc9b04877af398 Mon Sep 17 00:00:00 2001 From: Faizan Qazi Date: Thu, 23 Nov 2023 21:05:12 +0000 Subject: [PATCH 3/3] catalog/lease: fix flake for TestTableCreationPushesTxnsInRecentPast Previously, TestTableCreationPushesTxnsInRecentPast could flake because we attempted to increase the uncertainty interval error by adding a delay on KV RPC calls to make the uncertainty interval larger. This would work based on the observed timestamp having a variance between nodes. This wasn't effective, and we could still have intermittent failures, because the variance was not large enough. To address this, this patch will increase the maximum offset on the HLC clock itself, which is used as the base value for computing the uncertainty interval (see computeIntervalForTxn). With this change, we expect a more deterministic reproduction of the txn getting pushed because of the uncertainty interval. Fixes: #114366 Release note: None --- pkg/sql/catalog/lease/BUILD.bazel | 1 - pkg/sql/catalog/lease/lease_test.go | 9 ++------- 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/pkg/sql/catalog/lease/BUILD.bazel b/pkg/sql/catalog/lease/BUILD.bazel index 012bda04fed9..4cb7bc452d3f 100644 --- a/pkg/sql/catalog/lease/BUILD.bazel +++ b/pkg/sql/catalog/lease/BUILD.bazel @@ -88,7 +88,6 @@ go_test( "//pkg/jobs/jobspb", "//pkg/keys", "//pkg/kv", - "//pkg/kv/kvclient/kvcoord", "//pkg/kv/kvpb", "//pkg/kv/kvserver", "//pkg/kv/kvserver/kvserverbase", diff --git a/pkg/sql/catalog/lease/lease_test.go b/pkg/sql/catalog/lease/lease_test.go index 1c2e98354c20..e17279f55e46 100644 --- a/pkg/sql/catalog/lease/lease_test.go +++ b/pkg/sql/catalog/lease/lease_test.go @@ -28,7 +28,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/jobs/jobspb" "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/kv" - "github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord" "github.com/cockroachdb/cockroach/pkg/kv/kvpb" "github.com/cockroachdb/cockroach/pkg/kv/kvserver" "github.com/cockroachdb/cockroach/pkg/kv/kvserver/kvserverbase" @@ -2028,12 +2027,8 @@ func TestTableCreationPushesTxnsInRecentPast(t *testing.T) { tc := serverutils.StartCluster(t, 3, base.TestClusterArgs{ ServerArgs: base.TestServerArgs{ Knobs: base.TestingKnobs{ - KVClient: &kvcoord.ClientTestingKnobs{ - // Intentionally add latency so that the uncertainty - // limit is higher to reduce the risk of this test flaking. - LatencyFunc: func(id roachpb.NodeID) (time.Duration, bool) { - return time.Millisecond * 100, true - }, + Store: &kvserver.StoreTestingKnobs{ + MaxOffset: time.Second, }, }, DefaultTestTenant: base.TestDoesNotWorkWithSecondaryTenantsButWeDontKnowWhyYet(109385),