sql: remove InternalExecutor from EvalCtx

[RichardJCai]

Minor refactor to remove duplicate InternalExecutor definition
on EvalCtx and in the tree package.

We already have 2 interface declarations for InternalExecutor
in sql and sqlutil, this refactor hopefully makes the dependencies
a little more clear.

Resolves #60507

[cockroach-teamcity]

This change is 

[RichardJCai]

Need to do a bit more cleanup but what are the thoughts on using the InternalExecutor on ExecCfg for builtins.

The downside is that we need to specify more overrides to use the InternalExecutor for builtins but I think this is somewhat more clear of an interface.

[ajwerner]

I'm torn. I think removing the complexity as we have it is fine, but I don't know that this the direction I'd like for us to go longer term. Maybe this is a good stepping stone towards the better future.

I think my preference would be for us to rework the interfaces such that you always have to inject a bunch of information into the internal executor to use it. Like, imagine you always have a factory hanging off of the ExecCfg and you never have a thing that you can directly issue queries off of. Then, in the context of a transaction you you have a fully constructed internal executor that is tightly bound to a bunch of your conn executor state like the transaction, session data, extra txn state (including the descs collection). In that way, everything would be totally explicit and properly bound. What do you think of that sort of mental model? I tried, perhaps poorly, to express these ideas in #69495 (comment) and the subsequent comment.

[RichardJCai]

I'm torn. I think removing the complexity as we have it is fine, but I don't know that this the direction I'd like for us to go longer term. Maybe this is a good stepping stone towards the better future.

I think my preference would be for us to rework the interfaces such that you always have to inject a bunch of information into the internal executor to use it. Like, imagine you always have a factory hanging off of the ExecCfg and you never have a thing that you can directly issue queries off of. Then, in the context of a transaction you you have a fully constructed internal executor that is tightly bound to a bunch of your conn executor state like the transaction, session data, extra txn state (including the descs collection). In that way, everything would be totally explicit and properly bound. What do you think of that sort of mental model? I tried, perhaps poorly, to express these ideas in #69495 (comment) and the subsequent comment.

Yeah I'd argue this is a good stepping stone towards the future. In this pr, we essentially override every time we use the InternalExecutor for builtins. Going from here to using a factory hanging off ExecCfg that takes some params (session data, desc Collections, txn, state) to create a new InternalExecutor feels natural.

I'll see how viable / tricky that would be to do right now.

[rafiss]

i like the direction of this. definitely feels like an improvement.

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

---

