From fc08befa41fb62df543e77a40e431e69e69cff35 Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <akorotkov@postgresql.org>
Date: Mon, 25 Jul 2022 00:39:32 +0300
Subject: [PATCH 1/6] Remove Tuplesortstate.copytup function

It's currently unclear how do we split functionality between
Tuplesortstate.copytup() function and tuplesort_put*() functions.
For instance, copytup_index() and copytup_datum() return error while
tuplesort_putindextuplevalues() and tuplesort_putdatum() do their work.
This commit removes Tuplesortstate.copytup() altogether, putting the
corresponding code into tuplesort_put*().

Discussion: https://postgr.es/m/CAPpHfdvjix0Ahx-H3Jp1M2R%2B_74P-zKnGGygx4OWr%3DbUQ8BNdw%40mail.gmail.com
Author: Alexander Korotkov
Reviewed-by: Pavel Borisov, Maxim Orlov, Matthias van de Meent
Reviewed-by: Andres Freund, John Naylor
---
 src/backend/utils/sort/tuplesort.c | 330 ++++++++++++-----------------
 1 file changed, 132 insertions(+), 198 deletions(-)

diff --git a/src/backend/utils/sort/tuplesort.c b/src/backend/utils/sort/tuplesort.c
index 421afcf47d3..4812b1d9ae3 100644
--- a/src/backend/utils/sort/tuplesort.c
+++ b/src/backend/utils/sort/tuplesort.c
@@ -279,14 +279,6 @@ struct Tuplesortstate
 	 */
 	SortTupleComparator comparetup;
 
-	/*
-	 * Function to copy a supplied input tuple into palloc'd space and set up
-	 * its SortTuple representation (ie, set tuple/datum1/isnull1).  Also,
-	 * state->availMem must be decreased by the amount of space used for the
-	 * tuple copy (note the SortTuple struct itself is not counted).
-	 */
-	void		(*copytup) (Tuplesortstate *state, SortTuple *stup, void *tup);
-
 	/*
 	 * Function to write a stored tuple onto tape.  The representation of the
 	 * tuple on tape need not be the same as it is in memory; requirements on
@@ -549,7 +541,6 @@ struct Sharedsort
 	} while(0)
 
 #define COMPARETUP(state,a,b)	((*(state)->comparetup) (a, b, state))
-#define COPYTUP(state,stup,tup) ((*(state)->copytup) (state, stup, tup))
 #define WRITETUP(state,tape,stup)	((*(state)->writetup) (state, tape, stup))
 #define READTUP(state,stup,tape,len) ((*(state)->readtup) (state, stup, tape, len))
 #define LACKMEM(state)		((state)->availMem < 0 && !(state)->slabAllocatorUsed)
@@ -600,10 +591,7 @@ struct Sharedsort
  * a lot better than what we were doing before 7.3.  As of 9.6, a
  * separate memory context is used for caller passed tuples.  Resetting
  * it at certain key increments significantly ameliorates fragmentation.
- * Note that this places a responsibility on copytup routines to use the
- * correct memory context for these tuples (and to not use the reset
- * context for anything whose lifetime needs to span multiple external
- * sort runs).  readtup routines use the slab allocator (they cannot use
+ * readtup routines use the slab allocator (they cannot use
  * the reset context because it gets deleted at the point that merging
  * begins).
  */
@@ -643,14 +631,12 @@ static void markrunend(LogicalTape *tape);
 static void *readtup_alloc(Tuplesortstate *state, Size tuplen);
 static int	comparetup_heap(const SortTuple *a, const SortTuple *b,
 							Tuplesortstate *state);
-static void copytup_heap(Tuplesortstate *state, SortTuple *stup, void *tup);
 static void writetup_heap(Tuplesortstate *state, LogicalTape *tape,
 						  SortTuple *stup);
 static void readtup_heap(Tuplesortstate *state, SortTuple *stup,
 						 LogicalTape *tape, unsigned int len);
 static int	comparetup_cluster(const SortTuple *a, const SortTuple *b,
 							   Tuplesortstate *state);
