sql/sqlutil, descs: move TxnWithExecutor() and methods to sql.InternalExecutorFactory

[ZhouXing19]

Having TxnWithExecutor() in descs.CollectionFactory is unnatural, and will bring dependency loop headaches. This commit is to move the same logic under sql.InternalExecutorFactory.

Release note: None

[cockroach-teamcity]

This change is 

[ZhouXing19]

Do we really want this? Wouldn't it be enough to inject sqlutil.InternalExecutorFactory and use these methods directly?

[ZhouXing19]

RFAL

[ajwerner]

This seems right. Let's unwind the collection factory methods in the same PR while you're here.

Reviewed 3 of 4 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @ZhouXing19)

---

pkg/sql/catalog/descs/txn.go line 93 at r1 (raw file):

Previously, ZhouXing19 (Jane Xing) wrote…

Do we really want this? Wouldn't it be enough to inject sqlutil.InternalExecutorFactory and use these methods directly?

Yeah I think you're right that that's better.

---

pkg/sql/catalog/descs/txn.go line 115 at r2 (raw file):

// WaitForDescriptorsFunc return a function that waits for descriptors that
// were modified, skipping over ones that had their descriptor wiped.
func (cf *CollectionFactory) WaitForDescriptorsFunc(

I don't see why this needs to be here. The InternalExecutorFactory implementation has a leaseMgr. We're not getting anything else out of this method, are we?

Code quote:

// WaitForDescriptorsFunc return a function that waits for descriptors that
// were modified, skipping over ones that had their descriptor wiped.
func (cf *CollectionFactory) WaitForDescriptorsFunc(
	ctx context.Context,
) WaitForModifiedDescsFunc {
	return func(modifiedDescriptors []lease.IDVersion, deletedDescs catalog.DescriptorIDSet) error {

[ZhouXing19]

Let's unwind the collection factory methods in the same PR

Hmm do you mean completely replace descs.CollectionFactory.TxnWithExecutor() with sql.InternalExecutor.TxnWithExecutor()? I can do that, but want to get your approval first for the new methods we have here, so that we can have a concrete plan to change the existing usages.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner)

---

pkg/sql/catalog/descs/txn.go line 93 at r1 (raw file):

Previously, ajwerner wrote…

Yeah I think you're right that that's better.

Ah, I gave a second thought and I think you made sense. DescsTxnWithExecutor() and DescsTxn() are not in sqlutil. So if someone wants to use them, they have to convert sqltuil.InternalExecutor to sql.InternalExecutor, which means they will have to import sql. But with these 2 methods declared in an interface in descs, they just have to import descs, which is, I think, lighter than sql.
So I just moved them to descs.InternalExecutorFactoryWithTxn above.

---

pkg/sql/catalog/descs/txn.go line 115 at r2 (raw file):

Previously, ajwerner wrote…

I don't see why this needs to be here. The InternalExecutorFactory implementation has a leaseMgr. We're not getting anything else out of this method, are we?

Right there's no need to separate them into another function. Changed it back.

[ajwerner]

this is looking right to me

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @ZhouXing19)

---

pkg/sql/internal.go line 1402 at r3 (raw file):

) error {
	run := ApplyTxnOptions(db, opts...)
	cf := ief.server.cfg.CollectionFactory

I don't think we need to involve the CollectionFactory at all here, the lease manager is ief.server.cfg.LeaseManager

---

pkg/sql/catalog/descs/txn.go line 93 at r1 (raw file):

Previously, ZhouXing19 (Jane Xing) wrote…

Ah, I gave a second thought and I think you made sense. DescsTxnWithExecutor() and DescsTxn() are not in sqlutil. So if someone wants to use them, they have to convert sqltuil.InternalExecutor to sql.InternalExecutor, which means they will have to import sql. But with these 2 methods declared in an interface in descs, they just have to import descs, which is, I think, lighter than sql.
So I just moved them to descs.InternalExecutorFactoryWithTxn above.

right because we don't want sqlutil to depend on descs 👍

[ZhouXing19]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @ZhouXing19)

---

pkg/sql/internal.go line 1402 at r3 (raw file):

Previously, ajwerner wrote…

