sql: introduce new internal executor interfaces

[ZhouXing19]

This PR aims to provide a set of safer interfaces for the internal executor, making it less easy to abuse.

Currently, each conn executor underneath the internal executor (we call it “child executor”) has its own set of information, such as descriptor collection, job collection, schema change jobs, etc, even when it’s run with a not-nil outer kv.Txn, or there're multiple SQL executions under the same kv.Txn.
This is not intuitive, since it violates a rather deep principle that a descs.Collection and a SQL txn have a 1:1 relationship. The code doesn’t enforce that, but it ought to. The more places that make it possible to decouple this, the more anxious we get.

Ideally, internal executor with a not-nil txn is either planner or collectionFactory oriented, so that the txn is always tightly coupled with the descriptor collection. We thus propose a set of new interfaces to ensure this coupling.

Currently, the usage of an internal executor query function (e.g. InternalExecutor.ExecEx()) falls into the following 3 categories:

1. The query is run under a planner context and with a not-nil kv.Txn from this planner.
2. The query is run without a kv.Txn. (e.g. InternalExecutor.ExecEx(..., nil /* txn */, stmt...)
3. The query is running with a not-nil kv.Txn but not under the planner context.

For usage 1, the descriptor collections, txn state, job collections, and session data from the parent planner are expected to be passed to the internal executor's child conn executor.
For usage 2 and 3, if multiple SQL statements are run under the same txn, these executions should share the descs.Collection, txn state machine, job collections and session data for their conn executors.

To suit these 3 use cases, we proposed 3 interfaces for each of the query function:
(In the following we use InternalExecutor.ExecEx as the example)

• For case 1, refactor to use func (p *planner) ExecExUpdated(), where the internal executor is always initialized with descs.Collection, TxnState and etc. from the sql.planner.
• For case 2, refactor to use ieFactory.WithoutTxn(), where the query is always run with a nil kv.Txn.
• For case 3, refactor to use CollectionFactory.TxnWithExecutor(). In this function, the internal executor is generated and passed to the call back function to run the query.

We also tried refactoring some of the existing use cases to give an example of the new interface.

(Note that the ultimate goal of this improvement is to deprecate all the "free-hanging" InternalExecutor objects (such as sql.ExecutorConfig.InternalExecutor) and replace them with an InternalExecutorFactory field. InternalExecutorFactory is to initialize a REAL internal executor, but it cannot be used directly to run SQL statement queries.
Instead, we wrap the initialization of an internal executor inside each query function, i.e. init it only when you really need to run a query. In other words, the creation of an internal executor becomes closer to the query running.)

fixes #69495
fixes #78998

Release Note: None

[cockroach-teamcity]

This change is 

[knz]

Could you explain to us what you recommend happens in the following "common" use case:

err := db.Txn(ctx, func(ctx context.Context, txn *kv.Txn) error {
   ... := internalExecutor.QueryRowEx(ctx, ..., txn, query)
})

FYI, What this pattern does is to run the query but make it robust to transaction retry errors: the code inside db.Txn() will re-execute the closure (containing the QueryRowEx call) every time a retry error is encountered, automatically.

Now, what would happen with the proposed APIs? It seems to me that it would be "expensive" to re-initialize an internalexecutor from scratch every time db.Txn invokes the closure.

Would there be a way to "refresh" the kv.Txn object inside the executor, the same way that sql.connExecutor does internally for auto-retries (like in prepareTxnForRetryWithRewind())?

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

[ajwerner]

If I understand correctly, nothing really has been made worse by this interface change. I'd like to unpack the extent to which it has.

Could you explain to us what you recommend happens in the following "common" use case:

The db.Txn use case is not that common, it's pattern (3) in the list. A similarly common pattern today is to pass the nil /* txn */ (pattern (2)). In the nil txn case, the connExecutor does all of the restart management, as you propose.

It seems to me that it would be "expensive" to re-initialize an internalexecutor from scratch every time db.Txn invokes the closure.

I don't think I buy this. The internalExecutor we have today is shockingly thin. We already build a new connExecutor for each invocation. That means this change is not worse than any existing code which uses an explicit txn argument, right?

cockroach/pkg/sql/internal.go

Lines 129 to 140 in cc6aa33

	// initConnEx creates a connExecutor and runs it on a separate goroutine. It
	// takes in a StmtBuf into which commands can be pushed and a WaitGroup that
	// will be signaled when connEx.run() returns.
	//
	// If txn is not nil, the statement will be executed in the respective txn.
	//
	// The ieResultWriter coordinates communicating results to the client. It may
	// block execution when rows are being sent in order to prevent hazardous
	// concurrency.
	//
	// sd will constitute the executor's session state.
	func (ie *InternalExecutor) initConnEx(

[knz]

ok, that's a good explanation thanks

[ZhouXing19]

Thanks @ajwerner for the explanation! Yes the understanding is correct.

Would there be a way to "refresh" the kv.Txn object inside the executor, the same way that sql.connExecutor does internally for auto-retries (like in prepareTxnForRetryWithRewind())?

As Andrew mentioned, the initialization of an internal executor itself is to call sql.MakeInternalExecutor() coupled with some addition setting for session data, descs.Collection and etc. It does create and start new monitor for the new ie, but it doesn't seem that expensive.

So if it's acceptable to re-init the internal executor for each retry in the db.Txn() example, I don't think we need to enable "refreshing" the kv.Txn inside the internal executor, since in each retry, a new kv.Txn will be passed to the new ie.

[ZhouXing19]

Does this PR fix #70888?

Sadly no. This PR doesn't deprecate the existing ExecCfg.InternalExecutor, which we proposed to do after merging this one. This PR proposes a new interface to initialize an internal executor, which is expected to eventually replace the current ExecCfg.InternalExecutor and solve this issue. But given the large number of usages of the latter, we'd prefer to have the replacement in a future PR.

[cucaroach]

Okay thanks for the update! No worries, Rome wasn't built in a day. Could we get an Informs: in your commit message to that issue just so things are looped together? Thanks!

[ZhouXing19]

Could we get an Informs: in your commit message to that issue just so things are looped together?

Yeah, sure! I'll add that.

[ZhouXing19]

Made changes per comments, except changing sqlutil.InternalExecutorFactory into an interface, for I'm not 100% sure about how to define the InternalExecutorFactory field of structs: #82477 (comment)

[ZhouXing19]

Made changes including changing sqlutil.InternalExecutorFactory to an interface. Ready for another look.

[ZhouXing19]

Friendly reping-ing @ajwerner for another look. Thanks!

[ajwerner]

This is more or less where I want it. I think it'd be good if we delegate the functionality we need in the descs layer to a method in the sql layer on the InternalExecutorFactory, but otherwise, I'm content with the shape this has taken. Thanks for all the iteration.

[ajwerner]

it feels to me like this should just be a method on the InternalExecutorFactory and that this library should describe the interface method. That way, we can avoid the closure in the server package.

[ZhouXing19]

Yeah I thought about that but it can be a bit difficult -- we will have to import descs package to sqlutil for the collection parameter, but that will again bring dependency loop.

[ajwerner]

I don't think that's true, sqlutil doesn't need to include this method in its interface, just the sql.InternalExecutorFactory has to implement the method. As in, we define an interface in descs which is distinct from the interface in sqlutil.

[ZhouXing19]

Ah, i see what you meant. Yeah, that makes sense, will do!

[ZhouXing19]

Done.

[ZhouXing19]

Thanks all for reviewing!
bors r=rafiss,ajwerner

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[andreimatei]

Why was this method necessary / how is it different from NewInternalExecutor() ? Can't the caller run exactly the two lines inside this function?
The comment talks about "binding to a txn" without other explanations. I don't think either fewer or more words would be needed since most callers don't care about that / don't know what "binding to a txn" means.

[ZhouXing19]

I agree that this function is far from ideal... We made this function with the hope of avoiding use cases where an internal executor is created without binding to any txn-related metadata, but is used to run queries with a not nil-txn. In other words, we wanted to make the usages "with" and "without" an outer txn more distinct from each other, and let callers think twice about which one they should use.
I think we can add a comment saying that it's disallowed to use this function to run DDLs or multiple statement in a transactional manner.

[andreimatei]

You can have a NewInternalExecutorWithoutTxn if you insist on the importance of having the "without part" in your face, but I don't see why the caller needs to structure its logic into a closure if they're not getting anything in return.

[ZhouXing19]

I think if we only have NewInternalExecutorWithoutTxn, it can still happen to the caller to use it to run statements with a not-nil txn, which is wrong. To wrap it in this function is to make it more explicit that you shouldn't do this (though it's true that we can't truly disallow it here)

I think the ideal case is to remove the txn field in internal executor's query functions (e.g. ie.QueryRowEx()). The txn should be bound to the internal executor, rather than each statement execution. With that, I think it's totally fine for us to remove this function and just do ie := NewInternalExecutorWithoutTxn ()