*,isql: introduce isql.Txn and isql.DB

[ajwerner]

This massive refactor works to bind the *kv.Txn with the internal executor and other sql-layer, txn-associated state. This work follows on from an earlier project to more tightly couple internal executors to the rest of extraTxnState. That project resulted in sprawling changes to propagated the paired dependencies through the system. In practice, we're better off coupling them through an object.

There are some refactors added in here to curry and hide some of these dependencies from interfaces. Those may be possible to extract to be separate.

Additionally, not all of the dependency sprawl has been eliminated; there are cases where we could pass a isql.Txn but instead keep passing the underlying isql.Executor and *kv.Txn. We can do more cleanup along the way.

Lastly, I couldn't help myself from lifting some sql.ExecCfg arguments up and being more specific in some places.

Epic: none

Release note: None

[cockroach-teamcity]

This change is 

[postamar]

LGTM. I reviewed this PR looking at the changes in my editor and came up with some very uncontroversial fixes which I'm publishing as a patch file below. In addition to that, my comments:

1. This is a very welcome change. I particularly like the changes to the jobs.
2. This naturally begs for a further refactor in which the kv.txn no longer needs to be passed to the descs.Collection and Executor methods. I look forward to this and pledge to deal with the descs.Collection in particular. This is a golden opportunity to introduce better by-name and by-ID lookup interfaces with sane defaults and without the need to pass tree.CommonLookupFlags and friends. I have an idea on how to proceed. In any case perhaps TODO notes on sql.DescsTxn and the Executor methods are called for?
3. It might be worth tightening structs which hold on to a *kv.DB alongside one of the new interfaces only to use it for the clock or something like that. I see this in the lease manager's storage for instance.
4. Related: for instance stmtdiagnostics.NewRegistry takes both a *kv.DB and a isql.Executor because it needs to be given a different ie. Would it make sense to pass custom-made isql.DB instances instead? Perhaps by endowing that interface with an UsingExecutor method which returns a copy of itself but using a different ie?
5. Consider renaming descs.InternalExecFn and friends. At first glance I wondered why they couldn't be a func(ctx context.Context, txn descs.Txn) instead, but it appears that it's not so straightforward to do. Am I correct in guessing that this is left for later? If so consider leaving some TODO notes.
6. The jobs.Updater now indiscriminately passes useReadLock = false. Is this expected? Some call sites of Update are still littered with const useReadLock = true suggesting that it's something that mattered. Either un-plumb this boolean arg from u.update or bring in a new UpdateWithReadLock method.
7. Consider renaming sessiondata.InternalExecutorOverride to sessiondata.ExecutorOverride?
8. Consider adding TODOs to SchemaChangermethods which wrap retryables to rewrite these using descs.Txn and to use the session data inside the Txn instead of passing it separately?
9. The eval.Context is also going to need some love, TODOs please.
10. Do a full project text search for TODO (janexing) and update as needed please.
11. planner.Txn() isn't used much anymore, is it possible to get rid of it?

From 8ebb2cd8461f5b01a8b233ed701e6a53ad865aa4 Mon Sep 17 00:00:00 2001
From: Marius Posta <[email protected]>
Date: Thu, 15 Dec 2022 12:34:15 -0500
Subject: [PATCH] small fixes

---
 pkg/ccl/backupccl/schedule_exec.go               |  5 ++---
 .../multitenantccl/tenantcostserver/server.go    |  8 ++------
 pkg/jobs/scheduled_job_executor.go               |  3 +--
 pkg/server/node.go                               |  1 -
 pkg/server/server.go                             |  2 +-
 pkg/server/tenant.go                             |  6 +++---
 pkg/sql/backfill/mvcc_index_merger.go            |  2 +-
 pkg/sql/catalog/descidgen/generate_id.go         | 16 ++++------------
 .../schematelemetry/scheduled_job_executor.go    |  3 +--
 pkg/sql/compact_sql_stats.go                     |  3 +--
 pkg/sql/control_schedules.go                     |  1 -
 .../captured_index_usage_stats.go                |  4 ----
 pkg/sql/ttl/ttlschedule/ttlschedule.go           | 13 +++++--------
 pkg/testutils/serverutils/test_server_shim.go    |  5 ++---
 pkg/upgrade/tenant_upgrade.go                    |  1 -
 pkg/util/stop/stopper.go                         |  2 +-
 16 files changed, 24 insertions(+), 51 deletions(-)

