On Tue, Apr 19, 2022 at 2:01 AM Etsuro Fujita <etsuro.fuj...@gmail.com> wrote:
> Hi, > > On Sun, Apr 17, 2022 at 7:30 PM Zhihong Yu <z...@yugabyte.com> wrote: > > On Sun, Apr 17, 2022 at 1:48 AM Etsuro Fujita <etsuro.fuj...@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 > Hi, It is okay to keep the formation in your patch. Cheers
