-
Notifications
You must be signed in to change notification settings - Fork 4.1k
sql: legacy InternalExecutor interface is dangerous; we should migrate away #44223
Description
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.
Jira issue: CRDB-5244
Metadata
Metadata
Assignees
Labels
Type
Projects
Status