Be smarter about freeing tuples during tuplesorts

During dumptuples() the call to writetuple() would pfree any non-null
tuple.  This was quite wasteful as this happens just before we perform a
reset of the context which stores all of those tuples.

It seems to make sense to do a bit of a code refactor to make this work,
so here we just get rid of the writetuple function and adjust the WRITETUP
macro to call the state's writetup function.  The WRITETUP usage in
mergeonerun() always has state->slabAllocatorUsed == true, so writetuple()
would never free the tuple or do any memory accounting.  The only call
path that needs memory accounting done is in dumptuples(), so let's just
do it manually there.

In passing, let's get rid of the state->memtupcount-- code that counts the
memtupcount down to 0 one tuple at a time inside the loop.  That seems to
be a rather inefficient way to set memtupcount to 0, so let's just zero it
after the loop instead.

Author: David Rowley
Discussion: https://postgr.es/m/CAApHDvqZXoDCyrfCzZJR0-xH+7_q+GgitcQiYXUjRani7h4j8Q@mail.gmail.com

diff --git a/src/backend/utils/sort/tuplesort.c b/src/backend/utils/sort/tuplesort.c
index 66950983e68e58764ae5d6b0936ecbc9ffbcdb26..416f02ba3cb4e73132775abe1bcb8210ad530db1 100644 (file)
--- a/src/backend/utils/sort/tuplesort.c
+++ b/src/backend/utils/sort/tuplesort.c
@@ -395,7 +395,7 @@ struct Sharedsort
 
 #define REMOVEABBREV(state,stup,count) ((*(state)->base.removeabbrev) (state, stup, count))
 #define COMPARETUP(state,a,b)  ((*(state)->base.comparetup) (a, b, state))
-#define WRITETUP(state,tape,stup)      (writetuple(state, tape, stup))
+#define WRITETUP(state,tape,stup)      ((*(state)->base.writetup) (state, tape, stup))
 #define READTUP(state,stup,tape,len) ((*(state)->base.readtup) (state, stup, tape, len))
 #define FREESTATE(state)       ((state)->base.freestate ? (*(state)->base.freestate) (state) : (void) 0)
 #define LACKMEM(state)         ((state)->availMem < 0 && !(state)->slabAllocatorUsed)
@@ -453,8 +453,6 @@ struct Sharedsort
 
 
 static void tuplesort_begin_batch(Tuplesortstate *state);
-static void writetuple(Tuplesortstate *state, LogicalTape *tape,
-                                          SortTuple *stup);
 static bool consider_abort_common(Tuplesortstate *state);
 static void inittapes(Tuplesortstate *state, bool mergeruns);
 static void inittapestate(Tuplesortstate *state, int maxTapes);
@@ -1339,24 +1337,6 @@ tuplesort_puttuple_common(Tuplesortstate *state, SortTuple *tuple, bool useAbbre
        MemoryContextSwitchTo(oldcontext);
 }
 
-/*
- * Write a stored tuple onto tape.  Unless the slab allocator is
- * used, after writing the tuple, pfree() the out-of-line data (not the
- * SortTuple struct!), and increase state->availMem by the amount of
- * memory space thereby released.
- */
-static void
-writetuple(Tuplesortstate *state, LogicalTape *tape, SortTuple *stup)
-{
-       state->base.writetup(state, tape, stup);
-
-       if (!state->slabAllocatorUsed && stup->tuple)
-       {
-               FREEMEM(state, GetMemoryChunkSpace(stup->tuple));
-               pfree(stup->tuple);
-       }
-}
-
 static bool
 consider_abort_common(Tuplesortstate *state)
 {
@@ -2260,6 +2240,8 @@ mergeonerun(Tuplesortstate *state)
         */
        beginmerge(state);
 
+       Assert(state->slabAllocatorUsed);
+
        /*
         * Execute merge by repeatedly extracting lowest tuple in heap, writing it
         * out, and replacing it with next tuple from same tape (if there is
@@ -2418,10 +2400,20 @@ dumptuples(Tuplesortstate *state, bool alltuples)
        memtupwrite = state->memtupcount;
        for (i = 0; i < memtupwrite; i++)
        {
-               WRITETUP(state, state->destTape, &state->memtuples[i]);
-               state->memtupcount--;
+               SortTuple  *stup = &state->memtuples[i];
+
+               WRITETUP(state, state->destTape, stup);
+
+               /*
+                * Account for freeing the tuple, but no need to do the actual pfree
+                * since the tuplecontext is being reset after the loop.
+                */
+               if (stup->tuple != NULL)
+                       FREEMEM(state, GetMemoryChunkSpace(stup->tuple));
        }
 
+       state->memtupcount = 0;
+
        /*
         * Reset tuple memory.  We've freed all of the tuples that we previously
         * allocated.  It's important to avoid fragmentation when there is a stark