On Sun, Apr 17, 2022 at 1:48 AM Etsuro Fujita <etsuro.fuj...@gmail.com> wrote:
> On Mon, Apr 11, 2022 at 11:44 AM Zhihong Yu <z...@yugabyte.com> wrote: > > On Sun, Apr 10, 2022 at 7:41 PM Justin Pryzby <pry...@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). Cheers
