Re: Add proper planner support for ORDER BY / DISTINCT aggregates

Em seg., 5 de jul. de 2021 às 12:07, Ronan Dunklau <ronan(dot)dunklau(at)aiven(dot)io>
escreveu:

> Le lundi 5 juillet 2021, 16:51:59 CEST Ranier Vilela a écrit :
> > >Please find attached a POC patch to do just that.
> > >
> > >The switch to the single-datum tuplesort is done when there is only one
> > >attribute, it is byval (to avoid having to deal with copy of the
> >
> > references
> >
> > >everywhere) and we are not in bound mode (to also avoid having to move
> >
> > things
> >
> > >around).
> >
> > Hi, nice results!
> >
> > I have a few suggestions and questions to your patch:
>
> Thank you for those !
> >
> > 1. Why do you moved the declaration of variable *plannode?
> > I think this is unnecessary, extend the scope.
>
> Sorry, I should have cleaned it up before sending.
>
> >
> > 2. Why do you declare a new variable TupleDesc out_tuple_desc at
> > ExecInitSort?
> > I think this is unnecessary too, maybe I didn't notice something.
>
> Same as the above, thanks for the two.
> >
> > 3. I inverted the order of check at this line, I think "!node-bounded" is
> > more cheap that TupleDescAttr(tupDesc, 0) ->attbyval
>
> I'm not sure it matters since it's done once per sort but Ok
> >
> > 4. Once that you changed the order of execution, this test is unlikely
> that
> > happens, so add unlikely helps the branch.
>
> Ok.
>
> >
> > 5. I think that you add a invariant inside the loop
> > "if(node->is_single_val)"?
> > Would not be better two fors?
>
> Ok for me.
>
> >
> > For you convenience, I attached a v2 version (with styles changes),
> please
> > take a look and can you repeat yours tests?
>
> Tested it quickly, and did not see any change performance wise that cannot
> be
> attributed to noise on my laptop but it's fine.
>
Thanks for testing again.

> Thank you for the fixes !
>
You are welcome.

regards,
Ranier Vilela