-static void copytup_cluster(Tuplesortstate *state, SortTuple *stup, void *tup);
 static void writetup_cluster(Tuplesortstate *state, LogicalTape *tape,
 							 SortTuple *stup);
 static void readtup_cluster(Tuplesortstate *state, SortTuple *stup,
@@ -659,14 +645,12 @@ static int	comparetup_index_btree(const SortTuple *a, const SortTuple *b,
 								   Tuplesortstate *state);
 static int	comparetup_index_hash(const SortTuple *a, const SortTuple *b,
 								  Tuplesortstate *state);
-static void copytup_index(Tuplesortstate *state, SortTuple *stup, void *tup);
 static void writetup_index(Tuplesortstate *state, LogicalTape *tape,
 						   SortTuple *stup);
 static void readtup_index(Tuplesortstate *state, SortTuple *stup,
 						  LogicalTape *tape, unsigned int len);
 static int	comparetup_datum(const SortTuple *a, const SortTuple *b,
 							 Tuplesortstate *state);
-static void copytup_datum(Tuplesortstate *state, SortTuple *stup, void *tup);
 static void writetup_datum(Tuplesortstate *state, LogicalTape *tape,
 						   SortTuple *stup);
 static void readtup_datum(Tuplesortstate *state, SortTuple *stup,
@@ -1059,7 +1043,6 @@ tuplesort_begin_heap(TupleDesc tupDesc,
 								PARALLEL_SORT(state));
 
 	state->comparetup = comparetup_heap;
-	state->copytup = copytup_heap;
 	state->writetup = writetup_heap;
 	state->readtup = readtup_heap;
 	state->haveDatum1 = true;
@@ -1135,7 +1118,6 @@ tuplesort_begin_cluster(TupleDesc tupDesc,
 								PARALLEL_SORT(state));
 
 	state->comparetup = comparetup_cluster;
-	state->copytup = copytup_cluster;
 	state->writetup = writetup_cluster;
 	state->readtup = readtup_cluster;
 	state->abbrevNext = 10;
@@ -1240,7 +1222,6 @@ tuplesort_begin_index_btree(Relation heapRel,
 								PARALLEL_SORT(state));
 
 	state->comparetup = comparetup_index_btree;
-	state->copytup = copytup_index;
 	state->writetup = writetup_index;
 	state->readtup = readtup_index;
 	state->abbrevNext = 10;
@@ -1317,7 +1298,6 @@ tuplesort_begin_index_hash(Relation heapRel,
 	state->nKeys = 1;			/* Only one sort column, the hash code */
 
 	state->comparetup = comparetup_index_hash;
-	state->copytup = copytup_index;
 	state->writetup = writetup_index;
 	state->readtup = readtup_index;
 	state->haveDatum1 = true;
@@ -1358,7 +1338,6 @@ tuplesort_begin_index_gist(Relation heapRel,
 	state->nKeys = IndexRelationGetNumberOfKeyAttributes(indexRel);
 
 	state->comparetup = comparetup_index_btree;
-	state->copytup = copytup_index;
 	state->writetup = writetup_index;
 	state->readtup = readtup_index;
 	state->haveDatum1 = true;
@@ -1422,7 +1401,6 @@ tuplesort_begin_datum(Oid datumType, Oid sortOperator, Oid sortCollation,
 								PARALLEL_SORT(state));
 
 	state->comparetup = comparetup_datum;
-	state->copytup = copytup_datum;
 	state->writetup = writetup_datum;
 	state->readtup = readtup_datum;
 	state->abbrevNext = 10;
@@ -1839,14 +1817,75 @@ noalloc:
 void
 tuplesort_puttupleslot(Tuplesortstate *state, TupleTableSlot *slot)
 {
-	MemoryContext oldcontext = MemoryContextSwitchTo(state->sortcontext);
+	MemoryContext oldcontext = MemoryContextSwitchTo(state->tuplecontext);
 	SortTuple	stup;
+	Datum		original;
+	MinimalTuple tuple;
+	HeapTupleData htup;
 
-	/*
-	 * Copy the given tuple into memory we control, and decrease availMem.
-	 * Then call the common code.
-	 */
-	COPYTUP(state, &stup, (void *) slot);
+	/* copy the tuple into sort storage */
+	tuple = ExecCopySlotMinimalTuple(slot);
+	stup.tuple = (void *) tuple;
+	USEMEM(state, GetMemoryChunkSpace(tuple));
+	/* set up first-column key value */
+	htup.t_len = tuple->t_len + MINIMAL_TUPLE_OFFSET;
+	htup.t_data = (HeapTupleHeader) ((char *) tuple - MINIMAL_TUPLE_OFFSET);
+	original = heap_getattr(&htup,
+							state->sortKeys[0].ssup_attno,
+							state->tupDesc,
+							&stup.isnull1);
+
+	MemoryContextSwitchTo(state->sortcontext);
+
+	if (!state->sortKeys->abbrev_converter || stup.isnull1)
+	{
+		/*
+		 * Store ordinary Datum representation, or NULL value.  If there is a
+		 * converter it won't expect NULL values, and cost model is not
+		 * required to account for NULL, so in that case we avoid calling
+		 * converter and just set datum1 to zeroed representation (to be
+		 * consistent, and to support cheap inequality tests for NULL
+		 * abbreviated keys).
+		 */
+		stup.datum1 = original;
+	}
+	else if (!consider_abort_common(state))
+	{
+		/* Store abbreviated key representation */
+		stup.datum1 = state->sortKeys->abbrev_converter(original,
+														state->sortKeys);
+	}
+	else
+	{
+		/* Abort abbreviation */
+		int			i;
+
+		stup.datum1 = original;
+
+		/*
+		 * Set state to be consistent with never trying abbreviation.
+		 *
+		 * Alter datum1 representation in already-copied tuples, so as to
+		 * ensure a consistent representation (current tuple was just
+		 * handled).  It does not matter if some dumped tuples are already
+		 * sorted on tape, since serialized tuples lack abbreviated keys
+		 * (TSS_BUILDRUNS state prevents control reaching here in any case).
+		 */
+		for (i = 0; i < state->memtupcount; i++)
+		{
+			SortTuple  *mtup = &state->memtuples[i];
+
+			htup.t_len = ((MinimalTuple) mtup->tuple)->t_len +
+				MINIMAL_TUPLE_OFFSET;
+			htup.t_data = (HeapTupleHeader) ((char *) mtup->tuple -
+											 MINIMAL_TUPLE_OFFSET);
+
+			mtup->datum1 = heap_getattr(&htup,
+										state->sortKeys[0].ssup_attno,
+										state->tupDesc,
+										&mtup->isnull1);
+		}
+	}
 
 	puttuple_common(state, &stup);
 
@@ -1861,14 +1900,74 @@ tuplesort_puttupleslot(Tuplesortstate *state, TupleTableSlot *slot)
 void
 tuplesort_putheaptuple(Tuplesortstate *state, HeapTuple tup)
 {
-	MemoryContext oldcontext = MemoryContextSwitchTo(state->sortcontext);
 	SortTuple	stup;
+	Datum		original;
+	MemoryContext oldcontext = MemoryContextSwitchTo(state->tuplecontext);
+
+	/* copy the tuple into sort storage */
+	tup = heap_copytuple(tup);
+	stup.tuple = (void *) tup;
+	USEMEM(state, GetMemoryChunkSpace(tup));
+
+	MemoryContextSwitchTo(state->sortcontext);
 
 	/*
-	 * Copy the given tuple into memory we control, and decrease availMem.
-	 * Then call the common code.
+	 * set up first-column key value, and potentially abbreviate, if it's a
+	 * simple column
 	 */
-	COPYTUP(state, &stup, (void *) tup);
+	if (state->haveDatum1)
+	{
+		original = heap_getattr(tup,
+								state->indexInfo->ii_IndexAttrNumbers[0],
+								state->tupDesc,
+								&stup.isnull1);
+
+		if (!state->sortKeys->abbrev_converter || stup.isnull1)
+		{
+			/*
+			 * Store ordinary Datum representation, or NULL value.  If there is a
+			 * converter it won't expect NULL values, and cost model is not
+			 * required to account for NULL, so in that case we avoid calling
+			 * converter and just set datum1 to zeroed representation (to be
+			 * consistent, and to support cheap inequality tests for NULL
+			 * abbreviated keys).
+			 */
+			stup.datum1 = original;
+		}
+		else if (!consider_abort_common(state))
+		{
+			/* Store abbreviated key representation */
+			stup.datum1 = state->sortKeys->abbrev_converter(original,
+															state->sortKeys);
+		}
+		else
+		{
+			/* Abort abbreviation */
+			int			i;
+
+			stup.datum1 = original;
+
+			/*
+			 * Set state to be consistent with never trying abbreviation.
+			*
+			* Alter datum1 representation in already-copied tuples, so as to
+			* ensure a consistent representation (current tuple was just
+			* handled).  It does not matter if some dumped tuples are already
+			* sorted on tape, since serialized tuples lack abbreviated keys
+			* (TSS_BUILDRUNS state prevents control reaching here in any case).
+			*/
+			for (i = 0; i < state->memtupcount; i++)
+			{
+				SortTuple  *mtup = &state->memtuples[i];
+
+				tup = (HeapTuple) mtup->tuple;
+				mtup->datum1 = heap_getattr(tup,
+											state->indexInfo->ii_IndexAttrNumbers[0],
+											state->tupDesc,
+											&mtup->isnull1);
+			}
+		}
+	}
 
 	puttuple_common(state, &stup);
 
@@ -3947,84 +4046,6 @@ comparetup_heap(const SortTuple *a, const SortTuple *b, Tuplesortstate *state)
 	return 0;
 }
 
-static void
-copytup_heap(Tuplesortstate *state, SortTuple *stup, void *tup)
-{
-	/*
-	 * We expect the passed "tup" to be a TupleTableSlot, and form a
-	 * MinimalTuple using the exported interface for that.
-	 */
-	TupleTableSlot *slot = (TupleTableSlot *) tup;
-	Datum		original;
-	MinimalTuple tuple;
-	HeapTupleData htup;
-	MemoryContext oldcontext = MemoryContextSwitchTo(state->tuplecontext);
-
-	/* copy the tuple into sort storage */
-	tuple = ExecCopySlotMinimalTuple(slot);
-	stup->tuple = (void *) tuple;
-	USEMEM(state, GetMemoryChunkSpace(tuple));
-	/* set up first-column key value */
-	htup.t_len = tuple->t_len + MINIMAL_TUPLE_OFFSET;
-	htup.t_data = (HeapTupleHeader) ((char *) tuple - MINIMAL_TUPLE_OFFSET);
-	original = heap_getattr(&htup,
-							state->sortKeys[0].ssup_attno,
-							state->tupDesc,
-							&stup->isnull1);
-
-	MemoryContextSwitchTo(oldcontext);
-
-	if (!state->sortKeys->abbrev_converter || stup->isnull1)
-	{
-		/*
-		 * Store ordinary Datum representation, or NULL value.  If there is a
-		 * converter it won't expect NULL values, and cost model is not
-		 * required to account for NULL, so in that case we avoid calling
-		 * converter and just set datum1 to zeroed representation (to be
-		 * consistent, and to support cheap inequality tests for NULL
-		 * abbreviated keys).
-		 */
-		stup->datum1 = original;
-	}
-	else if (!consider_abort_common(state))
-	{
-		/* Store abbreviated key representation */
-		stup->datum1 = state->sortKeys->abbrev_converter(original,
-														 state->sortKeys);
-	}
-	else
-	{
-		/* Abort abbreviation */
-		int			i;
-
-		stup->datum1 = original;
-
-		/*
-		 * Set state to be consistent with never trying abbreviation.
-		 *
-		 * Alter datum1 representation in already-copied tuples, so as to
-		 * ensure a consistent representation (current tuple was just
-		 * handled).  It does not matter if some dumped tuples are already
-		 * sorted on tape, since serialized tuples lack abbreviated keys
-		 * (TSS_BUILDRUNS state prevents control reaching here in any case).
-		 */
-		for (i = 0; i < state->memtupcount; i++)
-		{
-			SortTuple  *mtup = &state->memtuples[i];
-
-			htup.t_len = ((MinimalTuple) mtup->tuple)->t_len +
-				MINIMAL_TUPLE_OFFSET;
-			htup.t_data = (HeapTupleHeader) ((char *) mtup->tuple -
-											 MINIMAL_TUPLE_OFFSET);
-
-			mtup->datum1 = heap_getattr(&htup,
-										state->sortKeys[0].ssup_attno,
-										state->tupDesc,
-										&mtup->isnull1);
-		}
-	}
-}
-
 static void
 writetup_heap(Tuplesortstate *state, LogicalTape *tape, SortTuple *stup)
 {
@@ -4193,79 +4214,6 @@ comparetup_cluster(const SortTuple *a, const SortTuple *b,
 	return 0;
 }
 
-static void
-copytup_cluster(Tuplesortstate *state, SortTuple *stup, void *tup)
-{
-	HeapTuple	tuple = (HeapTuple) tup;
-	Datum		original;
-	MemoryContext oldcontext = MemoryContextSwitchTo(state->tuplecontext);
-
-	/* copy the tuple into sort storage */
-	tuple = heap_copytuple(tuple);
-	stup->tuple = (void *) tuple;
-	USEMEM(state, GetMemoryChunkSpace(tuple));
-
-	MemoryContextSwitchTo(oldcontext);
-
-	/*
-	 * set up first-column key value, and potentially abbreviate, if it's a
-	 * simple column
-	 */
-	if (!state->haveDatum1)
-		return;
-
-	original = heap_getattr(tuple,
-							state->indexInfo->ii_IndexAttrNumbers[0],
-							state->tupDesc,
-							&stup->isnull1);
-
-	if (!state->sortKeys->abbrev_converter || stup->isnull1)
-	{
-		/*
-		 * Store ordinary Datum representation, or NULL value.  If there is a
-		 * converter it won't expect NULL values, and cost model is not
-		 * required to account for NULL, so in that case we avoid calling
-		 * converter and just set datum1 to zeroed representation (to be
-		 * consistent, and to support cheap inequality tests for NULL
-		 * abbreviated keys).
-		 */
-		stup->datum1 = original;
-	}
-	else if (!consider_abort_common(state))
-	{
-		/* Store abbreviated key representation */
-		stup->datum1 = state->sortKeys->abbrev_converter(original,
-														 state->sortKeys);
-	}
-	else
-	{
-		/* Abort abbreviation */
-		int			i;
-
-		stup->datum1 = original;
-
-		/*
-		 * Set state to be consistent with never trying abbreviation.
-		 *
-		 * Alter datum1 representation in already-copied tuples, so as to
-		 * ensure a consistent representation (current tuple was just
-		 * handled).  It does not matter if some dumped tuples are already
-		 * sorted on tape, since serialized tuples lack abbreviated keys
-		 * (TSS_BUILDRUNS state prevents control reaching here in any case).
-		 */
-		for (i = 0; i < state->memtupcount; i++)
-		{
-			SortTuple  *mtup = &state->memtuples[i];
-
-			tuple = (HeapTuple) mtup->tuple;
-			mtup->datum1 = heap_getattr(tuple,
-										state->indexInfo->ii_IndexAttrNumbers[0],
-										state->tupDesc,
-										&mtup->isnull1);
-		}
-	}
-}
-
 static void
 writetup_cluster(Tuplesortstate *state, LogicalTape *tape, SortTuple *stup)
 {
@@ -4512,13 +4460,6 @@ comparetup_index_hash(const SortTuple *a, const SortTuple *b,
 	return 0;
 }
 
-static void
-copytup_index(Tuplesortstate *state, SortTuple *stup, void *tup)
-{
-	/* Not currently needed */
-	elog(ERROR, "copytup_index() should not be called");
-}
-
 static void
 writetup_index(Tuplesortstate *state, LogicalTape *tape, SortTuple *stup)
 {
@@ -4583,13 +4524,6 @@ comparetup_datum(const SortTuple *a, const SortTuple *b, Tuplesortstate *state)
 	return compare;
 }
 
-static void
-copytup_datum(Tuplesortstate *state, SortTuple *stup, void *tup)
-{
-	/* Not currently needed */
-	elog(ERROR, "copytup_datum() should not be called");
-}
-
 static void
 writetup_datum(Tuplesortstate *state, LogicalTape *tape, SortTuple *stup)
 {
-- 
2.24.3 (Apple Git-128)

