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
Hi,
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.
I still think the check should be lifted to the beginning of the func (given the current cases).
