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