Re: A potential memory leak on Merge Join when Sort node is not below Materialize node

[David Rowley]

Thanks for investigating this and finding the guilty commit.

On Thu, 29 Sept 2022 at 07:34, Tom Lane <t...@sss.pgh.pa.us> 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;