sql: legacy InternalExecutor interface is dangerous; we should migrate away

[andreimatei]

Historically, the InternalExecutor (and the stuff that it replaced before it) had an interface like InternalExecutor.Query(stmt). The respective query would be executed as root if no user had been previously configured on the respective executor instance, or as that configured user otherwise. This is dangerous since queries that weren't intending to run as root could get root silently through programming errors where their InternalExecutor isn't set to a user. The reverse situation where something wanted root but starts getting something else is also possible.

#44170 introduces a set of new functions - QueryEx(), ExecEx() etc that don't have this behavior. They take a user explicitly, and if none is given they fall back to what had been previously set on the executor. If nothing had been previously set, they error out. In other words, they make it explicit if the caller expects to control the user of if it expects to inherit the user.
We should migrate everybody to this new world and remove the old interface. #44170 converts a bunch of things, but not everything.

In particular, there's a class of callers with a problem. Everything in the sem/tree package (particularly, the SQL builtin functions) can't use the new interface. They use the InternalExecutor through a subset of the interface: tree.InternalExecutor. They can't import sql.InternalExecutor, or even sqlutil.InternalExecutor (the latter being supposed to be used by people who can't import sql). The new functions can't be made part of tree.InternalExecutor because they have a sqlbase argument, and sem/tree can't even depend on sqlbase...
I think the builtins don't belong in the sem/tree package; at least not if sqlbase depends on sem/tree.

@otan you want it?
cc @knz

Jira issue: CRDB-5244

[otan]

i'm going to put it on our team dashboard so it doesn't get lost, but i'm not going to have time to actively work on this for a bit (so will unassign for now).

the sql import order thing i have also recently encountered, so i do have a minor interest in solving this down the line

[rafiss]

@yuzefovich i believe this is addressed by all the work you've done recently. want to close this, or re-classify this issue?

[yuzefovich]

No, I don't think it has been addressed. The recent work was to make the internal executor run in a streaming fashion (i.e. no buffering of all rows before returning to the caller), but this issue is about using the correct user in order to initiate the query. sql.InternalExecutor object and the corresponding sqlutil.InternalExecutor interface still have a lot of duplicated methods (e.g. Exec and ExecEx, the former being deprecated since it silently uses the root), so we need to refactor the code to only use *Ex methods and remove the unsafe ones as much as possible. The current issue tracks that work.

[github-actions]

We have marked this issue as stale because it has been inactive for
18 months. If this issue is still relevant, removing the stale label
or adding a comment will keep it active. Otherwise, we'll close it in
10 days to keep the issue queue tidy. Thank you for your contribution
to CockroachDB!