Merge #92601 #92967

92601: catalog: break dependency on eval package r=postamar a=postamar This dependency was introduced because virtual tables need to have privilege descriptors synthesized for them instead of having them provided by the table descriptor. This commit refactors the privilege descriptor retrieval logic to break this dependency. No functionality should be altered. Fixes #88400. Release note: None 92967: kvserver: log replicate queue traces only when expensive logging enabled r=AlexTalks a=AlexTalks While we previously logged replicate queue traces on both errors and slow processing at INFO level, we will now only log the error message on INFO and include the trace when the `vmodule` level is increased. This allows us to decrease the verbosity on these error/slow replica processing cases by default, while still incorporating the traces if verbosity is incresed. Epic: None Release note: None Co-authored-by: Marius Posta <[email protected]> Co-authored-by: Alex Sarkesian <[email protected]>
cockroachdb · Dec 8, 2022 · 4bba5c6 · 4bba5c6
3 parents 48966e9 + 5de13ed + 7e78b4f
commit 4bba5c6
Show file tree

Hide file tree

Showing 38 changed files with 220 additions and 304 deletions.
diff --git a/pkg/kv/kvserver/replicate_queue.go b/pkg/kv/kvserver/replicate_queue.go
@@ -823,7 +823,7 @@ func (rq *replicateQueue) processOneChangeWithTracing(
 		exceededDuration := loggingThreshold > time.Duration(0) && processDuration > loggingThreshold
 
 		var traceOutput string
-		traceLoggingNeeded := err != nil || exceededDuration
+		traceLoggingNeeded := (err != nil || exceededDuration) && log.ExpensiveLogEnabled(ctx, 1)
 		if traceLoggingNeeded {
 			// If we have tracing spans from execChangeReplicasTxn, filter it from
 			// the recording so that we can render the traces to the log without it,

diff --git a/pkg/kv/kvserver/replicate_queue_test.go b/pkg/kv/kvserver/replicate_queue_test.go
@@ -795,6 +795,7 @@ func TestReplicateQueueDecommissioningNonVoters(t *testing.T) {
 func TestReplicateQueueTracingOnError(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	s := log.ScopeWithoutShowLogs(t)
+	_ = log.SetVModule("replicate_queue=2")
 	defer s.Close(t)
 
 	// NB: This test injects a fake failure during replica rebalancing, and we use

diff --git a/pkg/sql/authorization.go b/pkg/sql/authorization.go
@@ -33,7 +33,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/sql/privilege"
 	"github.com/cockroachdb/cockroach/pkg/sql/roleoption"
-	"github.com/cockroachdb/cockroach/pkg/sql/sem/eval"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
 	"github.com/cockroachdb/cockroach/pkg/sql/sessioninit"
@@ -77,14 +76,14 @@ type userRoleMembership map[username.SQLUsername]bool
 type AuthorizationAccessor interface {
 	// CheckPrivilege verifies that the user has `privilege` on `descriptor`.
 	CheckPrivilegeForUser(
-		ctx context.Context, privilegeObject catalog.PrivilegeObject, privilege privilege.Kind, user username.SQLUsername,
+		ctx context.Context, privilegeObject privilege.Object, privilege privilege.Kind, user username.SQLUsername,
 	) error
 
 	// CheckPrivilege verifies that the current user has `privilege` on `descriptor`.
-	CheckPrivilege(ctx context.Context, privilegeObject catalog.PrivilegeObject, privilege privilege.Kind) error
+	CheckPrivilege(ctx context.Context, privilegeObject privilege.Object, privilege privilege.Kind) error
 
 	// CheckAnyPrivilege returns nil if user has any privileges at all.
-	CheckAnyPrivilege(ctx context.Context, descriptor catalog.PrivilegeObject) error
+	CheckAnyPrivilege(ctx context.Context, descriptor privilege.Object) error
 
 	// UserHasAdminRole returns tuple of bool and error:
 	// (true, nil) means that the user has an admin role (i.e. root or node)
@@ -121,7 +120,7 @@ var _ AuthorizationAccessor = &planner{}
 // Requires a valid transaction to be open.
 func (p *planner) CheckPrivilegeForUser(
 	ctx context.Context,
-	privilegeObject catalog.PrivilegeObject,
+	privilegeObject privilege.Object,
 	privilegeKind privilege.Kind,
 	user username.SQLUsername,
 ) error {
@@ -156,7 +155,7 @@ func (p *planner) CheckPrivilegeForUser(
 	// permission check).
 	p.maybeAudit(privilegeObject, privilegeKind)
 
-	privs, err := privilegeObject.GetPrivilegeDescriptor(ctx, p)
+	privs, err := p.getPrivilegeDescriptor(ctx, privilegeObject)
 	if err != nil {
 		return err
 	}
@@ -167,7 +166,7 @@ func (p *planner) CheckPrivilegeForUser(
 	}
 
 	hasPriv, err := p.checkRolePredicate(ctx, user, func(role username.SQLUsername) (bool, error) {
-		isOwner, err := IsOwner(ctx, p, privilegeObject, role)
+		isOwner, err := isOwner(ctx, p, privilegeObject, role)
 		return isOwner || privs.CheckPrivilege(role, privilegeKind), err
 	})
 	if err != nil {
@@ -185,7 +184,7 @@ func (p *planner) CheckPrivilegeForUser(
 // it should be probably be called CheckPrivilegesOrOwnership and return
 // a better error.
 func (p *planner) CheckPrivilege(
-	ctx context.Context, object catalog.PrivilegeObject, privilege privilege.Kind,
+	ctx context.Context, object privilege.Object, privilege privilege.Kind,
 ) error {
 	return p.CheckPrivilegeForUser(ctx, object, privilege, p.User())
 }
@@ -197,7 +196,7 @@ func (p *planner) CheckPrivilege(
 func (p *planner) CheckGrantOptionsForUser(
 	ctx context.Context,
 	privs *catpb.PrivilegeDescriptor,
-	privilegeObject catalog.PrivilegeObject,
+	privilegeObject privilege.Object,
 	privList privilege.List,
 	user username.SQLUsername,
 	isGrant bool,
@@ -211,7 +210,7 @@ func (p *planner) CheckGrantOptionsForUser(
 	}
 
 	hasPriv, err := p.checkRolePredicate(ctx, user, func(role username.SQLUsername) (bool, error) {
-		isOwner, err := IsOwner(ctx, p, privilegeObject, role)
+		isOwner, err := isOwner(ctx, p, privilegeObject, role)
 		return privs.CheckGrantOptions(role, privList) || isOwner, err
 	})
 	if err != nil {
@@ -237,10 +236,10 @@ func (p *planner) CheckGrantOptionsForUser(
 	)
 }
 
-func getOwnerOfPrivilegeObject(
-	ctx context.Context, p eval.Planner, privilegeObject catalog.PrivilegeObject,
+func (p *planner) getOwnerOfPrivilegeObject(
+	ctx context.Context, privilegeObject privilege.Object,
 ) (username.SQLUsername, error) {
-	privDesc, err := privilegeObject.GetPrivilegeDescriptor(ctx, p)
+	privDesc, err := p.getPrivilegeDescriptor(ctx, privilegeObject)
 	if err != nil {
 		return username.SQLUsername{}, err
 	}
@@ -260,14 +259,11 @@ func getOwnerOfPrivilegeObject(
 	return owner, nil
 }
 
-// IsOwner returns if the role has ownership on the privilege object.
-func IsOwner(
-	ctx context.Context,
-	p eval.Planner,
-	privilegeObject catalog.PrivilegeObject,
-	role username.SQLUsername,
+// isOwner returns if the role has ownership on the privilege object.
+func isOwner(
+	ctx context.Context, p *planner, privilegeObject privilege.Object, role username.SQLUsername,
 ) (bool, error) {
-	owner, err := getOwnerOfPrivilegeObject(ctx, p, privilegeObject)
+	owner, err := p.getOwnerOfPrivilegeObject(ctx, privilegeObject)
 	if err != nil {
 		return false, err
 	}
@@ -279,12 +275,12 @@ func IsOwner(
 // TODO(richardjcai): SUPERUSER has implicit ownership.
 // We do not have SUPERUSER privilege yet but should we consider root a superuser?
 func (p *planner) HasOwnership(
-	ctx context.Context, privilegeObject catalog.PrivilegeObject,
+	ctx context.Context, privilegeObject privilege.Object,
 ) (bool, error) {
 	user := p.SessionData().User()
 
 	return p.checkRolePredicate(ctx, user, func(role username.SQLUsername) (bool, error) {
-		return IsOwner(ctx, p, privilegeObject, role)
+		return isOwner(ctx, p, privilegeObject, role)
 	})
 }
 
@@ -321,9 +317,7 @@ func (p *planner) checkRolePredicate(
 
 // CheckAnyPrivilege implements the AuthorizationAccessor interface.
 // Requires a valid transaction to be open.
-func (p *planner) CheckAnyPrivilege(
-	ctx context.Context, privilegeObject catalog.PrivilegeObject,
-) error {
+func (p *planner) CheckAnyPrivilege(ctx context.Context, privilegeObject privilege.Object) error {
 	// Verify that the txn is valid in any case, so that
 	// we don't get the risk to say "OK" to root requests
 	// with an invalid API usage.
@@ -338,7 +332,7 @@ func (p *planner) CheckAnyPrivilege(
 		return nil
 	}
 
-	privs, err := privilegeObject.GetPrivilegeDescriptor(ctx, p)
+	privs, err := p.getPrivilegeDescriptor(ctx, privilegeObject)
 	if err != nil {
 		return err
 	}
@@ -913,7 +907,7 @@ func (p *planner) HasViewActivityOrViewActivityRedactedRole(ctx context.Context)
 }
 
 func insufficientPrivilegeError(
-	user username.SQLUsername, kind privilege.Kind, object catalog.PrivilegeObject,
+	user username.SQLUsername, kind privilege.Kind, object privilege.Object,
 ) error {
 	// For consistency Postgres, we report the error message as not
 	// having a privilege on the object type "relation".

diff --git a/pkg/sql/catalog/BUILD.bazel b/pkg/sql/catalog/BUILD.bazel
@@ -12,7 +12,6 @@ go_library(
         "errors.go",
         "metadata.go",
         "post_deserialization_changes.go",
-        "privilege_object.go",
         "schema.go",
         "system_table.go",
         "table_col_map.go",
@@ -36,7 +35,6 @@ go_library(
         "//pkg/sql/pgwire/pgerror",
         "//pkg/sql/privilege",
         "//pkg/sql/schemachanger/scpb",
-        "//pkg/sql/sem/eval",
         "//pkg/sql/sem/tree",
         "//pkg/sql/types",
         "//pkg/util",
@@ -75,6 +73,9 @@ go_test(
 disallowed_imports_test(
     "catalog",
     disallow_cdeps = True,
+    disallowed_list = [
+        "//pkg/sql/sem/eval",
+    ],
 )
 
 get_x_data(name = "get_x_data")
diff --git a/pkg/sql/catalog/dbdesc/BUILD.bazel b/pkg/sql/catalog/dbdesc/BUILD.bazel
@@ -19,7 +19,6 @@ go_library(
         "//pkg/sql/catalog/multiregion",
         "//pkg/sql/privilege",
         "//pkg/sql/schemachanger/scpb",
-        "//pkg/sql/sem/eval",
         "//pkg/sql/sem/tree",
         "//pkg/util/hlc",
         "//pkg/util/iterutil",

diff --git a/pkg/sql/catalog/dbdesc/database_desc.go b/pkg/sql/catalog/dbdesc/database_desc.go
@@ -13,7 +13,6 @@
 package dbdesc
 
 import (
-	"context"
 	"fmt"
 
 	"github.com/cockroachdb/cockroach/pkg/keys"
@@ -24,7 +23,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/multiregion"
 	"github.com/cockroachdb/cockroach/pkg/sql/privilege"
 	"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scpb"
-	"github.com/cockroachdb/cockroach/pkg/sql/sem/eval"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/iterutil"
@@ -519,18 +517,11 @@ func (desc *Mutable) SetDeclarativeSchemaChangerState(state *scpb.DescriptorStat
 	desc.DeclarativeSchemaChangerState = state
 }
 
-// GetObjectType implements the PrivilegeObject interface.
+// GetObjectType implements the Object interface.
 func (desc *immutable) GetObjectType() privilege.ObjectType {
 	return privilege.Database
 }
 
-// GetPrivilegeDescriptor implements the PrivilegeObject interface.
-func (desc *immutable) GetPrivilegeDescriptor(
-	ctx context.Context, planner eval.Planner,
-) (*catpb.PrivilegeDescriptor, error) {
-	return desc.GetPrivileges(), nil
-}
-
 // SkipNamespace implements the descriptor interface.
 func (desc *immutable) SkipNamespace() bool {
 	return false

diff --git a/pkg/sql/catalog/descriptor.go b/pkg/sql/catalog/descriptor.go
@@ -19,6 +19,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
+	"github.com/cockroachdb/cockroach/pkg/sql/privilege"
 	"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/types"
@@ -159,7 +160,7 @@ type LeasableDescriptor interface {
 type Descriptor interface {
 	NameEntry
 	LeasableDescriptor
-	PrivilegeObject
+	privilege.Object
 
 	// GetPrivileges returns this descriptor's PrivilegeDescriptor, which
 	// describes the set of privileges that users have to use, modify, or delete

diff --git a/pkg/sql/catalog/funcdesc/BUILD.bazel b/pkg/sql/catalog/funcdesc/BUILD.bazel
@@ -21,7 +21,6 @@ go_library(
         "//pkg/sql/privilege",
         "//pkg/sql/schemachanger/scpb",
         "//pkg/sql/sem/catid",
-        "//pkg/sql/sem/eval",
         "//pkg/sql/sem/tree",
         "//pkg/sql/sem/volatility",
         "//pkg/sql/types",

diff --git a/pkg/sql/catalog/funcdesc/func_desc.go b/pkg/sql/catalog/funcdesc/func_desc.go
@@ -11,8 +11,6 @@
 package funcdesc
 
 import (
-	"context"
-
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catprivilege"
@@ -23,7 +21,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/privilege"
 	"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/catid"
-	"github.com/cockroachdb/cockroach/pkg/sql/sem/eval"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/volatility"
 	"github.com/cockroachdb/cockroach/pkg/sql/types"
@@ -511,18 +508,11 @@ func (desc *immutable) ToFuncObj() tree.FuncObj {
 	return ret
 }
 
-// GetObjectType implements the PrivilegeObject interface.
+// GetObjectType implements the Object interface.
 func (desc *immutable) GetObjectType() privilege.ObjectType {
 	return privilege.Function
 }
 
-// GetPrivilegeDescriptor implements the PrivilegeObject interface.
-func (desc *immutable) GetPrivilegeDescriptor(
-	ctx context.Context, planner eval.Planner,
-) (*catpb.PrivilegeDescriptor, error) {
-	return desc.GetPrivileges(), nil
-}
-
 // FuncDesc implements the catalog.FunctionDescriptor interface.
 func (desc *immutable) FuncDesc() *descpb.FunctionDescriptor {
 	return &desc.FunctionDescriptor

diff --git a/pkg/sql/catalog/privilege_object.go b/pkg/sql/catalog/privilege_object.go
diff --git a/pkg/sql/catalog/schemadesc/BUILD.bazel b/pkg/sql/catalog/schemadesc/BUILD.bazel
@@ -25,7 +25,6 @@ go_library(
         "//pkg/sql/schemachanger/scpb",
         "//pkg/sql/sem/catconstants",
         "//pkg/sql/sem/catid",
-        "//pkg/sql/sem/eval",
         "//pkg/sql/sem/tree",
         "//pkg/sql/types",
         "//pkg/util/hlc",

diff --git a/pkg/sql/catalog/schemadesc/public_schema_desc.go b/pkg/sql/catalog/schemadesc/public_schema_desc.go
@@ -11,14 +11,11 @@
 package schemadesc
 
 import (
-	"context"
-
 	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/privilege"
-	"github.com/cockroachdb/cockroach/pkg/sql/sem/eval"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 )
 
@@ -43,7 +40,7 @@ type public struct {
 }
 
 var _ catalog.SchemaDescriptor = public{}
-var _ catalog.PrivilegeObject = public{}
+var _ privilege.Object = public{}
 
 func (p public) GetParentID() descpb.ID { return descpb.InvalidID }
 func (p public) GetID() descpb.ID       { return keys.PublicSchemaID }
@@ -53,14 +50,7 @@ func (p public) GetPrivileges() *catpb.PrivilegeDescriptor {
 }
 func (p public) GetRawBytesInStorage() []byte { return nil }
 
-// GetPrivilegeDescriptor implements the PrivilegeObject interface.
-func (p public) GetPrivilegeDescriptor(
-	ctx context.Context, planner eval.Planner,
-) (*catpb.PrivilegeDescriptor, error) {
-	return p.GetPrivileges(), nil
-}
-
-// GetObjectType implements the PrivilegeObject interface.
+// GetObjectType implements the Object interface.
 func (p public) GetObjectType() privilege.ObjectType {
 	return privilege.Schema
 }