sql: do not allow pausable portals on statements containing functions #107998
Conversation
|
This change is |
There was a problem hiding this comment.
Thanks for the changes!
LGTM but would like to defer the queries team to finalize the review, as I'm not very familiar with the func exprs.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @rharding6373 and @yuzefovich)
pkg/sql/pgwire/testdata/pgtest/portals_crbugs line 371 at r1 (raw file):
{"Type":"DataRow","Values":[{"text":"(1,1)"}]} {"Type":"PortalSuspended"} {"Type":"ErrorResponse","Code":"0A000","Message":"unimplemented: the statement for a pausable portal must be a read-only SELECT query with no sub-queries or post-queries","Detail":"cannot execute a portal while a different one is open","Hint":"You have attempted to use a feature that is not yet implemented.\nSee: https://go.crdb.dev/issue-v/98911/v23.2"}
nit: we may want to update the doc and the error message saying that we don't support function expr for now.
There was a problem hiding this comment.
Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status:
-- commits line 5 at r1:
I know I suggested previously to simply prohibit functions, but perhaps this is too strong of a limitation. I wonder whether we should introduce a session setting that controls whether functions are allowed for pausable portals execution model - this will allow us to not comment out the tests but also would give an option to users to execute functions if they know functions are safe.
pkg/sql/pgwire/testdata/pgtest/multiple_active_portals line 717 at r1 (raw file):
# TODO(#107130): Unskip once non-volatile functions can be used in pausable # portals. # the function is non-volatile.
nit: did you mean to add this line?
rharding6373
force-pushed
the
20230801_portal_func_107130
branch
from
9ebffe2 to
c658060
Compare
There was a problem hiding this comment.
TFTRs! RFAL
Reviewable status:
Previously, yuzefovich (Yahor Yuzefovich) wrote…
I know I suggested previously to simply prohibit functions, but perhaps this is too strong of a limitation. I wonder whether we should introduce a session setting that controls whether functions are allowed for pausable portals execution model - this will allow us to not comment out the tests but also would give an option to users to execute functions if they know functions are safe.
Added a session setting, PTAL.
pkg/sql/pgwire/testdata/pgtest/portals_crbugs line 371 at r1 (raw file):
Previously, ZhouXing19 (Jane Xing) wrote…
nit: we may want to update the doc and the error message saying that we don't support function expr for now.
Done.
There was a problem hiding this comment.
Reviewed 9 of 9 files at r2, all commit messages.
Reviewable status:
-- commits line 18 at r2:
nit: s/sql/sql change/.
pkg/sql/conn_executor_prepare.go line 533 at r2 (raw file):
func (ex *connExecutor) addPortal( ctx context.Context, evalCtx *extendedEvalContext,
nit: let's pass *sessiondata.SessionData directly (to reduce the spread of eval context unnecessarily).
rharding6373
force-pushed
the
20230801_portal_func_107130
branch
3 times, most recently
from
14a2691 to
2282a1b
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sem/tree/stmt.go line 133 at r3 (raw file):
func (v *checkPausableVisitor) VisitPre(expr Expr) (recurse bool, newExpr Expr) { switch expr.(type) { case *FuncExpr:
I think I remember hearing that only volatile UDFs can contain mutations. (Is that true?) If that is true, could the restriction be narrowed to only disallowing pausable portals when calling volatile UDFs?
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sem/tree/stmt.go line 133 at r3 (raw file):
Previously, michae2 (Michael Erickson) wrote…
I think I remember hearing that only volatile UDFs can contain mutations. (Is that true?) If that is true, could the restriction be narrowed to only disallowing pausable portals when calling volatile UDFs?
This is true, and I opened #107130 to address this in SQL foundations. The obstacle is that at the time we try to open a pausable portal, we only have an AST and have not resolved any functions. So we don't know whether a function is volatile or not, and workaround such as allowing builtin functions that are known to be non-volatile don't work since one could have a UDF with the same name that is volatile.
Do you think it would be better to hold off merging this until we see if the issue above will be resolved in this release? If it's resolved, then we don't need this hammer.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sem/tree/stmt.go line 133 at r3 (raw file):
Previously, rharding6373 (Rachael Harding) wrote…
This is true, and I opened #107130 to address this in SQL foundations. The obstacle is that at the time we try to open a pausable portal, we only have an AST and have not resolved any functions. So we don't know whether a function is volatile or not, and workaround such as allowing builtin functions that are known to be non-volatile don't work since one could have a UDF with the same name that is volatile.
Do you think it would be better to hold off merging this until we see if the issue above will be resolved in this release? If it's resolved, then we don't need this hammer.
If we're throwing an error anyway, could we do it later (say, during optbuilder) after the pausable portal has already been opened? Does the error need to be thrown exactly when we try to open the pausable portal?
(I apologize if this is obvious, I don't have a complete grasp of pausable portals yet...)
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sem/tree/stmt.go line 133 at r3 (raw file):
Previously, michae2 (Michael Erickson) wrote…
If we're throwing an error anyway, could we do it later (say, during optbuilder) after the pausable portal has already been opened? Does the error need to be thrown exactly when we try to open the pausable portal?
(I apologize if this is obvious, I don't have a complete grasp of pausable portals yet...)
I think it'd be possible, but I'm not sure whether we want to do that. In particular, my concern is about spreading this "pausable portal" context further than it currently is. Disallowing mutations doesn't seem like a fundamental limitation that couldn't be lifted in the future - it was just out of scope for the initial version. In my mind, this "pausable portal" execution mode is strictly execution time concept, so I'd like to keep it out of the optimizer if possible. Since the feature is currently in preview and requires users to enable it explicitly, it seems ok to ask them to change another session parameter if they need builtin functions.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sem/tree/stmt.go line 133 at r3 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
I think it'd be possible, but I'm not sure whether we want to do that. In particular, my concern is about spreading this "pausable portal" context further than it currently is. Disallowing mutations doesn't seem like a fundamental limitation that couldn't be lifted in the future - it was just out of scope for the initial version. In my mind, this "pausable portal" execution mode is strictly execution time concept, so I'd like to keep it out of the optimizer if possible. Since the feature is currently in preview and requires users to enable it explicitly, it seems ok to ask them to change another session parameter if they need builtin functions.
I don't think it necessarily requires teaching the optimizer about pausable portals; maybe we could check for planFlagContainsMutation somewhere in conn_executor_exec.go?
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sem/tree/stmt.go line 133 at r3 (raw file):
Previously, michae2 (Michael Erickson) wrote…
I don't think it necessarily requires teaching the optimizer about pausable portals; maybe we could check for
planFlagContainsMutationsomewhere inconn_executor_exec.go?
Oh, that's a very nice idea, if it works (and in theory it should), we could remove this IsAllowedToPause method altogether.
We currently do not allow pausable portals when a statement may mutate data. However, functions may also mutate data. This change adds a visitor that walks the statements checked by IsAllowedToPause and checks for function expressions. If there is a function expression anywhere in the AST, the statement is not allowed to pause. Users can allow function calls in pausable portals by setting the session setting `enable_functions_in_portals` to true. This setting is false by default. When true, the pausable portal behavior is the same as before this PR. Epic: None Informs: cockroachdb#107130 Release note (sql change): This change prevents statements containing function calls from being executed in pausable portals in order to prevent race conditions if the functions modify the database. This behavior is controlled by a session setting called `enable_functions_in_portals`, which is false by default. In order to achieve the old behavior and allow statments containing functions to use portals, users should use `SET enable_functions_in_portals = true`.
rharding6373
force-pushed
the
20230801_portal_func_107130
branch
from
2282a1b to
1023d35
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sem/tree/stmt.go line 133 at r3 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
Oh, that's a very nice idea, if it works (and in theory it should), we could remove this
IsAllowedToPausemethod altogether.
I'll look into this idea today. Thanks for the discussion.
We currently do not allow pausable portals when a statement may mutate data. However, functions may also mutate data. This change adds a visitor that walks the statements checked by IsAllowedToPause and checks for function expressions. If there is a function expression anywhere in the AST, the statement is not allowed to pause.
Epic: None
Informs: #107130
Release note: none