Re: TupleTableSlot abstraction

Hi,

On 2018-08-20 19:51:33 +0530, Ashutosh Bapat wrote:
> Sorry, forgot about that. Here's the patch set with that addressed.

Btw, you attach files as tar.zip, but they're actually gzip
compressed...


> From 838a463646a048b3dccff95079a514fdc86effb3 Mon Sep 17 00:00:00 2001
> From: Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>
> Date: Mon, 13 Aug 2018 11:27:57 +0530
> Subject: [PATCH 01/11] Split ExecStoreTuple into ExecStoreHeapTuple and
>  ExecStoreBufferHeapTuple
> 
> ExecStoreTuple() accepts a heap tuple from a buffer or constructed
> on-the-fly. In the first case the caller passed a valid buffer and in
> the later case it passes InvalidBuffer. In the first case,
> ExecStoreTuple() pins the given buffer and in the later case it
> records shouldFree flag. The function has some extra checks to
> differentiate between the two cases.  The usecases never overlap thus
> spending extra cycles in checks is useless. Hence separate these
> usecases into separate functions ExecStoreHeapTuple() to store
> on-the-fly tuple and ExecStoreBufferHeapTuple() to store an on-disk
> tuple from a buffer. This allows to shave some extra cycles while
> storing a tuple in the slot.

It doesn't *yet* allow shaving extra cycles, no?

> *	 SLOT ACCESSORS
>   *		ExecSetSlotDescriptor	- set a slot's tuple descriptor
> - *		ExecStoreTuple			- store a physical tuple in the slot
> + *		ExecStoreHeapTuple		- store an on-the-fly heap tuple in the slot
> + *		ExecStoreBufferHeapTuple - store an on-disk heap tuple in the slot
>   *		ExecStoreMinimalTuple	- store a minimal physical tuple in the slot
>   *		ExecClearTuple			- clear contents of a slot
>   *		ExecStoreVirtualTuple	- mark slot as containing a virtual
>   *		tuple

I'd advocate for a separate patch ripping these out, they're almost
always out of date.


>  /* --------------------------------
> - *		ExecStoreTuple
> + *		ExecStoreHeapTuple
>   *
> - *		This function is used to store a physical tuple into a specified
> + *		This function is used to store an on-the-fly physical tuple into a specified
>   *		slot in the tuple table.
>   *
>   *		tuple:	tuple to store
>   *		slot:	slot to store it in
> - *		buffer: disk buffer if tuple is in a disk page, else InvalidBuffer
>   *		shouldFree: true if ExecClearTuple should pfree() the tuple
>   *					when done with it
>   *
> - * If 'buffer' is not InvalidBuffer, the tuple table code acquires a pin
> - * on the buffer which is held until the slot is cleared, so that the tuple
> - * won't go away on us.
> + * shouldFree is normally set 'true' for tuples constructed on-the-fly.  But it
> + * can be 'false' when the referenced tuple is held in a tuple table slot
> + * belonging to a lower-level executor Proc node.  In this case the lower-level
> + * slot retains ownership and responsibility for eventually releasing the
> + * tuple.  When this method is used, we must be certain that the upper-level
> + * Proc node will lose interest in the tuple sooner than the lower-level one
> + * does!  If you're not certain, copy the lower-level tuple with heap_copytuple
> + * and let the upper-level table slot assume ownership of the copy!
> + *
> + * Return value is just the passed-in slot pointer.
> + * --------------------------------
> + */
> +TupleTableSlot *
> +ExecStoreHeapTuple(HeapTuple tuple,
> +				   TupleTableSlot *slot,
> +				   bool shouldFree)
> +{
> +	/*
> +	 * sanity checks
> +	 */
> +	Assert(tuple != NULL);
> +	Assert(slot != NULL);
> +	Assert(slot->tts_tupleDescriptor != NULL);
> +
> +	/*
> +	 * Free any old physical tuple belonging to the slot.
> +	 */
> +	if (slot->tts_shouldFree)
> +		heap_freetuple(slot->tts_tuple);
> +	if (slot->tts_shouldFreeMin)
> +		heap_free_minimal_tuple(slot->tts_mintuple);
> +
> +	/*
> +	 * Store the new tuple into the specified slot.
> +	 */
> +	slot->tts_isempty = false;
> +	slot->tts_shouldFree = shouldFree;
> +	slot->tts_shouldFreeMin = false;
> +	slot->tts_tuple = tuple;
> +	slot->tts_mintuple = NULL;
> +
> +	/* Mark extracted state invalid */
> +	slot->tts_nvalid = 0;
> +
> +	return slot;
> +}

