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

From: Ranier Vilela <[email-masked]>
Date: Mon, 5 Jul 2021 11:51:59 -0300

>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:

1. Why do you moved the declaration of variable *plannode?
I think this is unnecessary, extend the scope.

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.

3. I inverted the order of check at this line, I think "!node-bounded" is
more cheap that TupleDescAttr(tupDesc, 0) ->attbyval

4. Once that you changed the order of execution, this test is unlikely that
happens, so add unlikely helps the branch.

5. I think that you add a invariant inside the loop
"if(node->is_single_val)"?
Would not be better two fors?

For you convenience, I attached a v2 version (with styles changes), please
take a look and can you repeat yours tests?

regards,
Ranier Vilela

---

From: Ronan Dunklau <[email-masked]>
Date: Mon, 05 Jul 2021 17:07:27 +0200

> I have a few suggestions and questions to your patch:

Thank you for those !
> I think this is unnecessary, extend the scope.

Sorry, I should have cleaned it up before sending.

> I think this is unnecessary too, maybe I didn't notice something.

Same as the above, thanks for the two.
> more cheap that TupleDescAttr(tupDesc, 0) ->attbyval

I'm not sure it matters since it's done once per sort but Ok
> happens, so add unlikely helps the branch.

Ok.

> Would not be better two fors?

Ok for me.

> 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 !

-- 
Ronan Dunklau

---

From: Ranier Vilela <[email-masked]>
Date: Mon, 5 Jul 2021 12:39:52 -0300

> 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