I don't think we need to involve the CollectionFactory at all here, the lease manager is ief.server.cfg.LeaseManager

But eventually we'd need cf.NewCollection() to create the descriptor collection for the internal executor

[ajwerner]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @ZhouXing19)

---

pkg/sql/internal.go line 1402 at r3 (raw file):

Previously, ZhouXing19 (Jane Xing) wrote…

But eventually we'd need cf.NewCollection() to create the descriptor collection for the internal executor

right right, I guess I just mean we don't need its GetLeaseManager() method (which maybe we can now remove?)

[ZhouXing19]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @ZhouXing19)

---

pkg/sql/internal.go line 1402 at r3 (raw file):

Previously, ajwerner wrote…

right right, I guess I just mean we don't need its GetLeaseManager() method (which maybe we can now remove?)

Oh descs.CollectionFactory.leaseMgr can't be exported from descs, and we're at sql here, so i think we still need GetLeaseManager()

[ajwerner]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @ZhouXing19)

---

pkg/sql/internal.go line 1402 at r3 (raw file):

Previously, ZhouXing19 (Jane Xing) wrote…

Oh descs.CollectionFactory.leaseMgr can't be exported from descs, and we're at sql here, so i think we still need GetLeaseManager()

ief.server.cfg.LeaseManager has a handle to LeaseManager already

[ZhouXing19]

pkg/sql/internal.go line 1402 at r3 (raw file):

Previously, ajwerner wrote…

ief.server.cfg.LeaseManager has a handle to LeaseManager already

Ah yeah right 😅 made the change!

[ZhouXing19]

Failed tests were flaky, RFAL, thanks!

[ZhouXing19]

Friendly re-pinging for a review. Thanks!

[ajwerner]

In general, can you take a pass at removing the reaching from the server to the collection factory to an internal executor factory and rework those to go from the server right to the InternalExecutorFactory?

Reviewed all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @rhu713, and @ZhouXing19)

---

pkg/ccl/backupccl/restore_planning.go line 709 at r4 (raw file):

