opt: factor out common panic recovery into helper function #91752
Conversation
In the optimizer we use the convention of panic-based error handling for a performance advantage. This means at the boundaries we must remember to recover and return errors instead of letting panics escape. `sql.(*optPlanningCtx).makeQueryIndexRecommendation` calls some optimizer helper functions in addition to calling `xform.(*Optimizer).Optimize`. While `Optimize` has the panic-recovery code, these helper functions do not, so we need to add panic recovery to `makeQueryIndexRecommendation` as well. Fixes: cockroachdb#88246 Release note: None
In the optimizer we have the convention of using panics for error propagation, to avoid adding error handling code everywhere in deeply recursive call stacks. This is safe because the optimizer is single- threaded and not manipulating locks, shared state, etc. But it means that wherever we call into the optimizer we need to add panic recovery. We were duplicating the same basic block of panic recovery code in all these places. Factor it into a "catch" function, somewhat similar to the vectorized engine's `CatchVectorizedRuntimeError`.` Assists: cockroachdb#88246 Release note: None
|
This change is |
|
First commit is #91457 and can be ignored. |
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball, @RaduBerinde, and @yuzefovich)
pkg/sql/opt/catch.go line 25 at r2 (raw file):
// string which does not implement error. So in this case we suspect we are // not able to recover, and must crash. panic(r)
(This comes from #38570 (review) FYI.)
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1.
Reviewable status:
|
Unfortunately this approach is broken: https://go.dev/play/p/sXjrTeXt4G9 The call to
I think I'll probably do (2). |
|
Drive-by: users and TSE are frequently thrown by seeing "Error" in stack traces that aren't erroring. We may wish to rename those functions to not contain "Error", or instead use your strategy #1. |
Great point, thank you! I'll rename the function so it sounds benign. |
|
How would people feel about a pattern like this one? Closer to the panic-catching in the vectorized engine. |
In the optimizer we have the convention of using panics for error propagation, to avoid adding error handling code everywhere in deeply recursive call stacks. This is safe because the optimizer is single-threaded and not manipulating locks, shared state, etc. But it means that wherever we call into the optimizer we need to add panic recovery.
We were duplicating the same basic block of panic recovery code in all these places. Factor it into a "catch" function, somewhat similar to the vectorized engine's
CatchVectorizedRuntimeError.`Assists: #88246
Release note: None