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

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.

Thank you for the fixes !