pkg/sql/backfill.go, line 1882 at r3 (raw file):

			return ie.WithSyntheticDescriptors([]catalog.Descriptor{desc}, func() error {
				cnt, err := ie.QueryRowEx(ctx, "VERIFY INDEX", txn,
					sessiondata.NodeUserSessionDataOverride, query)

do you know when we are supposed to use the node user versus the root user?

---

pkg/sql/exec_util.go, line 1176 at r3 (raw file):

// InternalExecutorFactory is a function that produces a "session
// bound" internal executor.

does session bound in this context mean that it should always inherit session variables from the parent session?

thinking of #70888

speaking of which, @cucaroach you'd probably want to be aware of this change (at the moment still in draft state)

---

pkg/sql/planner.go, line 828 at r3 (raw file):

	opName string,
	txn *kv.Txn,
	session *sessiondata.SessionData,

nit: name it sd maybe?

---

pkg/sql/planner.go, line 840 at r3 (raw file):

	opName string,
	txn *kv.Txn,
	session *sessiondata.SessionData,

nit: name it sd

---

pkg/sql/sem/tree/eval.go, line 3248 at r3 (raw file):

// InternalRows is an iterator interface that's exposed by the internal
// executor. It provides access to the rows from a query.
type InternalRows interface {

hm, is tree the right package for InternalRows? i wonder if it'd be better to have it in a new package. well then again it makes sense to be in the same package as EvalPlanner. i'm fine to leave it here if there's no better place

anyway, would be worth adding a var _ InternalRows = rowsIterator{} assertion somewhere

[RichardJCai]

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

---

pkg/sql/backfill.go, line 1882 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

do you know when we are supposed to use the node user versus the root user?

I think root should be sufficient but I thought this fit similarly to how the other NodeUserSessionDataOverrides were used which is for "system" ops. I think backfill falls under that.

---

pkg/sql/exec_util.go, line 1176 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

does session bound in this context mean that it should always inherit session variables from the parent session?

thinking of #70888

speaking of which, @cucaroach you'd probably want to be aware of this change (at the moment still in draft state)

Yeah session data and some txn state should be passed when creating it.

---

pkg/sql/sem/tree/eval.go, line 3248 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

hm, is tree the right package for InternalRows? i wonder if it'd be better to have it in a new package. well then again it makes sense to be in the same package as EvalPlanner. i'm fine to leave it here if there's no better place

anyway, would be worth adding a var _ InternalRows = rowsIterator{} assertion somewhere

Added the var _ InternalRows = rowsIterator{}

The deps are pretty messy here, I'd like to make InternalRows standalone but the original one has colinfo.ResultColumns which has a dependency on tree and I can't find a nice way to make it work.

[RichardJCai]

Not sure why these disappeared before, they're defined in the orm_queries_bench_test.go file

[rafiss]

just some relatively small comments

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

---

pkg/sql/conn_executor.go, line 1500 at r5 (raw file):

	}

	if !ex.extraTxnState.keepDescCollectionOnClose {

should this change be done as a separate PR?

---

pkg/sql/internal.go, line 131 at r5 (raw file):

func (ie *InternalExecutor) SetExtraTxnState(extraTxnState ExtraTxnState) {
	ie.extraTxnState = extraTxnState

hm, i wonder if the extraTxnState stuff should be a different PR. I don't feel strongly, if it would be hard to separate into a different change I wouldn't worry much about it.

---

pkg/sql/planner.go, line 820 at r5 (raw file):

// QueryRowEx is like QueryRow, but allows the caller to override some session data
// fields (e.g. the user).

nit: the comment refers to QueryRow, but that isn't defined on this type

---

pkg/sql/resolve_oid.go, line 60 at r5 (raw file):

	toResolve tree.Datum,
	user security.SQLUsername,
	database string,

why did we need to add these two parameters?

---

pkg/sql/sem/tree/eval.go, line 3228 at r5 (raw file):

	ExternalWriteFile(ctx context.Context, uri string, content []byte) error

	QueryRowEx(

nit: comment this function and the next

---

pkg/sql/sem/tree/eval.go, line 3232 at r5 (raw file):

		opName string,
		txn *kv.Txn,
		session *sessiondata.SessionData,

nit: sessionData or sd instead of session

but does this actually need to be a param? won't the planner always have one of these available on it already? so we can just get it from the receiver

[RichardJCai]

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

---

pkg/sql/backfill.go, line 1566 at r6 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nvm, changed in the rebase

Done.

---

pkg/sql/backfill.go, line 1688 at r6 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nvm, changed in the rebase

Done.

---

pkg/sql/backfill.go, line 1787 at r6 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

ah i see it happened in a rebase. this seems fine then

Done.

---

pkg/sql/unsplit.go, line 110 at r6 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

this used to use root, but now doesn't have an override. does it still need to be root or node?

This actually used NoSessionDataOverride or rahter just an empty sessiondata.InternalExecutorOverride{} before - this should continue to be sufficient. (Look at first commit)

[rafiss]

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

---

pkg/sql/resolve_oid.go, line 78 at r7 (raw file):

	results, err := ie.QueryRowEx(ctx, "queryOid", txn,
		sessiondata.NoSessionDataOverride, q, toResolve)
	//results, err := ie.QueryRowEx(ctx, "queryOid", txn,

commented code (and just confirming that the user override is right?)

---

pkg/sql/unsplit.go, line 110 at r6 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

This actually used NoSessionDataOverride or rahter just an empty sessiondata.InternalExecutorOverride{} before - this should continue to be sufficient. (Look at first commit)

ah OK then i guess i got mixed up -- but now it looks like r7 is using NodeUser again?

---

pkg/bench/rttanalysis/testdata/benchmark_expectations, line 54 at r7 (raw file):

20,Grant/grant_all_on_3_tables
17,GrantRole/grant_1_role
20,GrantRole/grant_2_roles

looks like between r6 and r7 some of these disappeared again

10,InternalExecutor/has_schema_privilege_1_col
14,InternalExecutor/has_schema_privilege_2_col
18,InternalExecutor/has_schema_privilege_3_col
48,InternalExecutor/has_schema_privilege_3_schemas_3_col
72,InternalExecutor/has_schema_privilege_5_schemas_3_col

3,ORMQueries/information_schema._pg_index_position

4,ORMQueries/pg_is_other_temp_schema
4,ORMQueries/pg_is_other_temp_schema_multiple_times
2,ORMQueries/pg_my_temp_schema
2,ORMQueries/pg_my_temp_schema_multiple_times

[RichardJCai]

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

---

pkg/sql/resolve_oid.go, line 78 at r7 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

commented code (and just confirming that the user override is right?)

Yeah before this used to just be QueryRow so I think NoSessionDataOverride should work here

---

pkg/sql/unsplit.go, line 110 at r6 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

ah OK then i guess i got mixed up -- but now it looks like r7 is using NodeUser again?

Updated.

---

pkg/bench/rttanalysis/testdata/benchmark_expectations, line 54 at r7 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

looks like between r6 and r7 some of these disappeared again

10,InternalExecutor/has_schema_privilege_1_col
14,InternalExecutor/has_schema_privilege_2_col
18,InternalExecutor/has_schema_privilege_3_col
48,InternalExecutor/has_schema_privilege_3_schemas_3_col
72,InternalExecutor/has_schema_privilege_5_schemas_3_col

3,ORMQueries/information_schema._pg_index_position

4,ORMQueries/pg_is_other_temp_schema
4,ORMQueries/pg_is_other_temp_schema_multiple_times
2,ORMQueries/pg_my_temp_schema
2,ORMQueries/pg_my_temp_schema_multiple_times

Yeah I reverted all my benchmark changes since this shouldn't actually change anything if I'm not passing desc cols. Will re-add in future desc colsPPR.

[rafiss]

lgtm! i still don't really get why those benchmark changes were missing, but seems fine to add them later

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

[rafiss]

maybe a followup is to remove ExecCfg.InternalExecutor entirely?

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

[RichardJCai]

TFTR!
bors r=rafiss

[craig]

Build succeeded:

• GitHub CI (Cockroach)

[adityamaru]

@RichardJCai do you have a follow up to inject extra txn state into the IE sitting around, or should I write one up? I kinda need something similar for some IMPORT work I'm doing.

[RichardJCai]

@RichardJCai do you have a follow up to inject extra txn state into the IE sitting around, or should I write one up? I kinda need something similar for some IMPORT work I'm doing.

If you look at the reviewable on r5, I had something to inject extra txn state, however I'll probably not get to it until Tuesday, if you need it more urgently I'd recommend taking inspiration from r5 here and optionally pass in ExtraTxnState into SessionBoundInternalExecutorFactory it shouldn't be too much to add on I believe. I left it out of this PR to limit the scope of it. I'd recommend doing something like passing in optional functions into InternalExecutorFactory where the function calls takes an InternalExecutor and mutates it in some way (injecting state in this case)

[adityamaru]

I took a stab at #73193. I'm conflicted whether we should pass in a closure and leave it up to the user of the internal executor to set the appropriate fields, or if we should require the user to inject the state by having typed arguments in the factory?

[adityamaru]

Regardless of what we decide above, I think the next step as pointed out by rafi is to remove the InternalExecutor from ExecCfg entirely and force everyone to initialize a new one when they need one. This way we move closer to our eventual goal of tightly binding statement execution with the external state. I'll try putting that out next before trying to inject ExtraTxnState.