Hi,
On Sun, Apr 17, 2022 at 7:30 PM Zhihong Yu <zyu@yugabyte.com> wrote:
> On Sun, Apr 17, 2022 at 1:48 AM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
>> I think we might support more cases in the switch statement in the
>> future. My concern about your proposal is that it might make it hard
>> to add new cases to the statement. I agree that what I proposed has a
>> bit of redundant code, but writing code inside each case independently
>> would make it easy to add them, making code consistent across branches
>> and thus making back-patching easy.
> When a new case arises where the plan is not a Result node, this func can be rewritten.
> If there is only one such new case, the check at the beginning of the func can be tuned to exclude that case.
Sorry, I don't agree with you.
> I still think the check should be lifted to the beginning of the func (given the current cases).
The given path isn't limited to SubqueryScanPath, ForeignPath and
ProjectionPath, so another concern is extra cycles needed when the
path is other path type that is projection-capable (e.g., Path for
sequential scan, IndexPath, NestPath, ...). Assume that the given
path is a Path (that doesn't contain pseudoconstant quals). In that
case the given SeqScan plan node wouldn't contain a gating Result
node, so if we put the if test at the top of the function, we need to
execute not only the test but the switch statement for the given
path/plan nodes. But if we put the if test inside each case block, we
only need to execute the switch statement, without executing the test.
In the latter case I think we can save cycles for normal cases.
In short: I don't think it's a great idea to put the if test at the
top of the function.
Best regards,
Etsuro Fujita