// restore.
func dropDefaultUserDBs(ctx context.Context, execCfg *sql.ExecutorConfig) error {
	ief := execCfg.InternalExecutorFactory.(*sql.InternalExecutorFactory)

I don't love this type assertion. Can we define a superset interface for the sql.ExecutorConfig. Maybe descs.InternalExecutorFactory which embeds sqlutils.InternalExecutorFactory and has the added methods?

---

pkg/sql/catalog/descs/factory.go line 41 at r4 (raw file):

	spanConfigLimiter  spanconfig.Limiter
	defaultMonitor     *mon.BytesMonitor
	ieFactoryWithTxn   InternalExecutorFactoryWithTxn

what is this now used for?

---

pkg/sql/catalog/descs/factory.go line 52 at r4 (raw file):

// collection factory.
func (cf *CollectionFactory) GetInternalExecutorFactory() InternalExecutorFactoryWithTxn {
	return cf.ieFactoryWithTxn

Why do these exist? The places they seem to be accessed already have the

Code quote:

// GetClusterSettings returns the cluster setting from the collection factory.
func (cf *CollectionFactory) GetClusterSettings() *cluster.Settings {
	return cf.settings
}

// GetInternalExecutorFactory returns the internal executor factory of the
// collection factory.
func (cf *CollectionFactory) GetInternalExecutorFactory() InternalExecutorFactoryWithTxn {
	return cf.ieFactoryWithTxn
}

---

pkg/sql/catalog/descs/factory.go line 58 at r4 (raw file):

// with associated extra txn state information.
// It should only be used as a field hanging off CollectionFactory.
type InternalExecutorFactoryWithTxn interface {

Let's rename this to just InternalExecutorFactory and embed sqlutils.InternalExecutorFactory in it? Alternatively, what do you think about TxnManager as the name?

---

pkg/sql/catalog/descs/factory.go line 59 at r4 (raw file):

// It should only be used as a field hanging off CollectionFactory.
type InternalExecutorFactoryWithTxn interface {
	MemoryMonitor() *mon.BytesMonitor

Are either of these methods needed anymore?

Code quote:

	MemoryMonitor() *mon.BytesMonitor

	NewInternalExecutorWithTxn(
		sd *sessiondata.SessionData,
		sv *settings.Values,
		txn *kv.Txn,
		descCol *Collection,
	) (sqlutil.InternalExecutor, InternalExecutorCommitTxnFunc)

---

pkg/sql/catalog/descs/system_table.go line 46 at r4 (raw file):

	var id descpb.ID
	if err := r.collectionFactory.GetInternalExecutorFactory().DescsTxn(ctx, r.db, func(

let's change the dependency here

---

pkg/sql/stats/automatic_stats.go line 354 at r4 (raw file):

	ctx context.Context, tableID descpb.ID,
) (desc catalog.TableDescriptor) {
	if err := r.cache.collectionFactory.GetInternalExecutorFactory().DescsTxn(ctx, r.cache.ClientDB, func(

let's change the dependency here to descs.InternalExecutorFactory

[ZhouXing19]

In general, can you take a pass at removing the reaching from the server to the collection factory to an internal executor factory and rework those to go from the server right to the InternalExecutorFactory?

Made the changes!
This should be ready for another look. Thank you!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @herkolategan, @rhu713, and @ZhouXing19)

---

pkg/ccl/backupccl/restore_planning.go line 709 at r4 (raw file):

Previously, ajwerner wrote…

I don't love this type assertion. Can we define a superset interface for the sql.ExecutorConfig. Maybe descs.InternalExecutorFactory which embeds sqlutils.InternalExecutorFactory and has the added methods?

Thanks for the good idea! I made descs.TxnManager a superset interface, and changed execCfg.InternalExecutorFactory to a descs.TxnManager as well.

---

pkg/sql/catalog/descs/factory.go line 41 at r4 (raw file):

Previously, ajwerner wrote…

what is this now used for?

Removed.

---

pkg/sql/catalog/descs/factory.go line 52 at r4 (raw file):

Previously, ajwerner wrote…

Why do these exist? The places they seem to be accessed already have the

Removed.

---

pkg/sql/catalog/descs/factory.go line 58 at r4 (raw file):

Previously, ajwerner wrote…

Let's rename this to just InternalExecutorFactory and embed sqlutils.InternalExecutorFactory in it? Alternatively, what do you think about TxnManager as the name?

Renamed to TxnManager.

---

pkg/sql/catalog/descs/factory.go line 59 at r4 (raw file):

Previously, ajwerner wrote…

Are either of these methods needed anymore?

Nope, removed.

---

pkg/sql/catalog/descs/system_table.go line 46 at r4 (raw file):

Previously, ajwerner wrote…

let's change the dependency here

Done.

---

pkg/sql/stats/automatic_stats.go line 354 at r4 (raw file):

Previously, ajwerner wrote…

let's change the dependency here to descs.InternalExecutorFactory

Done.

[ajwerner]

just one more dep to clean up

Reviewed 1 of 5 files at r1, 1 of 32 files at r5, 75 of 75 files at r6, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @herkolategan, @rhu713, and @ZhouXing19)

---

pkg/sql/catalog/descs/factory.go line 52 at r6 (raw file):

	sqlutil.InternalExecutorFactory

	DescsTxnWithExecutor(

nit: put some commentary on these methods -- at least referencing the sqlutil methods they are like

---

pkg/sql/stats/stats_cache.go line 78 at r6 (raw file):

	// Used to resolve descriptors.
	collectionFactory *descs.CollectionFactory

I think you can remove this dependency now

[ZhouXing19]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ajwerner, @herkolategan, @rhu713, and @ZhouXing19)

---

pkg/sql/catalog/descs/factory.go line 52 at r6 (raw file):

Previously, ajwerner wrote…

nit: put some commentary on these methods -- at least referencing the sqlutil methods they are like

Done adding the comments.

---

pkg/sql/stats/stats_cache.go line 78 at r6 (raw file):

Previously, ajwerner wrote…

I think you can remove this dependency now

Done.

[ZhouXing19]

TFTR!
bors r=ajwerner

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from 4954b70 to blathers/backport-release-22.2-88875: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.2.x failed. See errors above.

---

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}