diff --git a/pkg/ccl/backupccl/schedule_exec.go b/pkg/ccl/backupccl/schedule_exec.go
index a17602b896b..ea809fa721c 100644
--- a/pkg/ccl/backupccl/schedule_exec.go
+++ b/pkg/ccl/backupccl/schedule_exec.go
@@ -419,7 +419,7 @@ func unlinkOrDropDependentSchedule(
 	ctx context.Context,
 	scheduleControllerEnv scheduledjobs.ScheduleControllerEnv,
 	env scheduledjobs.JobSchedulerEnv,
-	txn isql.Txn,
+	txn descs.Txn,
 	args *backuppb.ScheduledBackupExecutionArgs,
 ) (int, error) {
 	if args.DependentScheduleID == 0 {
@@ -478,8 +478,7 @@ func (e *scheduledBackupExecutor) OnDrop(
 	scheduleControllerEnv scheduledjobs.ScheduleControllerEnv,
 	env scheduledjobs.JobSchedulerEnv,
 	sj *jobs.ScheduledJob,
-	txn isql.Txn,
-	descsCol *descs.Collection,
+	txn descs.Txn,
 ) (int, error) {
 	args := &backuppb.ScheduledBackupExecutionArgs{}
 
diff --git a/pkg/ccl/multitenantccl/tenantcostserver/server.go b/pkg/ccl/multitenantccl/tenantcostserver/server.go
index 3a4911e9001..ebd1f92618d 100644
--- a/pkg/ccl/multitenantccl/tenantcostserver/server.go
+++ b/pkg/ccl/multitenantccl/tenantcostserver/server.go
@@ -11,7 +11,6 @@ package tenantcostserver
 import (
 	"time"
 
-	"github.com/cockroachdb/cockroach/pkg/kv"
 	"github.com/cockroachdb/cockroach/pkg/multitenant"
 	"github.com/cockroachdb/cockroach/pkg/server"
 	"github.com/cockroachdb/cockroach/pkg/settings"
@@ -22,7 +21,6 @@ import (
 )
 
 type instance struct {
-	db         *kv.DB
 	ief        isql.DB
 	metrics    Metrics
 	timeSource timeutil.TimeSource
@@ -41,10 +39,9 @@ var instanceInactivity = settings.RegisterDurationSetting(
 )
 
 func newInstance(
-	settings *cluster.Settings, db *kv.DB, ief isql.DB, timeSource timeutil.TimeSource,
+	settings *cluster.Settings, ief isql.DB, timeSource timeutil.TimeSource,
 ) *instance {
 	res := &instance{
-		db:         db,
 		ief:        ief,
 		timeSource: timeSource,
 		settings:   settings,
@@ -63,9 +60,8 @@ var _ multitenant.TenantUsageServer = (*instance)(nil)
 func init() {
 	server.NewTenantUsageServer = func(
 		settings *cluster.Settings,
-		db *kv.DB,
 		ief isql.DB,
 	) multitenant.TenantUsageServer {
-		return newInstance(settings, db, ief, timeutil.DefaultTimeSource{})
+		return newInstance(settings, ief, timeutil.DefaultTimeSource{})
 	}
 }
diff --git a/pkg/jobs/scheduled_job_executor.go b/pkg/jobs/scheduled_job_executor.go
index b6600bfb446..99baa012764 100644
--- a/pkg/jobs/scheduled_job_executor.go
+++ b/pkg/jobs/scheduled_job_executor.go
@@ -73,8 +73,7 @@ type ScheduledJobController interface {
 		scheduleControllerEnv scheduledjobs.ScheduleControllerEnv,
 		env scheduledjobs.JobSchedulerEnv,
 		schedule *ScheduledJob,
-		txn isql.Txn,
-		descsCol *descs.Collection,
+		txn descs.Txn,
 	) (int, error)
 }
 
diff --git a/pkg/server/node.go b/pkg/server/node.go
index c953358613f..5b7fd01cee6 100644
--- a/pkg/server/node.go
+++ b/pkg/server/node.go
@@ -1809,7 +1809,6 @@ func (n *Node) TokenBucket(
 // server.
 var NewTenantUsageServer = func(
 	settings *cluster.Settings,
-	db *kv.DB,
 	ief isql.DB,
 ) multitenant.TenantUsageServer {
 	return dummyTenantUsageServer{}
diff --git a/pkg/server/server.go b/pkg/server/server.go
index 7d5c1fa8ee4..e7505d116db 100644
--- a/pkg/server/server.go
+++ b/pkg/server/server.go
@@ -725,7 +725,7 @@ func NewServer(cfg Config, stopper *stop.Stopper) (*Server, error) {
 		updates.TestingKnobs = &cfg.TestingKnobs.Server.(*TestingKnobs).DiagnosticsTestingKnobs
 	}
 
-	tenantUsage := NewTenantUsageServer(st, db, insqlDB)
+	tenantUsage := NewTenantUsageServer(st, insqlDB)
 	registry.AddMetricStruct(tenantUsage.Metrics())
 
 	tenantSettingsWatcher := tenantsettingswatcher.New(
diff --git a/pkg/server/tenant.go b/pkg/server/tenant.go
index 93f9c4e5c21..54225dc0664 100644
--- a/pkg/server/tenant.go
+++ b/pkg/server/tenant.go
@@ -832,14 +832,14 @@ func makeTenantSQLServerArgs(
 	)
 
 	circularInternalExecutor := &sql.InternalExecutor{}
-	internalExecutorFactory := &sql.InternalDB{}
+	internalDB := &sql.InternalDB{}
 	circularJobRegistry := &jobs.Registry{}
 
 	// Initialize the protectedts subsystem in multi-tenant clusters.
 	var protectedTSProvider protectedts.Provider
 	protectedtsKnobs, _ := baseCfg.TestingKnobs.ProtectedTS.(*protectedts.TestingKnobs)
 	pp, err := ptprovider.New(ptprovider.Config{
-		DB:       internalExecutorFactory,
+		DB:       internalDB,
 		Settings: st,
 		Knobs:    protectedtsKnobs,
 		ReconcileStatusFuncs: ptreconcile.StatusFuncs{
@@ -934,7 +934,7 @@ func makeTenantSQLServerArgs(
 		sessionRegistry:          sessionRegistry,
 		remoteFlowRunner:         remoteFlowRunner,
 		circularInternalExecutor: circularInternalExecutor,
-		internalDB:               internalExecutorFactory,
+		internalDB:               internalDB,
 		circularJobRegistry:      circularJobRegistry,
 		protectedtsProvider:      protectedTSProvider,
 		rangeFeedFactory:         rangeFeedFactory,
diff --git a/pkg/sql/backfill/mvcc_index_merger.go b/pkg/sql/backfill/mvcc_index_merger.go
index 2d82970efed..692d81ccf7a 100644
--- a/pkg/sql/backfill/mvcc_index_merger.go
+++ b/pkg/sql/backfill/mvcc_index_merger.go
@@ -377,7 +377,7 @@ func (ibm *IndexBackfillMerger) merge(
 			}
 		}
 		return nil
-	})
+	}, isql.WithPriority(admissionpb.BulkNormalPri))
 
 	return err
 }
diff --git a/pkg/sql/catalog/descidgen/generate_id.go b/pkg/sql/catalog/descidgen/generate_id.go
index 0b21e12a7e3..43416c4d976 100644
--- a/pkg/sql/catalog/descidgen/generate_id.go
+++ b/pkg/sql/catalog/descidgen/generate_id.go
@@ -28,7 +28,6 @@ import (
 type generator struct {
 	settings *cluster.Settings
 	codec    keys.SQLCodec
-	key      func(context.Context) (roachpb.Key, error)
 	getOrInc func(ctx context.Context, key roachpb.Key, inc int64) (int64, error)
 }
 
@@ -54,9 +53,9 @@ func (g *generator) PeekNextUniqueDescID(ctx context.Context) (descpb.ID, error)
 
 // run is a convenience method for accessing the descriptor ID counter.
 func (g *generator) run(ctx context.Context, inc int64) (catid.DescID, error) {
-	key, err := g.key(ctx)
+	key, err := key(ctx, g.codec, g.settings)
 	if err != nil {
-		return 0, err
+		return catid.InvalidDescID, err
 	}
 	nextID, err := g.getOrInc(ctx, key, inc)
 	return catid.DescID(nextID), err
@@ -88,9 +87,6 @@ func NewGenerator(settings *cluster.Settings, codec keys.SQLCodec, db *kv.DB) ev
 	return &generator{
 		settings: settings,
 		codec:    codec,
-		key: func(ctx context.Context) (roachpb.Key, error) {
-			return key(ctx, codec, settings)
-		},
 		getOrInc: func(ctx context.Context, key roachpb.Key, inc int64) (int64, error) {
 			if inc == 0 {
 				ret, err := db.Get(ctx, key)
@@ -104,7 +100,6 @@ func NewGenerator(settings *cluster.Settings, codec keys.SQLCodec, db *kv.DB) ev
 func key(
 	ctx context.Context, codec keys.SQLCodec, settings *cluster.Settings,
 ) (roachpb.Key, error) {
-	key := codec.SequenceKey(keys.DescIDSequenceID)
 	if cv := settings.Version; codec.ForSystemTenant() &&
 		!cv.IsActive(ctx, clusterversion.V23_1DescIDSequenceForSystemTenant) {
 		// At this point, the system tenant may still be using a legacy non-SQL key,
@@ -113,9 +108,9 @@ func key(
 		if cv.IsActive(ctx, clusterversion.V23_1DescIDSequenceForSystemTenant-1) {
 			return nil, ErrDescIDSequenceMigrationInProgress
 		}
-		key = keys.LegacyDescIDGenerator
+		return keys.LegacyDescIDGenerator, nil
 	}
-	return key, nil
+	return codec.SequenceKey(keys.DescIDSequenceID), nil
 }
 
 // NewTransactionalGenerator constructs a transactional eval.DescIDGenerator.
@@ -125,9 +120,6 @@ func NewTransactionalGenerator(
 	return &generator{
 		settings: settings,
 		codec:    codec,
-		key: func(ctx context.Context) (roachpb.Key, error) {
-			return key(ctx, codec, settings)
-		},
 		getOrInc: func(ctx context.Context, key roachpb.Key, inc int64) (_ int64, err error) {
 			var ret kv.KeyValue
 			if inc == 0 {
diff --git a/pkg/sql/catalog/schematelemetry/scheduled_job_executor.go b/pkg/sql/catalog/schematelemetry/scheduled_job_executor.go
index 4b937c64695..5ef68e1f61a 100644
--- a/pkg/sql/catalog/schematelemetry/scheduled_job_executor.go
+++ b/pkg/sql/catalog/schematelemetry/scheduled_job_executor.go
@@ -48,8 +48,7 @@ func (s schemaTelemetryExecutor) OnDrop(
 	scheduleControllerEnv scheduledjobs.ScheduleControllerEnv,
 	env scheduledjobs.JobSchedulerEnv,
 	schedule *jobs.ScheduledJob,
-	txn isql.Txn,
-	descsCol *descs.Collection,
+	txn descs.Txn,
 ) (int, error) {
 	return 0, errScheduleUndroppable
 }
diff --git a/pkg/sql/compact_sql_stats.go b/pkg/sql/compact_sql_stats.go
index e945967ca62..10d69fd6b1c 100644
--- a/pkg/sql/compact_sql_stats.go
+++ b/pkg/sql/compact_sql_stats.go
@@ -158,8 +158,7 @@ func (e *scheduledSQLStatsCompactionExecutor) OnDrop(
 	scheduleControllerEnv scheduledjobs.ScheduleControllerEnv,
 	env scheduledjobs.JobSchedulerEnv,
 	schedule *jobs.ScheduledJob,
-	txn isql.Txn,
-	descsCol *descs.Collection,
+	txn descs.Txn,
 ) (int, error) {
 	return 0, persistedsqlstats.ErrScheduleUndroppable
 }
diff --git a/pkg/sql/control_schedules.go b/pkg/sql/control_schedules.go
index 363cb575f90..ce141285667 100644
--- a/pkg/sql/control_schedules.go
+++ b/pkg/sql/control_schedules.go
@@ -170,7 +170,6 @@ func (n *controlSchedulesNode) startExec(params runParams) error {
 					scheduledjobs.ProdJobSchedulerEnv,
 					schedule,
 					params.p.InternalSQLTxn(),
-					params.p.Descriptors(),
 				)
 				if err != nil {
 					return errors.Wrap(err, "failed to run OnDrop")
diff --git a/pkg/sql/scheduledlogging/captured_index_usage_stats.go b/pkg/sql/scheduledlogging/captured_index_usage_stats.go
index 8bcee647ab8..92ae78829ce 100644
--- a/pkg/sql/scheduledlogging/captured_index_usage_stats.go
+++ b/pkg/sql/scheduledlogging/captured_index_usage_stats.go
@@ -14,7 +14,6 @@ import (
 	"context"
 	"time"
 
-	"github.com/cockroachdb/cockroach/pkg/kv"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/security/username"
 	"github.com/cockroachdb/cockroach/pkg/settings"
@@ -82,7 +81,6 @@ func (*CaptureIndexUsageStatsTestingKnobs) ModuleTestingKnobs() {}
 // CaptureIndexUsageStatsLoggingScheduler is responsible for logging index usage stats
 // on a scheduled interval.
 type CaptureIndexUsageStatsLoggingScheduler struct {
-	db                      *kv.DB
 	st                      *cluster.Settings
 	ie                      isql.Executor
 	knobs                   *CaptureIndexUsageStatsTestingKnobs
@@ -120,13 +118,11 @@ func (s *CaptureIndexUsageStatsLoggingScheduler) durationUntilNextInterval() tim
 func Start(
 	ctx context.Context,
 	stopper *stop.Stopper,
-	db *kv.DB,
 	cs *cluster.Settings,
 	ie isql.Executor,
 	knobs *CaptureIndexUsageStatsTestingKnobs,
 ) {
 	scheduler := CaptureIndexUsageStatsLoggingScheduler{
-		db:    db,
 		st:    cs,
 		ie:    ie,
 		knobs: knobs,
diff --git a/pkg/sql/ttl/ttlschedule/ttlschedule.go b/pkg/sql/ttl/ttlschedule/ttlschedule.go
index 7b2e0eb320b..74efc2aed77 100644
--- a/pkg/sql/ttl/ttlschedule/ttlschedule.go
+++ b/pkg/sql/ttl/ttlschedule/ttlschedule.go
@@ -16,7 +16,6 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/jobs"
 	"github.com/cockroachdb/cockroach/pkg/jobs/jobspb"
-	"github.com/cockroachdb/cockroach/pkg/kv"
 	"github.com/cockroachdb/cockroach/pkg/scheduledjobs"
 	"github.com/cockroachdb/cockroach/pkg/security/username"
 	"github.com/cockroachdb/cockroach/pkg/sql"
@@ -56,8 +55,7 @@ func (s rowLevelTTLExecutor) OnDrop(
 	scheduleControllerEnv scheduledjobs.ScheduleControllerEnv,
 	env scheduledjobs.JobSchedulerEnv,
 	schedule *jobs.ScheduledJob,
-	txn isql.Txn,
-	descsCol *descs.Collection,
+	txn descs.Txn,
 ) (int, error) {
 
 	var args catpb.ScheduledRowLevelTTLArgs
@@ -65,13 +63,13 @@ func (s rowLevelTTLExecutor) OnDrop(
 		return 0, err
 	}
 
-	canDrop, err := canDropTTLSchedule(ctx, txn.KV(), descsCol, schedule, args)
+	canDrop, err := canDropTTLSchedule(ctx, txn, schedule, args)
 	if err != nil {
 		return 0, err
 	}
 
 	if !canDrop {
-		tn, err := descs.GetTableNameByID(ctx, txn.KV(), descsCol, args.TableID)
+		tn, err := descs.GetTableNameByID(ctx, txn.KV(), txn.Descriptors(), args.TableID)
 		if err != nil {
 			return 0, err
 		}
@@ -92,12 +90,11 @@ func (s rowLevelTTLExecutor) OnDrop(
 // valid.
 func canDropTTLSchedule(
 	ctx context.Context,
-	txn *kv.Txn,
-	descsCol *descs.Collection,
+	txn descs.Txn,
 	schedule *jobs.ScheduledJob,
 	args catpb.ScheduledRowLevelTTLArgs,
 ) (bool, error) {
-	desc, err := descsCol.GetImmutableTableByID(ctx, txn, args.TableID, tree.ObjectLookupFlags{})
+	desc, err := txn.Descriptors().GetImmutableTableByID(ctx, txn.KV(), args.TableID, tree.ObjectLookupFlags{})
 	if err != nil {
 		// If the descriptor does not exist we can drop this schedule.
 		if sqlerrors.IsUndefinedRelationError(err) {
diff --git a/pkg/testutils/serverutils/test_server_shim.go b/pkg/testutils/serverutils/test_server_shim.go
index 2f7ce1838b0..21107baf6a0 100644
--- a/pkg/testutils/serverutils/test_server_shim.go
+++ b/pkg/testutils/serverutils/test_server_shim.go
@@ -130,15 +130,14 @@ type TestServerInterface interface {
 	// DB returns a *client.DB instance for talking to this KV server.
 	DB() *kv.DB
 
-	// LeaseManager() returns the *sql.LeaseManager as an interface{}.
+	// LeaseManager returns the *sql.LeaseManager as an interface{}.
 	LeaseManager() interface{}
 
 	// InternalExecutor returns a *sql.InternalExecutor as an interface{} (which
 	// also implements insql.InternalExecutor if the test cannot depend on sql).
 	InternalExecutor() interface{}
 
-	// InternalExecutorInternalExecutorFactory returns a
-	// insql.InternalDB as an interface{}.
+	// InternalDB returns an isql.InternalDB as an interface{}.
 	InternalDB() interface{}
 
 	// TracerI returns a *tracing.Tracer as an interface{}.
diff --git a/pkg/upgrade/tenant_upgrade.go b/pkg/upgrade/tenant_upgrade.go
index cd0850ed004..769e22325ab 100644
--- a/pkg/upgrade/tenant_upgrade.go
+++ b/pkg/upgrade/tenant_upgrade.go
@@ -33,7 +33,6 @@ import (
 // TenantDeps are the dependencies of upgrades which perform actions at the
 // SQL layer.
 type TenantDeps struct {
-	KVDB             *kv.DB
 	Codec            keys.SQLCodec
 	Settings         *cluster.Settings
 	DB               descs.DB
diff --git a/pkg/util/stop/stopper.go b/pkg/util/stop/stopper.go
index 870219716a3..21b081e7106 100644
--- a/pkg/util/stop/stopper.go
+++ b/pkg/util/stop/stopper.go
@@ -283,7 +283,7 @@ func (s *Stopper) WithCancelOnQuiesce(ctx context.Context) (context.Context, fun
 // RunTask adds one to the count of tasks left to quiesce in the system.
 // Any worker which is a "first mover" when starting tasks must call this method
 // before starting work on a new task. First movers include goroutines launched
-// to do periodic work and the kv/isql_db.go gateway which accepts external client
+// to do periodic work and the kv/db.go gateway which accepts external client
 // requests.
 //
 // taskName is used as the "operation" field of the span opened for this task
-- 
2.38.1

[ajwerner]

bors r+

[craig]

Build failed:

• Bazel Essential CI (Cockroach)

[ajwerner]

The flake was #92809. I keep trying to repro with COCKROACH_DEBUG_SPAN_USE_AFTER_FINISH=t and it won't even though it repros very fast without that set. I'm going to just try again.

bors r+

[craig]

Canceled.

[ajwerner]

bors r+

🤞

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[yuzefovich]

It seems like this change made it much more likely for the race in #92809 to occur, and I have a fix in flight. Andrew, you could have just pinged me to fix it sooner if it was bothering you :)

[ajwerner]

@yuzefovich I stressed it with COCKROACH_DEBUG_SPAN_USE_AFTER_FINISH=t for over 30 minutes and I couldn't get it to fail, though it did flake very rapidly without that flag. I have no idea.

[yuzefovich]

I think the failure occurs when the node is quescing at the end of the logic test, and perhaps COCKROACH_DEBUG_SPAN_USE_AFTER_FINISH knob slows the shutdown process enough for race to be much more rare.