Uh, there could very well be a buffer previously stored in the slot, no?
This can't currently be applied independently afaict.


> From c4c55bc0d501400ffca2e7393039b2d38660cd2d Mon Sep 17 00:00:00 2001
> From: Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>
> Date: Mon, 13 Aug 2018 11:27:57 +0530
> Subject: [PATCH 02/11] tts_nvalid in TupleTableSlot is AttrNumber

> @@ -125,7 +125,7 @@ typedef struct TupleTableSlot
>  	MemoryContext tts_mcxt;		/* slot itself is in this context */
>  	Buffer		tts_buffer;		/* tuple's buffer, or InvalidBuffer */
>  #define FIELDNO_TUPLETABLESLOT_NVALID 9
> -	int			tts_nvalid;		/* # of valid values in tts_values */
> +	AttrNumber	tts_nvalid;		/* # of valid values in tts_values */
>  #define FIELDNO_TUPLETABLESLOT_VALUES 10
>  	Datum	   *tts_values;		/* current per-attribute values */
>  #define FIELDNO_TUPLETABLESLOT_ISNULL 11

Shouldn't this be adapting at least a *few* more things, like
slot_getattr's argument?



>   /*
> - * Fill in missing values for a TupleTableSlot.
> - *
> - * This is only exposed because it's needed for JIT compiled tuple
> - * deforming. That exception aside, there should be no callers outside of this
> - * file.
> - */
> -void
> -slot_getmissingattrs(TupleTableSlot *slot, int startAttNum, int lastAttNum)
> -{
> -	AttrMissing *attrmiss = NULL;
> -	int			missattnum;
> -
> -	if (slot->tts_tupleDescriptor->constr)
> -		attrmiss = slot->tts_tupleDescriptor->constr->missing;
> -
> -	if (!attrmiss)
> -	{
> -		/* no missing values array at all, so just fill everything in as NULL */
> -		memset(slot->tts_values + startAttNum, 0,
> -			   (lastAttNum - startAttNum) * sizeof(Datum));
> -		memset(slot->tts_isnull + startAttNum, 1,
> -			   (lastAttNum - startAttNum) * sizeof(bool));
> -	}
> -	else
> -	{
> -		/* if there is a missing values array we must process them one by one */
> -		for (missattnum = startAttNum;
> -			 missattnum < lastAttNum;
> -			 missattnum++)
> -		{
> -			slot->tts_values[missattnum] = attrmiss[missattnum].am_value;
> -			slot->tts_isnull[missattnum] = !attrmiss[missattnum].am_present;
> -		}
> -	}
> -}

I would split out these moves into a separate commit, they are trivially
committable separately. The commit's pretty big already, and that'd make
it easier to see the actual differences.


