sql,logictest,descidgen: abstract descriptor ID generation, make deterministic in logictests

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",

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)
 		}

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
 					}

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
 		}

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",

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",
     ],
 )
 

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,15 +33,20 @@ 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,
 	regions []tree.Name,
 	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",
 	)
 }

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",

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 {

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
 	}

pkg/sql/catalog/descidgen/BUILD.bazel

@@ -10,6 +10,7 @@ go_library(
         "//pkg/keys",
         "//pkg/kv",
         "//pkg/sql/catalog/descpb",
+        "//pkg/sql/sem/catid",
     ],
 )
 

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 {

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 {

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
 	}