diff --git a/pkg/ccl/backupccl/BUILD.bazel b/pkg/ccl/backupccl/BUILD.bazel index c8ccc4aa7d22..db43b63f4a42 100644 --- a/pkg/ccl/backupccl/BUILD.bazel +++ b/pkg/ccl/backupccl/BUILD.bazel @@ -75,7 +75,6 @@ go_library( "//pkg/sql/catalog/colinfo", "//pkg/sql/catalog/dbdesc", "//pkg/sql/catalog/descbuilder", - "//pkg/sql/catalog/descidgen", "//pkg/sql/catalog/descpb", "//pkg/sql/catalog/descs", "//pkg/sql/catalog/ingesting", @@ -224,7 +223,6 @@ go_test( "//pkg/sql/catalog/bootstrap", "//pkg/sql/catalog/catalogkeys", "//pkg/sql/catalog/catprivilege", - "//pkg/sql/catalog/descidgen", "//pkg/sql/catalog/descpb", "//pkg/sql/catalog/descs", "//pkg/sql/catalog/desctestutils", diff --git a/pkg/ccl/backupccl/backup_test.go b/pkg/ccl/backupccl/backup_test.go index 3d5f1169da09..99933c755ded 100644 --- a/pkg/ccl/backupccl/backup_test.go +++ b/pkg/ccl/backupccl/backup_test.go @@ -73,7 +73,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql" "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/catalog/bootstrap" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/descidgen" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/desctestutils" "github.com/cockroachdb/cockroach/pkg/sql/catalog/systemschema" @@ -1633,7 +1632,8 @@ func TestBackupRestoreResume(t *testing.T) { sqlDB.Exec(t, `BACKUP DATABASE DATA TO $1`, restoreDir) sqlDB.Exec(t, `CREATE DATABASE restoredb`) restoreDatabaseID := sqlutils.QueryDatabaseID(t, sqlDB.DB, "restoredb") - restoreTableID, err := descidgen.GenerateUniqueDescID(ctx, tc.Servers[0].DB(), keys.SystemSQLCodec) + restoreTableID, err := tc.Server(0).ExecutorConfig().(sql.ExecutorConfig). + DescIDGenerator.GenerateUniqueDescID(ctx) if err != nil { t.Fatal(err) } diff --git a/pkg/ccl/backupccl/datadriven_test.go b/pkg/ccl/backupccl/datadriven_test.go index 4a4a79fbd804..d794cd45662f 100644 --- a/pkg/ccl/backupccl/datadriven_test.go +++ b/pkg/ccl/backupccl/datadriven_test.go @@ -28,7 +28,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql" "github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkeys" "github.com/cockroachdb/cockroach/pkg/sql/catalog/catprivilege" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/descidgen" "github.com/cockroachdb/cockroach/pkg/sql/catalog/systemschema" "github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc" "github.com/cockroachdb/cockroach/pkg/testutils" @@ -679,7 +678,8 @@ func TestDataDriven(t *testing.T) { codec := ds.servers[lastCreatedServer].ExecutorConfig().(sql.ExecutorConfig).Codec dummyTable := systemschema.SettingsTable err := db.Txn(ctx, func(ctx context.Context, txn *kv.Txn) error { - id, err := descidgen.GenerateUniqueDescID(ctx, db, codec) + id, err := ds.servers[lastCreatedServer].ExecutorConfig().(sql.ExecutorConfig). + DescIDGenerator.GenerateUniqueDescID(ctx) if err != nil { return err } diff --git a/pkg/ccl/backupccl/restore_job.go b/pkg/ccl/backupccl/restore_job.go index b7b56e76f61e..368af69940ad 100644 --- a/pkg/ccl/backupccl/restore_job.go +++ b/pkg/ccl/backupccl/restore_job.go @@ -36,7 +36,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkeys" "github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/dbdesc" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/descidgen" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descs" "github.com/cockroachdb/cockroach/pkg/sql/catalog/ingesting" @@ -1165,7 +1164,7 @@ func remapPublicSchemas( // if the database does not have a public schema backed by a descriptor // (meaning they were created before 22.1), we need to create a public // schema descriptor for it. - id, err := descidgen.GenerateUniqueDescID(ctx, p.ExecCfg().DB, p.ExecCfg().Codec) + id, err := p.ExecCfg().DescIDGenerator.GenerateUniqueDescID(ctx) if err != nil { return err } diff --git a/pkg/ccl/backupccl/restore_planning.go b/pkg/ccl/backupccl/restore_planning.go index ec04c2a2eb9a..5b23dd75e720 100644 --- a/pkg/ccl/backupccl/restore_planning.go +++ b/pkg/ccl/backupccl/restore_planning.go @@ -40,7 +40,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo" "github.com/cockroachdb/cockroach/pkg/sql/catalog/dbdesc" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descbuilder" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/descidgen" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descs" "github.com/cockroachdb/cockroach/pkg/sql/catalog/multiregion" @@ -153,7 +152,7 @@ func synthesizePGTempSchema( return errors.Newf("attempted to synthesize temp schema during RESTORE but found"+ " another schema already using the same schema key %s", schemaName) } - synthesizedSchemaID, err = descidgen.GenerateUniqueDescID(ctx, p.ExecCfg().DB, p.ExecCfg().Codec) + synthesizedSchemaID, err = p.ExecCfg().DescIDGenerator.GenerateUniqueDescID(ctx) if err != nil { return err } @@ -517,7 +516,7 @@ func allocateDescriptorRewrites( } } - tempSysDBID, err := descidgen.GenerateUniqueDescID(ctx, p.ExecCfg().DB, p.ExecCfg().Codec) + tempSysDBID, err := p.ExecCfg().DescIDGenerator.GenerateUniqueDescID(ctx) if err != nil { return nil, err } @@ -874,7 +873,7 @@ func allocateDescriptorRewrites( if descriptorCoverage == tree.AllDescriptors { newID = db.GetID() } else { - newID, err = descidgen.GenerateUniqueDescID(ctx, p.ExecCfg().DB, p.ExecCfg().Codec) + newID, err = p.ExecCfg().DescIDGenerator.GenerateUniqueDescID(ctx) if err != nil { return nil, err } @@ -945,7 +944,7 @@ func allocateDescriptorRewrites( // Generate new IDs for the schemas, tables, and types that need to be // remapped. for _, desc := range descriptorsToRemap { - id, err := descidgen.GenerateUniqueDescID(ctx, p.ExecCfg().DB, p.ExecCfg().Codec) + id, err := p.ExecCfg().DescIDGenerator.GenerateUniqueDescID(ctx) if err != nil { return nil, err } @@ -2218,7 +2217,9 @@ func planDatabaseModifiersForRestore( if defaultPrimaryRegion == "" { return nil, nil, nil } - if err := multiregionccl.CheckClusterSupportsMultiRegion(p.ExecCfg()); err != nil { + if err := multiregionccl.CheckClusterSupportsMultiRegion( + p.ExecCfg().Settings, p.ExecCfg().NodeInfo.LogicalClusterID(), p.ExecCfg().Organization(), + ); err != nil { return nil, nil, errors.WithHintf( err, "try disabling the default PRIMARY REGION by using RESET CLUSTER SETTING %s", diff --git a/pkg/ccl/multiregionccl/BUILD.bazel b/pkg/ccl/multiregionccl/BUILD.bazel index 037570b467b9..421901eb3126 100644 --- a/pkg/ccl/multiregionccl/BUILD.bazel +++ b/pkg/ccl/multiregionccl/BUILD.bazel @@ -8,15 +8,17 @@ go_library( visibility = ["//visibility:public"], deps = [ "//pkg/ccl/utilccl", + "//pkg/settings/cluster", "//pkg/sql", "//pkg/sql/catalog/catpb", - "//pkg/sql/catalog/descidgen", "//pkg/sql/catalog/descpb", "//pkg/sql/catalog/multiregion", "//pkg/sql/catalog/typedesc", "//pkg/sql/pgwire/pgcode", "//pkg/sql/pgwire/pgerror", + "//pkg/sql/sem/eval", "//pkg/sql/sem/tree", + "//pkg/util/uuid", ], ) diff --git a/pkg/ccl/multiregionccl/multiregion.go b/pkg/ccl/multiregionccl/multiregion.go index c7517965bfd2..9a5ce0a8b24f 100644 --- a/pkg/ccl/multiregionccl/multiregion.go +++ b/pkg/ccl/multiregionccl/multiregion.go @@ -13,15 +13,17 @@ import ( "sort" "github.com/cockroachdb/cockroach/pkg/ccl/utilccl" + "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/sql" "github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/descidgen" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/multiregion" "github.com/cockroachdb/cockroach/pkg/sql/catalog/typedesc" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" + "github.com/cockroachdb/cockroach/pkg/sql/sem/eval" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" + "github.com/cockroachdb/cockroach/pkg/util/uuid" ) func init() { @@ -31,7 +33,10 @@ func init() { func initializeMultiRegionMetadata( ctx context.Context, - execCfg *sql.ExecutorConfig, + descIDGenerator eval.DescIDGenerator, + settings *cluster.Settings, + clusterID uuid.UUID, + clusterOrganization string, liveRegions sql.LiveClusterRegions, goal tree.SurvivalGoal, primaryRegion catpb.RegionName, @@ -39,7 +44,9 @@ func initializeMultiRegionMetadata( dataPlacement tree.DataPlacement, secondaryRegion catpb.RegionName, ) (*multiregion.RegionConfig, error) { - if err := CheckClusterSupportsMultiRegion(execCfg); err != nil { + if err := CheckClusterSupportsMultiRegion( + settings, clusterID, clusterOrganization, + ); err != nil { return nil, err } @@ -94,7 +101,7 @@ func initializeMultiRegionMetadata( // Generate a unique ID for the multi-region enum type descriptor here as // well. - regionEnumID, err := descidgen.GenerateUniqueDescID(ctx, execCfg.DB, execCfg.Codec) + regionEnumID, err := descIDGenerator.GenerateUniqueDescID(ctx) if err != nil { return nil, err } @@ -117,11 +124,13 @@ func initializeMultiRegionMetadata( // CheckClusterSupportsMultiRegion returns whether the current cluster supports // multi-region features. -func CheckClusterSupportsMultiRegion(execCfg *sql.ExecutorConfig) error { +func CheckClusterSupportsMultiRegion( + settings *cluster.Settings, clusterID uuid.UUID, organization string, +) error { return utilccl.CheckEnterpriseEnabled( - execCfg.Settings, - execCfg.NodeInfo.LogicalClusterID(), - execCfg.Organization(), + settings, + clusterID, + organization, "multi-region features", ) } diff --git a/pkg/server/BUILD.bazel b/pkg/server/BUILD.bazel index 024e90a4f24a..76af2e90ba51 100644 --- a/pkg/server/BUILD.bazel +++ b/pkg/server/BUILD.bazel @@ -158,6 +158,7 @@ go_library( "//pkg/sql/catalog/bootstrap", "//pkg/sql/catalog/catalogkeys", "//pkg/sql/catalog/colinfo", + "//pkg/sql/catalog/descidgen", "//pkg/sql/catalog/descpb", "//pkg/sql/catalog/descs", "//pkg/sql/catalog/hydratedtables", diff --git a/pkg/server/server_sql.go b/pkg/server/server_sql.go index 7398490c3c5e..0a7b8dfacaa6 100644 --- a/pkg/server/server_sql.go +++ b/pkg/server/server_sql.go @@ -59,6 +59,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/spanconfig/spanconfigsqlwatcher" "github.com/cockroachdb/cockroach/pkg/sql" "github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkeys" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/descidgen" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descs" "github.com/cockroachdb/cockroach/pkg/sql/catalog/hydratedtables" "github.com/cockroachdb/cockroach/pkg/sql/catalog/lease" @@ -825,6 +826,7 @@ func newSQLServer(ctx context.Context, cfg sqlServerArgs) (*SQLServer, error) { SystemTableIDResolver: descs.MakeSystemTableIDResolver(collectionFactory, cfg.circularInternalExecutor, cfg.db), ConsistencyChecker: consistencychecker.NewConsistencyChecker(cfg.db), RangeProber: rangeprober.NewRangeProber(cfg.db), + DescIDGenerator: descidgen.NewGenerator(codec, cfg.db), } if sqlSchemaChangerTestingKnobs := cfg.TestingKnobs.SQLSchemaChanger; sqlSchemaChangerTestingKnobs != nil { diff --git a/pkg/sql/authorization.go b/pkg/sql/authorization.go index 933a868ac473..eff1509a925e 100644 --- a/pkg/sql/authorization.go +++ b/pkg/sql/authorization.go @@ -750,7 +750,7 @@ func (p *planner) checkCanAlterToNewOwner( ctx context.Context, desc catalog.MutableDescriptor, newOwner username.SQLUsername, ) error { // Make sure the newOwner exists. - roleExists, err := RoleExists(ctx, p.ExecCfg(), p.Txn(), newOwner) + roleExists, err := RoleExists(ctx, p.ExecCfg().InternalExecutor, p.Txn(), newOwner) if err != nil { return err } diff --git a/pkg/sql/catalog/descidgen/BUILD.bazel b/pkg/sql/catalog/descidgen/BUILD.bazel index 864320d02880..88a1757a71ce 100644 --- a/pkg/sql/catalog/descidgen/BUILD.bazel +++ b/pkg/sql/catalog/descidgen/BUILD.bazel @@ -10,6 +10,7 @@ go_library( "//pkg/keys", "//pkg/kv", "//pkg/sql/catalog/descpb", + "//pkg/sql/sem/catid", ], ) diff --git a/pkg/sql/catalog/descidgen/generate_id.go b/pkg/sql/catalog/descidgen/generate_id.go index 944c3a04f8cc..4d68b08fa1e2 100644 --- a/pkg/sql/catalog/descidgen/generate_id.go +++ b/pkg/sql/catalog/descidgen/generate_id.go @@ -16,27 +16,66 @@ import ( "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/kv" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" + "github.com/cockroachdb/cockroach/pkg/sql/sem/catid" ) +// Generator implements DescIDGenerator. +type Generator struct { + db *kv.DB + codec keys.SQLCodec +} + +// NewGenerator constructs a new Generator. +func NewGenerator(codec keys.SQLCodec, db *kv.DB) *Generator { + return &Generator{ + codec: codec, + db: db, + } +} + // GenerateUniqueDescID returns the next available Descriptor ID and increments -// the counter. The incrementing is non-transactional, and the counter could be -// incremented multiple times because of retries. -// -// TODO(postamar): define an interface for ID generation, -// both the db and codec dependencies are singletons anyway. -func GenerateUniqueDescID(ctx context.Context, db *kv.DB, codec keys.SQLCodec) (descpb.ID, error) { +// the counter. +func (g *Generator) GenerateUniqueDescID(ctx context.Context) (catid.DescID, error) { // Increment unique descriptor counter. - newVal, err := kv.IncrementValRetryable(ctx, db, codec.DescIDSequenceKey(), 1) + newVal, err := kv.IncrementValRetryable(ctx, g.db, g.codec.DescIDSequenceKey(), 1) if err != nil { return descpb.InvalidID, err } return descpb.ID(newVal - 1), nil } +// TransactionalGenerator implements eval.DescIDGenerator using a transaction. +type TransactionalGenerator struct { + codec keys.SQLCodec + txn *kv.Txn +} + +// GenerateUniqueDescID returns the next available Descriptor ID and increments +// the counter. +func (t *TransactionalGenerator) GenerateUniqueDescID(ctx context.Context) (catid.DescID, error) { + got, err := t.txn.Inc(ctx, t.codec.DescIDSequenceKey(), 1) + if err != nil { + return 0, err + } + return catid.DescID(got.ValueInt() - 1), nil +} + +// NewTransactionalGenerator constructs a transactional DescIDGenerator. +func NewTransactionalGenerator(codec keys.SQLCodec, txn *kv.Txn) *TransactionalGenerator { + return &TransactionalGenerator{ + codec: codec, + txn: txn, + } +} + // PeekNextUniqueDescID returns the next as-of-yet unassigned unique descriptor // ID in the sequence. Note that this value is _not_ guaranteed to be the same // as that returned by a subsequent call to GenerateUniqueDescID. It will, // however, be a lower bound on it. +// +// Note that this function must not be used during the execution of a +// transaction which uses a TransactionalGenerator. Otherwise, deadlocks +// may occur. func PeekNextUniqueDescID(ctx context.Context, db *kv.DB, codec keys.SQLCodec) (descpb.ID, error) { v, err := db.Get(ctx, codec.DescIDSequenceKey()) if err != nil { diff --git a/pkg/sql/conn_executor.go b/pkg/sql/conn_executor.go index bbe1e7c55c6b..b6c2e27a4871 100644 --- a/pkg/sql/conn_executor.go +++ b/pkg/sql/conn_executor.go @@ -31,6 +31,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/server/telemetry" "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/descidgen" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descs" "github.com/cockroachdb/cockroach/pkg/sql/clusterunique" @@ -2715,6 +2716,7 @@ func (ex *connExecutor) initEvalCtx(ctx context.Context, evalCtx *extendedEvalCo StmtDiagnosticsRequestInserter: ex.server.cfg.StmtDiagnosticsRecorder.InsertRequest, CatalogBuiltins: &p.evalCatalogBuiltins, QueryCancelKey: ex.queryCancelKey, + DescIDGenerator: ex.getDescIDGenerator(), }, Tracing: &ex.sessionTracing, MemMetrics: &ex.memMetrics, @@ -3272,6 +3274,14 @@ func (ex *connExecutor) runPreCommitStages(ctx context.Context) error { return nil } +func (ex *connExecutor) getDescIDGenerator() eval.DescIDGenerator { + if ex.server.cfg.TestingKnobs.UseTransactionalDescIDGenerator && + ex.state.mu.txn != nil { + return descidgen.NewTransactionalGenerator(ex.server.cfg.Codec, ex.state.mu.txn) + } + return ex.server.cfg.DescIDGenerator +} + // StatementCounters groups metrics for counting different types of // statements. type StatementCounters struct { diff --git a/pkg/sql/create_function.go b/pkg/sql/create_function.go index c5a1ad912a91..66197e93e1f6 100644 --- a/pkg/sql/create_function.go +++ b/pkg/sql/create_function.go @@ -19,7 +19,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/catprivilege" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/descidgen" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/funcdesc" "github.com/cockroachdb/cockroach/pkg/sql/catalog/schemadesc" @@ -239,7 +238,7 @@ func (n *createFunctionNode) getMutableFuncDesc(params runParams) (*funcdesc.Mut // (3) add validation that if existing function is referenced then it cannot be replace. // (4) add `if` branch so that we only create new descriptor when it's not a replace. - funcDescID, err := descidgen.GenerateUniqueDescID(params.ctx, params.p.ExecCfg().DB, params.p.ExecCfg().Codec) + funcDescID, err := params.EvalContext().DescIDGenerator.GenerateUniqueDescID(params.ctx) if err != nil { return nil, err } diff --git a/pkg/sql/create_schema.go b/pkg/sql/create_schema.go index 0be9241b2f3d..0071f7a9df64 100644 --- a/pkg/sql/create_schema.go +++ b/pkg/sql/create_schema.go @@ -21,7 +21,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkeys" "github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/catprivilege" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/descidgen" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descs" "github.com/cockroachdb/cockroach/pkg/sql/catalog/schemadesc" @@ -29,10 +28,12 @@ import ( "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/sem/eval" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" "github.com/cockroachdb/cockroach/pkg/sql/sqlerrors" "github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry" + "github.com/cockroachdb/cockroach/pkg/sql/sqlutil" "github.com/cockroachdb/cockroach/pkg/util/log/eventpb" ) @@ -51,7 +52,8 @@ func CreateUserDefinedSchemaDescriptor( n *tree.CreateSchema, txn *kv.Txn, descriptors *descs.Collection, - execCfg *ExecutorConfig, + ie sqlutil.InternalExecutor, + descIDGenerator eval.DescIDGenerator, db catalog.DatabaseDescriptor, allocateID bool, ) (*schemadesc.Mutable, *catpb.PrivilegeDescriptor, error) { @@ -108,7 +110,7 @@ func CreateUserDefinedSchemaDescriptor( owner := user if !n.AuthRole.Undefined() { - exists, err := RoleExists(ctx, execCfg, txn, authRole) + exists, err := RoleExists(ctx, ie, txn, authRole) if err != nil { return nil, nil, err } @@ -119,7 +121,9 @@ func CreateUserDefinedSchemaDescriptor( owner = authRole } - desc, privs, err := CreateSchemaDescriptorWithPrivileges(ctx, execCfg.DB, execCfg.Codec, db, schemaName, user, owner, allocateID) + desc, privs, err := CreateSchemaDescriptorWithPrivileges( + ctx, descIDGenerator, db, schemaName, user, owner, allocateID, + ) if err != nil { return nil, nil, err } @@ -131,8 +135,7 @@ func CreateUserDefinedSchemaDescriptor( // the provided name and privileges. func CreateSchemaDescriptorWithPrivileges( ctx context.Context, - kvDB *kv.DB, - codec keys.SQLCodec, + descIDGenerator eval.DescIDGenerator, db catalog.DatabaseDescriptor, schemaName string, user, owner username.SQLUsername, @@ -142,7 +145,7 @@ func CreateSchemaDescriptorWithPrivileges( var id descpb.ID var err error if allocateID { - id, err = descidgen.GenerateUniqueDescID(ctx, kvDB, codec) + id, err = descIDGenerator.GenerateUniqueDescID(ctx) if err != nil { return nil, nil, err } @@ -207,8 +210,11 @@ func (p *planner) createUserDefinedSchema(params runParams, n *tree.CreateSchema return err } - desc, privs, err := CreateUserDefinedSchemaDescriptor(params.ctx, params.SessionData(), n, - p.Txn(), p.Descriptors(), p.ExecCfg(), db, true /* allocateID */) + desc, privs, err := CreateUserDefinedSchemaDescriptor( + params.ctx, params.SessionData(), n, p.Txn(), p.Descriptors(), + p.ExecCfg().InternalExecutor, p.extendedEvalCtx.DescIDGenerator, + db, true, /* allocateID */ + ) if err != nil { return err } diff --git a/pkg/sql/create_sequence.go b/pkg/sql/create_sequence.go index 8db6f5f27cd6..a22683c55b4e 100644 --- a/pkg/sql/create_sequence.go +++ b/pkg/sql/create_sequence.go @@ -23,7 +23,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/catprivilege" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descbuilder" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/descidgen" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc" "github.com/cockroachdb/cockroach/pkg/sql/privilege" @@ -105,7 +104,7 @@ func doCreateSequence( opts tree.SequenceOptions, jobDesc string, ) (*tabledesc.Mutable, error) { - id, err := descidgen.GenerateUniqueDescID(ctx, p.ExecCfg().DB, p.ExecCfg().Codec) + id, err := p.EvalContext().DescIDGenerator.GenerateUniqueDescID(ctx) if err != nil { return nil, err } diff --git a/pkg/sql/create_table.go b/pkg/sql/create_table.go index 368afc222d8b..e556ae2df166 100644 --- a/pkg/sql/create_table.go +++ b/pkg/sql/create_table.go @@ -33,7 +33,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/catprivilege" "github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/descidgen" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/multiregion" "github.com/cockroachdb/cockroach/pkg/sql/catalog/resolver" @@ -352,7 +351,8 @@ func (n *createTableNode) startExec(params runParams) error { } } - id, err := descidgen.GenerateUniqueDescID(params.ctx, params.p.ExecCfg().DB, params.p.ExecCfg().Codec) + id, err := params.extendedEvalCtx.DescIDGenerator. + GenerateUniqueDescID(params.ctx) if err != nil { return err } diff --git a/pkg/sql/create_type.go b/pkg/sql/create_type.go index c3d5ee04c464..3c53f5a919a6 100644 --- a/pkg/sql/create_type.go +++ b/pkg/sql/create_type.go @@ -19,7 +19,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkeys" "github.com/cockroachdb/cockroach/pkg/sql/catalog/catprivilege" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/descidgen" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descs" "github.com/cockroachdb/cockroach/pkg/sql/catalog/typedesc" @@ -268,7 +267,7 @@ func (p *planner) createArrayType( arrayTypeKey := catalogkeys.MakeObjectNameKey(params.ExecCfg().Codec, db.GetID(), schemaID, arrayTypeName) // Generate the stable ID for the array type. - id, err := descidgen.GenerateUniqueDescID(params.ctx, params.ExecCfg().DB, params.ExecCfg().Codec) + id, err := params.EvalContext().DescIDGenerator.GenerateUniqueDescID(params.ctx) if err != nil { return 0, err } @@ -300,9 +299,7 @@ func (p *planner) createArrayType( func (p *planner) createUserDefinedEnum(params runParams, n *createTypeNode) error { // Generate a stable ID for the new type. - id, err := descidgen.GenerateUniqueDescID( - params.ctx, params.ExecCfg().DB, params.ExecCfg().Codec, - ) + id, err := params.EvalContext().DescIDGenerator.GenerateUniqueDescID(params.ctx) if err != nil { return err } diff --git a/pkg/sql/create_view.go b/pkg/sql/create_view.go index 6b1f4f288555..d7dac30cdf41 100644 --- a/pkg/sql/create_view.go +++ b/pkg/sql/create_view.go @@ -23,7 +23,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/catprivilege" "github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/descidgen" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/resolver" "github.com/cockroachdb/cockroach/pkg/sql/catalog/seqexpr" @@ -204,7 +203,8 @@ func (n *createViewNode) startExec(params runParams) error { } } else { // If we aren't replacing anything, make a new table descriptor. - id, err := descidgen.GenerateUniqueDescID(params.ctx, params.p.ExecCfg().DB, params.p.ExecCfg().Codec) + id, err := params.EvalContext().DescIDGenerator. + GenerateUniqueDescID(params.ctx) if err != nil { return err } diff --git a/pkg/sql/descriptor.go b/pkg/sql/descriptor.go index 96e752e8e6df..3505312e3384 100644 --- a/pkg/sql/descriptor.go +++ b/pkg/sql/descriptor.go @@ -20,11 +20,11 @@ import ( "github.com/cockroachdb/cockroach/pkg/security/username" "github.com/cockroachdb/cockroach/pkg/server/telemetry" "github.com/cockroachdb/cockroach/pkg/settings" + "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkeys" "github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/dbdesc" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/descidgen" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/funcdesc" "github.com/cockroachdb/cockroach/pkg/sql/catalog/multiregion" @@ -35,10 +35,12 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgnotice" + "github.com/cockroachdb/cockroach/pkg/sql/sem/eval" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sqlerrors" "github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry" "github.com/cockroachdb/cockroach/pkg/util/log" + "github.com/cockroachdb/cockroach/pkg/util/uuid" "github.com/cockroachdb/errors" ) @@ -92,7 +94,7 @@ func (p *planner) createDatabase( return nil, false, err } - id, err := descidgen.GenerateUniqueDescID(ctx, p.ExecCfg().DB, p.ExecCfg().Codec) + id, err := p.extendedEvalCtx.DescIDGenerator.GenerateUniqueDescID(ctx) if err != nil { return nil, false, err } @@ -174,7 +176,7 @@ func (p *planner) maybeCreatePublicSchemaWithDescriptor( return descpb.InvalidID, nil } - publicSchemaID, err := descidgen.GenerateUniqueDescID(ctx, p.ExecCfg().DB, p.ExecCfg().Codec) + publicSchemaID, err := p.EvalContext().DescIDGenerator.GenerateUniqueDescID(ctx) if err != nil { return descpb.InvalidID, err } @@ -352,7 +354,10 @@ func (p *planner) checkRegionIsCurrentlyActive(ctx context.Context, region catpb // CCL-licensed multi-region initialization code. var InitializeMultiRegionMetadataCCL = func( ctx context.Context, - execCfg *ExecutorConfig, + descIDGenerator eval.DescIDGenerator, + settings *cluster.Settings, + clusterID uuid.UUID, + clusterOrganization string, liveClusterRegions LiveClusterRegions, survivalGoal tree.SurvivalGoal, primaryRegion catpb.RegionName, @@ -443,7 +448,10 @@ func (p *planner) maybeInitializeMultiRegionMetadata( regionConfig, err := InitializeMultiRegionMetadataCCL( ctx, - p.ExecCfg(), + p.EvalContext().DescIDGenerator, + p.EvalContext().Settings, + p.ExecCfg().NodeInfo.LogicalClusterID(), + p.ExecCfg().Organization(), liveRegions, survivalGoal, catpb.RegionName(primaryRegion), diff --git a/pkg/sql/exec_util.go b/pkg/sql/exec_util.go index 3fad0443cbeb..46b961dca2ce 100644 --- a/pkg/sql/exec_util.go +++ b/pkg/sql/exec_util.go @@ -1307,6 +1307,9 @@ type ExecutorConfig struct { // RangeProber is used in calls to crdb_internal.probe_ranges. RangeProber eval.RangeProber + + // DescIDGenerator generates unique descriptor IDs. + DescIDGenerator eval.DescIDGenerator } // UpdateVersionSystemSettingHook provides a callback that allows us @@ -1467,6 +1470,11 @@ type ExecutorTestingKnobs struct { // OnRecordTxnFinish, if set, will be called as we record a transaction // finishing. OnRecordTxnFinish func(isInternal bool, phaseTimes *sessionphase.Times, stmt string) + + // UseTransactionDescIDGenerator is used to force descriptor ID generation + // to use a transaction, and, in doing so, more deterministically allocate + // descriptor IDs at the cost of decreased parallelism. + UseTransactionalDescIDGenerator bool } // PGWireTestingKnobs contains knobs for the pgwire module. diff --git a/pkg/sql/importer/import_job.go b/pkg/sql/importer/import_job.go index 5d0080cca046..e86faa5ffce7 100644 --- a/pkg/sql/importer/import_job.go +++ b/pkg/sql/importer/import_job.go @@ -522,7 +522,7 @@ func prepareNewTablesForIngestion( } seqVals := make(map[descpb.ID]int64, len(importTables)) for _, tableDesc := range importTables { - id, err := descidgen.GenerateUniqueDescID(ctx, p.ExecCfg().DB, p.ExecCfg().Codec) + id, err := p.ExecCfg().DescIDGenerator.GenerateUniqueDescID(ctx) if err != nil { return nil, err } @@ -637,7 +637,7 @@ func (r *importResumer) prepareSchemasForIngestion( // Verification steps have passed, generate a new schema ID. We do this // last because we want to avoid calling GenerateUniqueDescID if there's // any kind of error in the prior stages of import. - id, err := descidgen.GenerateUniqueDescID(ctx, p.ExecCfg().DB, p.ExecCfg().Codec) + id, err := p.ExecCfg().DescIDGenerator.GenerateUniqueDescID(ctx) if err != nil { return nil, err } diff --git a/pkg/sql/importer/read_import_pgdump.go b/pkg/sql/importer/read_import_pgdump.go index bcb7dedd4af7..d5c7d6dfe06f 100644 --- a/pkg/sql/importer/read_import_pgdump.go +++ b/pkg/sql/importer/read_import_pgdump.go @@ -251,7 +251,8 @@ func createPostgresSchemas( dbDesc catalog.DatabaseDescriptor, schema *tree.CreateSchema, ) (*schemadesc.Mutable, error) { desc, _, err := sql.CreateUserDefinedSchemaDescriptor( - ctx, sessionData, schema, txn, descriptors, execCfg, dbDesc, false, /* allocateID */ + ctx, sessionData, schema, txn, descriptors, execCfg.InternalExecutor, + execCfg.DescIDGenerator, dbDesc, false, /* allocateID */ ) if err != nil { return nil, err diff --git a/pkg/sql/logictest/logic.go b/pkg/sql/logictest/logic.go index 7a41b3e66753..38b2d051047a 100644 --- a/pkg/sql/logictest/logic.go +++ b/pkg/sql/logictest/logic.go @@ -1171,7 +1171,8 @@ func (t *logicTest) newCluster( ForceProductionValues: serverArgs.ForceProductionValues, }, SQLExecutor: &sql.ExecutorTestingKnobs{ - DeterministicExplain: true, + DeterministicExplain: true, + UseTransactionalDescIDGenerator: true, }, SQLStatsKnobs: &sqlstats.TestingKnobs{ AOSTClause: "AS OF SYSTEM TIME '-1us'", diff --git a/pkg/sql/mvcc_backfiller_test.go b/pkg/sql/mvcc_backfiller_test.go index 0a425b569b69..1187c081f4b6 100644 --- a/pkg/sql/mvcc_backfiller_test.go +++ b/pkg/sql/mvcc_backfiller_test.go @@ -57,6 +57,7 @@ func TestIndexBackfillMergeRetry(t *testing.T) { defer log.Scope(t).Close(t) skip.UnderStressRace(t, "TODO(ssd) test times outs under race") + skip.UnderRace(t, "TODO(ssd) test times outs under race") params, _ := tests.CreateTestServerParams() diff --git a/pkg/sql/opt_catalog.go b/pkg/sql/opt_catalog.go index 5f10d787d085..14997058d863 100644 --- a/pkg/sql/opt_catalog.go +++ b/pkg/sql/opt_catalog.go @@ -449,7 +449,7 @@ func (oc *optCatalog) fullyQualifiedNameWithTxn( // RoleExists is part of the cat.Catalog interface. func (oc *optCatalog) RoleExists(ctx context.Context, role username.SQLUsername) (bool, error) { - return RoleExists(ctx, oc.planner.ExecCfg(), oc.planner.Txn(), role) + return RoleExists(ctx, oc.planner.ExecCfg().InternalExecutor, oc.planner.Txn(), role) } // dataSourceForDesc returns a data source wrapper for the given descriptor. diff --git a/pkg/sql/reassign_owned_by.go b/pkg/sql/reassign_owned_by.go index 3405d26131dd..52cb97fbcb79 100644 --- a/pkg/sql/reassign_owned_by.go +++ b/pkg/sql/reassign_owned_by.go @@ -54,7 +54,7 @@ func (p *planner) ReassignOwnedBy(ctx context.Context, n *tree.ReassignOwnedBy) // is a member of old roles and new roles and has CREATE privilege. // Postgres first checks if the role exists before checking privileges. for _, oldRole := range normalizedOldRoles { - roleExists, err := RoleExists(ctx, p.ExecCfg(), p.Txn(), oldRole) + roleExists, err := RoleExists(ctx, p.ExecCfg().InternalExecutor, p.Txn(), oldRole) if err != nil { return nil, err } @@ -68,7 +68,7 @@ func (p *planner) ReassignOwnedBy(ctx context.Context, n *tree.ReassignOwnedBy) if err != nil { return nil, err } - roleExists, err := RoleExists(ctx, p.ExecCfg(), p.Txn(), newRole) + roleExists, err := RoleExists(ctx, p.ExecCfg().InternalExecutor, p.Txn(), newRole) if !roleExists { return nil, pgerror.Newf(pgcode.UndefinedObject, "role/user %q does not exist", newRole) } diff --git a/pkg/sql/reparent_database.go b/pkg/sql/reparent_database.go index e25e58171681..c4709f7d4538 100644 --- a/pkg/sql/reparent_database.go +++ b/pkg/sql/reparent_database.go @@ -17,7 +17,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkeys" "github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/dbdesc" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/descidgen" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/resolver" "github.com/cockroachdb/cockroach/pkg/sql/catalog/schemadesc" @@ -109,7 +108,7 @@ func (n *reparentDatabaseNode) startExec(params runParams) error { ctx, p, codec := params.ctx, params.p, params.ExecCfg().Codec // Make a new schema corresponding to the target db. - id, err := descidgen.GenerateUniqueDescID(ctx, p.ExecCfg().DB, codec) + id, err := p.EvalContext().DescIDGenerator.GenerateUniqueDescID(ctx) if err != nil { return err } diff --git a/pkg/sql/sem/eval/context.go b/pkg/sql/sem/eval/context.go index 6e046b25c081..afb7694ed9a4 100644 --- a/pkg/sql/sem/eval/context.go +++ b/pkg/sql/sem/eval/context.go @@ -25,6 +25,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgwirecancel" + "github.com/cockroachdb/cockroach/pkg/sql/sem/catid" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" "github.com/cockroachdb/cockroach/pkg/sql/sessiondatapb" @@ -216,6 +217,13 @@ type Context struct { // QueryCancelKey is the key used by the pgwire protocol to cancel the // query currently running in this session. QueryCancelKey pgwirecancel.BackendKeyData + + DescIDGenerator DescIDGenerator +} + +// DescIDGenerator generates unique descriptor IDs. +type DescIDGenerator interface { + GenerateUniqueDescID(ctx context.Context) (catid.DescID, error) } var _ tree.ParseTimeContext = &Context{} diff --git a/pkg/sql/temporary_schema.go b/pkg/sql/temporary_schema.go index 99f69e03de26..8d33f94fc80b 100644 --- a/pkg/sql/temporary_schema.go +++ b/pkg/sql/temporary_schema.go @@ -27,7 +27,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkeys" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/descidgen" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descs" "github.com/cockroachdb/cockroach/pkg/sql/catalog/resolver" @@ -114,7 +113,7 @@ func (p *planner) getOrCreateTemporarySchema( sKey := catalogkeys.NewNameKeyComponents(db.GetID(), keys.RootNamespaceID, tempSchemaName) // The temporary schema has not been created yet. - id, err := descidgen.GenerateUniqueDescID(ctx, p.ExecCfg().DB, p.ExecCfg().Codec) + id, err := p.EvalContext().DescIDGenerator.GenerateUniqueDescID(ctx) if err != nil { return nil, err } diff --git a/pkg/sql/user.go b/pkg/sql/user.go index a1b166578ea0..0cd96fa86e90 100644 --- a/pkg/sql/user.go +++ b/pkg/sql/user.go @@ -436,15 +436,15 @@ func (p *planner) GetAllRoles(ctx context.Context) (map[username.SQLUsername]boo // RoleExists returns true if the role exists. func (p *planner) RoleExists(ctx context.Context, role username.SQLUsername) (bool, error) { - return RoleExists(ctx, p.ExecCfg(), p.Txn(), role) + return RoleExists(ctx, p.execCfg.InternalExecutor, p.Txn(), role) } // RoleExists returns true if the role exists. func RoleExists( - ctx context.Context, execCfg *ExecutorConfig, txn *kv.Txn, role username.SQLUsername, + ctx context.Context, ie sqlutil.InternalExecutor, txn *kv.Txn, role username.SQLUsername, ) (bool, error) { query := `SELECT username FROM system.users WHERE username = $1` - row, err := execCfg.InternalExecutor.QueryRowEx( + row, err := ie.QueryRowEx( ctx, "read-users", txn, sessiondata.InternalExecutorOverride{User: username.RootUserName()}, query, role, diff --git a/pkg/upgrade/upgrades/descriptor_utils.go b/pkg/upgrade/upgrades/descriptor_utils.go index 7b69eba8a181..5486167ba7e0 100644 --- a/pkg/upgrade/upgrades/descriptor_utils.go +++ b/pkg/upgrade/upgrades/descriptor_utils.go @@ -58,7 +58,8 @@ func CreateSystemTableInTxn( if got.Value.IsPresent() { return descpb.InvalidID, false, nil } - id, err := descidgen.GenerateUniqueDescID(ctx, db, codec) + id, err := descidgen.NewTransactionalGenerator(codec, txn). + GenerateUniqueDescID(ctx) if err != nil { return descpb.InvalidID, false, err } diff --git a/pkg/upgrade/upgrades/public_schema_migration.go b/pkg/upgrade/upgrades/public_schema_migration.go index a4ab8d5f4baa..67407db41be2 100644 --- a/pkg/upgrade/upgrades/public_schema_migration.go +++ b/pkg/upgrade/upgrades/public_schema_migration.go @@ -22,6 +22,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkeys" "github.com/cockroachdb/cockroach/pkg/sql/catalog/dbdesc" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/descidgen" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descs" "github.com/cockroachdb/cockroach/pkg/sql/catalog/schemadesc" @@ -106,7 +107,8 @@ func createPublicSchemaDescriptor( b := txn.NewBatch() publicSchemaDesc, _, err := sql.CreateSchemaDescriptorWithPrivileges( - ctx, d.DB, d.Codec, desc, tree.PublicSchema, username.AdminRoleName(), username.AdminRoleName(), true, /* allocateID */ + ctx, descidgen.NewGenerator(d.Codec, d.DB), desc, tree.PublicSchema, + username.AdminRoleName(), username.AdminRoleName(), true, /* allocateID */ ) // The public role has hardcoded privileges; see comment in // descpb.NewPublicSchemaPrivilegeDescriptor.