opt: remove panics in execbuilder

[mgartner]

This commit replaces panics in execbuilder with returned errors. This
is safer because execbuilder functions are invoked outside the
panic-recoverable Optimizer.Optimize function.

Informs #98786

Release note: None

[blathers-crl]

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[cockroach-teamcity]

This change is 

[mgartner]

I've added the backport label because I think this reduces the risk of uncaught panics, but let me know if you disagree.

[yuzefovich]

Do you envision that we'll both keep the existing panic-catching mechanism where it is already present and also prohibit the usage of error propagation via panics? Should we still then introduce panic injection as Drew's PR does? Should we introduce a linter to prohibit the usage of panics in some optimizer packages?

I don't have too strong of an opinion here, but in the vectorized engine I think the panic-catching mechanism of error propagation worked well for us (in comparison to row-by-row engine we're more likely to return an internal error in some unexpected state rather than crash), so I'd lean towards flushing out all the places where Optimize is invoked. It took us some time to do that in the vectorized engine, but I think we now have panic-catches in all (or at least in the most important) places, so personally I'd put more focus on flushing those callsites out.

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball)

[DrewKimball]

If we decide to go with this method.

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @mgartner)

[mgartner]

Do you envision that we'll both keep the existing panic-catching mechanism where it is already present and also prohibit the usage of error propagation via panics? Should we still then introduce panic injection as Drew's PR does? Should we introduce a linter to prohibit the usage of panics in some optimizer packages?

I don't envision getting rid of most of the panics or panic-catching within the optimizer. But we need to have a more confident understand and respect for which functions can panic and which cannot.

All of execbuilder lives outside the panic-catching of the Optimize function, so it's really not safe to be panicking here - unless we add new panic-catchers. Because execbuilder already returns errors from the Build function, and there were only a handful of panics, it seemed easier and less error-prone to turn all execbuilder panics into errors, than to try to add panic catching.

[mgartner]

I think this is orthogonal to Drew's panic injector, which will help us find uncaught panics in other parts of the optimizer.

[yuzefovich]

I don't envision getting rid of most of the panics or panic-catching within the optimizer. But we need to have a more confident understand and respect for which functions can panic and which cannot.

All of execbuilder lives outside the panic-catching of the Optimize function, so it's really not safe to be panicking here - unless we add new panic-catchers. Because execbuilder already returns errors from the Build function, and there were only a handful of panics, it seemed easier and less error-prone to turn all execbuilder panics into errors, than to try to add panic catching.

Ok, SGTM.

[mgartner]

bors r+

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)