Thanks for investigating this and finding the guilty commit.
On Thu, 29 Sept 2022 at 07:34, Tom Lane <[email protected]> wrote:
> After looking at that for a little while, I wonder if we shouldn't
> fix this by restricting the Datum-sort path to be used only with
> pass-by-value data types. That'd require only a minor addition
> to the new logic in ExecInitSort.
I'm also wondering if that's the best fix given the timing of this discovery.
> The alternative of inserting a pfree of the old value would complicate
> the code nontrivially, I think, and really it would necessitate a
> complete performance re-test. I'm wondering if the claimed speedup
> for pass-by-ref types wasn't fictional and based on skipping the
> required pfrees. Besides, if you think this code is hot enough that
> you don't want to add a test-and-branch per tuple (a claim I also
> doubt, BTW) then you probably don't want to add such overhead into
> the pass-by-value case where the speedup is clear.
I'm wondering if the best way to fix it if doing it that way would be
to invent tuplesort_getdatum_nocopy() which would be the same as
tuplesort_getdatum() except it wouldn't do the datumCopy for byref
types. It looks like tuplesort_gettupleslot() when copy==false just
directly stores the MinimalTuple that's in stup.tuple and shouldFree
is set to false.
Going by [1], it looks like I saw gains in test 6, which was a byref
Datum. Skipping the datumCopy() I imagine could only make the gains
slightly higher on that. That puts me a bit more on the fence about
the best fix for PG15.
I've attached a patch to restrict the optimisation to byval types in
the meantime.
David
[1]
https://www.postgresql.org/message-id/CAApHDvrWV%3Dv0qKsC9_BHqhCn9TusrNvCaZDz77StCO--fmgbKA%40mail.gmail.com
diff --git a/src/backend/executor/nodeSort.c b/src/backend/executor/nodeSort.c
index 3c28d60c3e..740ad37717 100644
--- a/src/backend/executor/nodeSort.c
+++ b/src/backend/executor/nodeSort.c
@@ -220,6 +220,7 @@ SortState *
ExecInitSort(Sort *node, EState *estate, int eflags)
{
SortState *sortstate;
+ TupleDesc outerTupDesc;
SO1_printf("ExecInitSort: %s\n",
"initializing sort node");
@@ -274,11 +275,13 @@ ExecInitSort(Sort *node, EState *estate, int eflags)
ExecInitResultTupleSlotTL(&sortstate->ss.ps, &TTSOpsMinimalTuple);
sortstate->ss.ps.ps_ProjInfo = NULL;
+ outerTupDesc = ExecGetResultType(outerPlanState(sortstate));
+
/*
- * We perform a Datum sort when we're sorting just a single column,
+ * We perform a Datum sort when we're sorting just a single byval
column,
* otherwise we perform a tuple sort.
*/
- if (ExecGetResultType(outerPlanState(sortstate))->natts == 1)
+ if (outerTupDesc->natts == 1 && outerTupDesc->attrs[0].attbyval)
sortstate->datumSort = true;
else
sortstate->datumSort = false;
