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(dot)bapat(at)enterprisedb(dot)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(dot)bapat(at)enterprisedb(dot)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(dot)bapat(at)enterprisedb(dot)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