On Mon, Apr 11, 2022 at 11:44 AM Zhihong Yu <zyu@yugabyte.com> wrote:
> On Sun, Apr 10, 2022 at 7:41 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
>> On Sun, Apr 10, 2022 at 06:46:25AM -0700, Zhihong Yu wrote:
>> > Looking at the second hunk of the patch:
>> > FdwRoutine *fdwroutine = path->parent->fdwroutine;
>> > ...
>> > + if (IsA(plan, Result))
>> > + return false;
>> >
>> > It seems the check of whether plan is a Result node can be lifted ahead of
>> > the switch statement (i.e. to the beginning of mark_async_capable_plan).
>> >
>> > This way, we don't have to check for every case in the switch statement.
>>
>> I think you misread it - the other branch says: if (*not* IsA())
>>
> No, I didn't misread:
>
> if (!IsA(plan, Result) &&
> mark_async_capable_plan(plan,
> ((ProjectionPath *) path)->subpath))
> return true;
> return false;
>
> If the plan is Result node, false would be returned.
> So the check can be lifted to the beginning of the func.
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.
Thanks for reviewing!
Best regards,
Etsuro Fujita
