Em seg., 5 de jul. de 2021 às 12:07, Ronan Dunklau <ronan.dunk...@aiven.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