> -
> -/*
>   * heap_compute_data_size
>   *		Determine size of the data area of a tuple to be constructed
>   */
> @@ -1407,10 +1370,9 @@ heap_deform_tuple(HeapTuple tuple, TupleDesc tupleDesc,
>   *		re-computing information about previously extracted attributes.
>   *		slot->tts_nvalid is the number of attributes already extracted.
>   */
> -static void
> -slot_deform_tuple(TupleTableSlot *slot, int natts)
> +void
> +slot_deform_tuple(TupleTableSlot *slot, HeapTuple tuple, uint32 *offp, int natts)
>  {

This should be renamed to include "heap" in the name, as it's not going
to be usable for, say, zheap.


> -/*
> - * slot_getsysattr
> - *		This function fetches a system attribute of the slot's current tuple.
> - *		Unlike slot_getattr, if the slot does not contain system attributes,
> - *		this will return false (with a NULL attribute value) instead of
> - *		throwing an error.
> - */
> -bool
> -slot_getsysattr(TupleTableSlot *slot, int attnum,
> -				Datum *value, bool *isnull)
> -{
> -	HeapTuple	tuple = slot->tts_tuple;
> -
> -	Assert(attnum < 0);			/* else caller error */
> -	if (tuple == NULL ||
> -		tuple == &(slot->tts_minhdr))
> -	{
> -		/* No physical tuple, or minimal tuple, so fail */
> -		*value = (Datum) 0;
> -		*isnull = true;
> -		return false;
> -	}
> -	*value = heap_getsysattr(tuple, attnum, slot->tts_tupleDescriptor, isnull);
> -	return true;
> -}

I think I was wrong at saying that we should remove this. I think you
were right that it should become a callback...


> +/*
> + * This is a function used by all getattr() callbacks which deal with a heap
> + * tuple or some tuple format which can be represented as a heap tuple e.g. a
> + * minimal tuple.
> + *
> + * heap_getattr considers any attnum beyond the attributes available in the
> + * tuple as NULL. This function however returns the values of missing
> + * attributes from the tuple descriptor in that case. Also this function does
> + * not support extracting system attributes.
> + *
> + * If the attribute needs to be fetched from the tuple, the function fills in
> + * tts_values and tts_isnull arrays upto the required attnum.
> + */
> +Datum
> +tts_heap_getattr_common(TupleTableSlot *slot, HeapTuple tuple, uint32 *offp,
> +						int attnum, bool *isnull)
> +{
> +	HeapTupleHeader tup = tuple->t_data;
> +	Assert(slot->tts_nvalid < attnum);
> +
> +	Assert(attnum > 0);
> +
> +	if (attnum > HeapTupleHeaderGetNatts(tup))
> +		return getmissingattr(slot->tts_tupleDescriptor, attnum, isnull);
> +
> +	/*
> +	 * check if target attribute is null: no point in groveling through tuple
> +	 */
> +	if (HeapTupleHasNulls(tuple) && att_isnull(attnum - 1, tup->t_bits))
> +	{
> +		*isnull = true;
> +		return (Datum) 0;
> +	}

I still think this is an optimization with a negative benefit,
especially as it requires an extra callback. We should just rely on
slot_deform_tuple and then access that. That'll also just access the
null bitmap for the relevant column, and it'll make successive accesses
cheaper.

> @@ -2883,7 +2885,7 @@ CopyFrom(CopyState cstate)
>  			if (slot == NULL)	/* "do nothing" */
>  				skip_tuple = true;
>  			else				/* trigger might have changed tuple */
> -				tuple = ExecMaterializeSlot(slot);
> +				tuple = ExecFetchSlotTuple(slot, true);
>  		}

Could we do the Materialize vs Fetch vs Copy change separately?


> diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
> index e1eb7c3..9957c70 100644
> --- a/src/backend/commands/matview.c
> +++ b/src/backend/commands/matview.c
> @@ -484,7 +484,7 @@ transientrel_receive(TupleTableSlot *slot, DestReceiver *self)
>  	 * get the heap tuple out of the tuple table slot, making sure we have a
>  	 * writable copy
>  	 */
> -	tuple = ExecMaterializeSlot(slot);
> +	tuple = ExecCopySlotTuple(slot);
>  
>  	heap_insert(myState->transientrel,
>  				tuple,
> @@ -494,6 +494,9 @@ transientrel_receive(TupleTableSlot *slot, DestReceiver *self)
>  
>  	/* We know this is a newly created relation, so there are no indexes */
>  
> +	/* Free the copied tuple. */
> +	heap_freetuple(tuple);
> +
>  	return true;
>  }

This'll potentially increase a fair amount of extra allocation overhead,
no?


> diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
> index 9d6e25a..1b4e726 100644
> --- a/src/backend/executor/execExprInterp.c
> +++ b/src/backend/executor/execExprInterp.c
> @@ -490,54 +490,21 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
>  
>  		EEO_CASE(EEOP_INNER_SYSVAR)
>  		{
> -			int			attnum = op->d.var.attnum;
> -			Datum		d;
> -
> -			/* these asserts must match defenses in slot_getattr */
> -			Assert(innerslot->tts_tuple != NULL);
> -			Assert(innerslot->tts_tuple != &(innerslot->tts_minhdr));
> -
> -			/* heap_getsysattr has sufficient defenses against bad attnums */
> -			d = heap_getsysattr(innerslot->tts_tuple, attnum,
> -								innerslot->tts_tupleDescriptor,
> -								op->resnull);
> -			*op->resvalue = d;
> +			ExecEvalSysVar(state, op, econtext, innerslot);

Please split this out into a separate patch.


> +/*
> + * TupleTableSlotOps implementation for BufferHeapTupleTableSlot.
> + */
> +
> +static void
> +tts_buffer_init(TupleTableSlot *slot)
> +{
> +}

Should rename these to buffer_heap or such.

> +/*
> + * Store the given tuple into the given BufferHeapTupleTableSlot and pin the
> + * given buffer. If the tuple already contained in the slot can be freed free
> + * it.
> + */
> +static void
> +tts_buffer_store_tuple(TupleTableSlot *slot, HeapTuple tuple, Buffer buffer)
> +{
> +	BufferHeapTupleTableSlot *bslot = (BufferHeapTupleTableSlot *) slot;
> +
> +	if (IS_TTS_SHOULDFREE(slot))
> +	{
> +		/*
> +		 * A heap tuple stored in a BufferHeapTupleTableSlot should have a
> +		 * buffer associated with it, unless it's materialized.
> +		 */
> +		Assert(!BufferIsValid(bslot->buffer));
> +
> +		heap_freetuple(bslot->base.tuple);
> +		RESET_TTS_SHOULDFREE(slot);
> +	}
> +
> +	RESET_TTS_EMPTY(slot);
> +	slot->tts_nvalid = 0;
> +	bslot->base.tuple = tuple;
> +	bslot->base.off = 0;
> +
> +	/*
> +	 * If tuple is on a disk page, keep the page pinned as long as we hold a
> +	 * pointer into it.  We assume the caller already has such a pin.
> +	 *
> +	 * This is coded to optimize the case where the slot previously held a
> +	 * tuple on the same disk page: in that case releasing and re-acquiring
> +	 * the pin is a waste of cycles.  This is a common situation during
> +	 * seqscans, so it's worth troubling over.
> +	 */
> +	if (bslot->buffer != buffer)
> +	{
> +		if (BufferIsValid(bslot->buffer))
> +			ReleaseBuffer(bslot->buffer);
> +		bslot->buffer = buffer;
> +		IncrBufferRefCount(buffer);
> +	}
> +}

This needs to also support storing a non-buffer tuple, I think.


> +/*
> + * TupleTableSlotOps for each of TupleTableSlotTypes. These are used to
> + * identify the type of slot.
> + */
> +const TupleTableSlotOps TTSOpsVirtual = {
> +	sizeof(TupleTableSlot),
> +	tts_virtual_init,
> +	tts_virtual_release,
> +	tts_virtual_clear,
> +	tts_virtual_getsomeattrs,
> +	tts_virtual_attisnull,
> +	tts_virtual_getattr,
> +	tts_virtual_materialize,
> +	tts_virtual_copyslot,
> +
> +	/*
> +	 * A virtual tuple table slot can not "own" a heap tuple or a minimal
> +	 * tuple.
> +	 */
> +	NULL,
> +	NULL,
> +	tts_virtual_copy_heap_tuple,
> +	tts_virtual_copy_minimal_tuple,
> +};

As we're now going to require C99, could you convert these into
designated initializer style (i.e. .init = tts_heap_init etc)?


> @@ -353,25 +1285,9 @@ ExecStoreHeapTuple(HeapTuple tuple,
>  	Assert(slot != NULL);
>  	Assert(slot->tts_tupleDescriptor != NULL);
>  
> -	/*
> -	 * Free any old physical tuple belonging to the slot.
> -	 */
> -	if (IS_TTS_SHOULDFREE(slot))
> -		heap_freetuple(slot->tts_tuple);
> -	if (IS_TTS_SHOULDFREEMIN(slot))
> -		heap_free_minimal_tuple(slot->tts_mintuple);
> -
> -	/*
> -	 * Store the new tuple into the specified slot.
> -	 */
> -	RESET_TTS_EMPTY(slot);
> -	shouldFree ? SET_TTS_SHOULDFREE(slot) : RESET_TTS_SHOULDFREE(slot);
> -	RESET_TTS_SHOULDFREEMIN(slot);
> -	slot->tts_tuple = tuple;
> -	slot->tts_mintuple = NULL;
> -
> -	/* Mark extracted state invalid */
> -	slot->tts_nvalid = 0;
> +	if (!TTS_IS_HEAPTUPLE(slot))
> +		elog(ERROR, "trying to store a heap tuple into wrong type of slot");
> +	tts_heap_store_tuple(slot, tuple, shouldFree);
>  
>  	return slot;
>  }

This should allow for buffer tuples too.


> @@ -983,9 +1595,10 @@ ExecInitExtraTupleSlot(EState *estate, TupleDesc tupledesc)
>   * ----------------
>   */
>  TupleTableSlot *
> -ExecInitNullTupleSlot(EState *estate, TupleDesc tupType)
> +ExecInitNullTupleSlot(EState *estate, TupleDesc tupType,
> +					  const TupleTableSlotOps *tts_cb)
>  {
> -	TupleTableSlot *slot = ExecInitExtraTupleSlot(estate, tupType);
> +	TupleTableSlot *slot = ExecInitExtraTupleSlot(estate, tupType, tts_cb);
>  
>  	return ExecStoreAllNullTuple(slot);
>  }

It's a bit weird that the type name is *Ops but the param is tts_cb, no?


> @@ -1590,7 +1590,8 @@ ExecHashTableInsert(HashJoinTable hashtable,
>  					TupleTableSlot *slot,
>  					uint32 hashvalue)
>  {
> -	MinimalTuple tuple = ExecFetchSlotMinimalTuple(slot);
> +	bool		shouldFree;
> +	MinimalTuple tuple = ExecFetchSlotMinimalTuple(slot, &shouldFree);
>  	int			bucketno;
>  	int			batchno;
>  
> @@ -1664,6 +1665,9 @@ ExecHashTableInsert(HashJoinTable hashtable,
>  							  hashvalue,
>  							  &hashtable->innerBatchFile[batchno]);
>  	}
> +
> +	if (shouldFree)
> +		heap_free_minimal_tuple(tuple);
>  }

Hm, how about splitting these out?


> @@ -277,10 +277,32 @@ ExecInsert(ModifyTableState *mtstate,
>  	OnConflictAction onconflict = node->onConflictAction;
>  
>  	/*
> -	 * get the heap tuple out of the tuple table slot, making sure we have a
> -	 * writable copy
> +	 * Get the heap tuple out of the tuple table slot, making sure we have a
> +	 * writable copy.
> +	 *
> +	 * If the slot can contain a heap tuple, materialize the tuple within the
> +	 * slot itself so that the slot "owns" it and any changes to the tuple
> +	 * reflect in the slot as well.
> +	 *
> +	 * Otherwise, store the copy of the heap tuple in es_dml_input_tuple_slot, which
> +	 * is assumed to be able to hold a heap tuple, so that repeated requests
> +	 * for heap tuple in the following code will not create multiple copies and
> +	 * leak memory. Also keeping the tuple in the slot makes sure that it will
> +	 * be freed when it's no more needed either because a trigger modified it
> +	 * or when we are done processing it.
>  	 */
> -	tuple = ExecMaterializeSlot(slot);
> +	if (!(TTS_IS_HEAPTUPLE(slot) || TTS_IS_BUFFERTUPLE(slot)))
> +	{
> +		TupleTableSlot *es_slot = estate->es_dml_input_tuple_slot;
> +
> +		Assert(es_slot && TTS_IS_HEAPTUPLE(es_slot));
> +		if (es_slot->tts_tupleDescriptor != slot->tts_tupleDescriptor)
> +			ExecSetSlotDescriptor(es_slot, slot->tts_tupleDescriptor);
> +		ExecCopySlot(es_slot, slot);
> +		slot = es_slot;
> +	}
> +
> +	tuple = ExecFetchSlotTuple(slot, true);

I strongly dislike this.  Could you look at my pluggable storage tree
and see whether you could do something similar here?


> @@ -2402,8 +2454,15 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
>  		 */
>  		mtstate->ps.plan->targetlist = (List *) linitial(node->returningLists);
>  
> -		/* Set up a slot for the output of the RETURNING projection(s) */
> -		ExecInitResultTupleSlotTL(estate, &mtstate->ps);
> +		/*
> +		 * Set up a slot for the output of the RETURNING projection(s).
> +		 * ExecDelete() requies the contents of the slot to be
> +		 * saved/materialized, so use heap tuple table slot for a DELETE.
> +		 * Otherwise a virtual tuple table slot suffices.
> +		 */
> +		ExecInitResultTupleSlotTL(estate, &mtstate->ps,
> +								  operation == CMD_DELETE ?
> +								  &TTSOpsHeapTuple : &TTSOpsVirtual);
>  		slot = mtstate->ps.ps_ResultTupleSlot;

I'm not clear on why this this the case?

>  		/* Need a
>         diff --git a/src/backend/executor/tqueue.c b/src/backend/executor/tqueue.c
> index ecdbe7f..ea2858b 100644
> --- a/src/backend/executor/tqueue.c
> +++ b/src/backend/executor/tqueue.c
> @@ -56,11 +56,28 @@ tqueueReceiveSlot(TupleTableSlot *slot, DestReceiver *self)
>  	TQueueDestReceiver *tqueue = (TQueueDestReceiver *) self;
>  	HeapTuple	tuple;
>  	shm_mq_result result;
> +	bool		tuple_copied = false;
> +
> +	/* Get the tuple out of slot, if necessary converting the slot's contents
> +	 * into a heap tuple by copying. In the later case we need to free the copy.
> +	 */
> +	if (TTS_IS_HEAPTUPLE(slot) || TTS_IS_BUFFERTUPLE(slot))
> +	{
> +		tuple = ExecFetchSlotTuple(slot, true);
> +		tuple_copied = false;
> +	}
> +	else
> +	{
> +		tuple = ExecCopySlotTuple(slot);
> +		tuple_copied = true;
> +	}

To me needing this if() here is a bad sign, I think we want a
ExecFetchSlotTuple* like api with a bool *shouldFree arg, like you did
for minimal tuples instead.

> diff --git a/src/backend/tcop/pquery.c b/src/backend/tcop/pquery.c
> index 66cc5c3..b44438b 100644
> --- a/src/backend/tcop/pquery.c
> +++ b/src/backend/tcop/pquery.c
> @@ -1071,7 +1071,7 @@ RunFromStore(Portal portal, ScanDirection direction, uint64 count,
>  	uint64		current_tuple_count = 0;
>  	TupleTableSlot *slot;
>  
> -	slot = MakeSingleTupleTableSlot(portal->tupDesc);
> +	slot = MakeSingleTupleTableSlot(portal->tupDesc, &TTSOpsMinimalTuple);
>  
>  	dest->rStartup(dest, CMD_SELECT, portal->tupDesc);

These fairly rote changes make it really hard to see the actual meat of
the changes in the patch. I think one way to address that would be to
introduce stub &TTSOps* variables, add them to MakeSingleTupleTableSlot
etc, but still have them return the "old style" slots.  Then that can be
reviewed separately.

> @@ -1375,9 +1376,9 @@ hypothetical_dense_rank_final(PG_FUNCTION_ARGS)
>  	 * previous row available for comparisons.  This is accomplished by
>  	 * swapping the slot pointer variables after each row.
>  	 */
> -	extraslot = MakeSingleTupleTableSlot(osastate->qstate->tupdesc);
> +	extraslot = MakeSingleTupleTableSlot(osastate->qstate->tupdesc,
> +										 &TTSOpsMinimalTuple);
>  	slot2 = extraslot;
> -
>  	/* iterate till we find the hypothetical row */
>  	while (tuplesort_gettupleslot(osastate->sortstate, true, true, slot,
>  								  &abbrevVal))

superflous change.




> diff --git a/src/backend/utils/sort/tuplestore.c b/src/backend/utils/sort/tuplestore.c
> index 5560a3e..ba98908 100644
> --- a/src/backend/utils/sort/tuplestore.c
> +++ b/src/backend/utils/sort/tuplestore.c
> @@ -1073,6 +1073,13 @@ tuplestore_gettuple(Tuplestorestate *state, bool forward,
>   * pointer to a tuple held within the tuplestore.  The latter is more
>   * efficient but the slot contents may be corrupted if additional writes to
>   * the tuplestore occur.  (If using tuplestore_trim, see comments therein.)
> + *
> + * If the given slot can not contain a minimal tuple, the given minimal tuple
> + * is converted into the form that the given slot can contain. Irrespective of
> + * the value of copy, that conversion might need to create a copy. If
> + * should_free is set to true by tuplestore_gettuple(), the minimal tuple is
> + * freed after the conversion, if necessary. Right now, the function only
> + * supports slots of type HeapTupleTableSlot, other than MinimalTupleTableSlot.
>   */
>  bool
>  tuplestore_gettupleslot(Tuplestorestate *state, bool forward,
> @@ -1085,12 +1092,25 @@ tuplestore_gettupleslot(Tuplestorestate *state, bool forward,
>  
>  	if (tuple)
>  	{
> -		if (copy && !should_free)
> +		if (TTS_IS_MINIMALTUPLE(slot))
>  		{
> -			tuple = heap_copy_minimal_tuple(tuple);
> +			if (copy && !should_free)
> +			{
> +				tuple = heap_copy_minimal_tuple(tuple);
> +				should_free = true;
> +			}
> +			ExecStoreMinimalTuple(tuple, slot, should_free);
> +		}
> +		else if (TTS_IS_HEAPTUPLE(slot))
> +		{
> +			HeapTuple htup = heap_tuple_from_minimal_tuple(tuple);
> +
> +			if (should_free)
> +				heap_free_minimal_tuple(tuple);
>  			should_free = true;
> +
> +			ExecStoreHeapTuple(htup, slot, should_free);
>  		}
> -		ExecStoreMinimalTuple(tuple, slot, should_free);
>  		return true;
>  	}
>  	else

Why is this a good idea? Shouldn't the caller always have a minimal slot
here? This is problematic, because it means this'd need adapting for
every new slot type, which'd be kinda against the idea of the whole
thing...


> +#define TTS_IS_VIRTUAL(slot) ((slot)->tts_cb == &TTSOpsVirtual)
> +#define TTS_IS_HEAPTUPLE(slot) ((slot)->tts_cb == &TTSOpsHeapTuple)
> +#define TTS_IS_MINIMALTUPLE(slot) ((slot)->tts_cb == &TTSOpsMinimalTuple)
> +#define TTS_IS_BUFFERTUPLE(slot) ((slot)->tts_cb == &TTSOpsBufferTuple)
> +
> +extern Datum ExecFetchSlotTupleDatum(TupleTableSlot *slot);

this is a weird place for the function protoype?


> +/* TupleTableSlotType specific routines */
> +typedef struct TupleTableSlotOps
> +{
> +	/* Minimum size of the slot */
> +	size_t			base_slot_size;
> +
> +	/* Initialization. */
> +	void (*init)(TupleTableSlot *slot);
> +
> +	/* Destruction. */
> +	void (*release)(TupleTableSlot *slot);
> +
> +	/*
> +	 * Clear the contents of the slot. Only the contents are expected to be
> +	 * cleared and not the tuple descriptor. Typically an implementation of
> +	 * this callback should free the memory allocated for the tuple contained
> +	 * in the slot.
> +	 */
> +	void (*clear)(TupleTableSlot *slot);
> +
> +	/*
> +	 * Fill up first natts entries of tts_values and tts_isnull arrays with
> +	 * values from the tuple contained in the slot. The function may be called
> +	 * with natts more than the number of attributes available in the tuple, in
> +	 * which case it should fill up as many entries as the number of available
> +	 * attributes. The callback should update tts_nvalid with number of entries
> +	 * filled up.
> +	 */
> +	void (*getsomeattrs)(TupleTableSlot *slot, int natts);
> +
> +	/*
> +	 * Returns true if the attribute given by attnum is NULL, return false
> +	 * otherwise. Some slot types may have more efficient methods to return
> +	 * NULL-ness of a given attribute compared to checking NULL-ness after
> +	 * calling getsomeattrs(). So this is a separate callback. We expect this
> +	 * callback to be invoked by slot_attisnull() only. That function returns
> +	 * if the information is available readily e.g. in tts_isnull array.  The
> +	 * callback need not repeat the same.
> +	 */
> +	bool (*attisnull)(TupleTableSlot *slot, int attnum);
> +
> +	/*
> +	 * Returns value of the given attribute as a datum and sets isnull to
> +	 * false, if it's not NULL. If the attribute is NULL, it sets isnull to
> +	 * true. Some slot types may have more efficient methods to return value of
> +	 * a given attribute rather than returning the attribute value from
> +	 * tts_values and tts_isnull after calling getsomeattrs(). So this is a
> +	 * separate callback. We expect this callback to be invoked by
> +	 * slot_getattr() only. That function returns if the information is
> +	 * available readily e.g. in tts_values and tts_isnull array or can be
> +	 * inferred from tuple descriptor.  The callback need not repeat the same.
> +	 */
> +	Datum (*getattr)(TupleTableSlot *slot, int attnum, bool *isnull);

These two really show go.


> +/* virtual or base type */
> +struct TupleTableSlot
>  {
>  	NodeTag		type;
>  #define FIELDNO_TUPLETABLESLOT_FLAGS 1
> -	uint16		tts_flags;		/* Boolean states */
> -#define FIELDNO_TUPLETABLESLOT_TUPLE 2
> -	HeapTuple	tts_tuple;		/* physical tuple, or NULL if virtual */
> -#define FIELDNO_TUPLETABLESLOT_TUPLEDESCRIPTOR 3
> -	TupleDesc	tts_tupleDescriptor;	/* slot's tuple descriptor */
> -	MemoryContext tts_mcxt;		/* slot itself is in this context */
> -	Buffer		tts_buffer;		/* tuple's buffer, or InvalidBuffer */
> -#define FIELDNO_TUPLETABLESLOT_NVALID 6
> +	uint16		tts_flags;
> +#define FIELDNO_TUPLETABLESLOT_NVALID 2
>  	AttrNumber	tts_nvalid;		/* # of valid values in tts_values */
> -#define FIELDNO_TUPLETABLESLOT_VALUES 7
> +
> +	const TupleTableSlotOps *const tts_cb;
> +#define FIELDNO_TUPLETABLESLOT_TUPLEDESCRIPTOR 4
> +	TupleDesc	tts_tupleDescriptor;	/* slot's tuple descriptor */
> +#define FIELDNO_TUPLETABLESLOT_VALUES 5
>  	Datum	   *tts_values;		/* current per-attribute values */
> -#define FIELDNO_TUPLETABLESLOT_ISNULL 8
> +#define FIELDNO_TUPLETABLESLOT_ISNULL 6
>  	bool	   *tts_isnull;		/* current per-attribute isnull flags */
> -	MinimalTuple tts_mintuple;	/* minimal tuple, or NULL if none */
> -	HeapTupleData tts_minhdr;	/* workspace for minimal-tuple-only case */
> -#define FIELDNO_TUPLETABLESLOT_OFF 11
> -	uint32		tts_off;		/* saved state for slot_deform_tuple */
> -} TupleTableSlot;
>  
> -#define TTS_HAS_PHYSICAL_TUPLE(slot)  \
> -	((slot)->tts_tuple != NULL && (slot)->tts_tuple != &((slot)->tts_minhdr))
> +	/* can we optimize away? */
> +	MemoryContext tts_mcxt;		/* slot itself is in this context */
> +};

the "can we" comment above is pretty clearly wrong, I think (Yes, I made
it...).



> From 84046126d5b8c9d780cb7134048cc717bda462a8 Mon Sep 17 00:00:00 2001
> From: Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>
> Date: Mon, 13 Aug 2018 11:27:57 +0530
> Subject: [PATCH 07/11] Reset expression context just after resetting per tuple
>  context in ExecModifyTable().
> 
> Expression context saved in ps_ExprContext for ModifyTable node is
> used to process returning clause and on conflict clause. For every
> processed tuple it's reset in ExecProcessReturning() and
> ExecOnConflictUpdate(). When a query has both RETURNING and ON
> CONFLICT clauses, the reset happens twice and the first one of those
> might reset memory used by the other. For some reason this doesn't
> show up on HEAD, but is apparent when virtual tuple table slots, which
> do not copy the datums in its own memory, are used for tuples returned
> by RETURNING clause.
> 
> This is fix for a query failing in sql/insert_conflict.sql
> 
> insert into insertconflicttest as i values (23, 'Avocado') on conflict (key) do update set fruit = excluded.*::text
>   returning *;
> 
> Ashutosh Bapat, per suggestion by Andres Freund

Doesn't this have to be earlier in the series?


Phew, sorry if some things are daft, I'm kinda jetlagged...

- Andres