Re: TupleTableSlot abstraction

On Fri, Aug 24, 2018 at 6:46 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> 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...

I am using "tar -zcvf" to create the files where -z indicates gzip.
But if I use .gz as extension for some reason it's not able to
recognize the format. So, I just keep .zip. Do you see any problem
with that extension? The attached file has extension .tar.gz.

>
>
>> 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?

By segregating those two functions we are already saving some cycles
by not checking for a valid buffer for an on the fly tuple. That's not
huge but some. More saving will come with the complete abstraction.

>
>> * 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.

Done. Added as 0001 patch.

>
>> + * Return value is just the passed-in slot pointer.
>> + * --------------------------------
>> + */
>> +TupleTableSlot *
>> +ExecStoreHeapTuple(HeapTuple tuple,
>> + TupleTableSlot *slot,
>> + bool shouldFree)
>> +{
>> +
>> + return slot;
>> +}
>
> Uh, there could very well be a buffer previously stored in the slot, no?
> This can't currently be applied independently afaict.

Sorry, I missed that when splitting the function. Added code to unpin
any buffer that was pinned for an earlier tuple.

>
>> 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 1
>> 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?

That's not something I had in mind for this patch. There are many
other places where we declare attribute number variables as "int"
instead of "AttrNumber". I don't think that's what we should do in
this patch set. The idea of this patch is to reduce the size of
TupleTableSlot by 2 bytes. Changing signatures of the functions,
though has some readability improvement, doesn't achieve any such
benefit.

Even then I went ahead and change signatures of those functions, but
server built with LLVM crashed. So, I have reverted back all those
changes. I think we should change the JIT code automatically when
function signatures/structure definitions change OR at least
compilation should fail indicating the difference between JIT code and
normal code. Investigating a crash takes a hell lot of time and is not
easy without a custom LLVM build. I doubt if every PostgreSQL
developer would want to do that.

>
>> /*
>> - * 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.

I have included some of those movements in the same patch which
changes the type of tts_nvalid. Functions like slot_attisnull() are
changed to be inline static functions in tuptable.h. Those functions
are now inline since they are just wrapper around slot specific
callbacks. Thus without the TupleTableSlot abstraction patch it's not
possible to mark them "inline and thus move them to a header file e.g.
tuptable.h. For now I have left these kinds of movements in
TupleTableSlot abstraction patch.

>
>
>> -
>> -/*
>> * 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.

LGTM. Done.

>
>
>> -/*
>> - * 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...

We have replaced all slot_getsysattrs() with heap_getsysattr(). Do you
want to reinstantiate those as well? If so, slot_getsysattr() becomes
a wrapper around getsysattr() 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.

I don't understand why we have differing implementations for
slot_attisnull(), slot_getsomeattrs(), slot_getattr() in HEAD. If what
you are saying is true, we should have implemented all the first and
last as a call to slot_getsomeattrs() followed by returing values from
tts_values and tts_isnull. Since this is refactoring work, I am trying
not to change the existing functionality of those functions.

>
>> @@ -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?
>

Ok. I will do that.

>
>> 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?

Yes. There are two ways to avoid that 1. Using a slot type specific
for the transient rel which is receiving the tuple OR 2. Allocate
memory for the converted tuple into per tuple memory context. The
first case too will have the same problem since we will free one tuple
at at time in ExecClearSlot(). I am inclined to use second option.
Comments?

>
>
>> 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.
>

Ok. Will do (not in the attached patch-set)

>
>> +/*
>> + * TupleTableSlotOps implementation for BufferHeapTupleTableSlot.
>> + */
>> +
>> +static void
>> +tts_buffer_init(TupleTableSlot *slot)
>> +{
>> +}
>
> Should rename these to buffer_heap or such.

Ok. Done.

>
>> +/*
>> + * 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.

I don't see a reason why that's needed. A non-buffer tuple should be
stored in a HeapTupleTableSlot. It might happen that
ExecMaterializeSlot() creates a copy of the buffer tuple in the memory
context of the slot. But there should be a buffer associated with the
heap tuple being stored in BufferHeapTupleTableSlot. Otherwise, why
are we separating those two types.

In fact, as discussed earlier offline, I am of the opinion that buffer
should be an attribute of every tuple table slot which may carry tuple
from a buffer, instead of creating two slot types with and without
buffer.

OTOH, I observe that tts_buffer_store_tuple() is called only from
ExecStoreBufferHeapTuple(). We may want to merge the first into the
later.

>
>> +/*
>> + * 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)?

That's a good idea. Done.

>
>> @@ -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.

Same answer as the answer to your comment on tts_buffer_store_tuple().

>
>> @@ -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?

Your original patch used the same names and I haven't changed that. I
am fine with tts_ops as well. Is that good enough?

>
>
>> @@ -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?

Split into a separate patch? It doesn't make sense to add this patch
before 0006 since the slots in those patches can "own" a minimal
tuple. Let's add a patch after 0006 i.e. tuple table abstraction
patch. Will do.

>
>
>> @@ -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?

Can you please provide the URL to your tree?

>
>
>> @@ -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?

Do you mean why does ExecDelete() materializes the tuple? I don't
know. But my guess is materialization ensures valid tuple being
returned in case the deleted tuple's contents are destroyed.

>
>> /* 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.

If we change the existing API, we will need to handle shouldFree flag
everywhere ExecFetchSlotTuple() is being called. Most of the callers
of ExecFetchSlotTuple pass tuple table slots which can "own" a heap
tuple. Your suggestion to use a separate ExecFetchSlotTuple() like API
looks good.. But I am not sure how useful it will be if there's going
to be a single caller of that API.

>
>> 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.

This means that we have to define TTSOps variables before the actual
abstraction patch and then redefine those in the abstraction patch
(0006). Instead we could separate changes to the slot initialization
functions in a separate patch which comes after the abstraction proper
patch. The abstraction proper patch always creates a virtual tuple
table slot. We will need to compile, test and commit both the patches
together but reviewing becomes easier. Also the tuple table
abstraction patch doesn't pass regession on its own, it requires some
patches that follow it. So it should be fine. Comments?

>
>> @@ -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.

Done.

>
>
>
>
>> 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?

As I have explained in the commit message, not every caller has a
minimal slot right now. For example, AfterTriggerExecute fetches
foreign tuples from a tuple store as minimal tuples and requires those
to be in the form of heap tuple for trigger processing. We can use two
slots, first a minimal for fetching tuple from tuple store and second
a heap for trigger processing and convert from minimal tuple to heap
tuple using ExecCopySlotTuple(). That increases the number of slots.

> 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...

I don't think we will need to support more slot types in than what's
there right now. But in case we have to we have two more options as
below.

Amit Khandekar suggested offlist to invoke a (new) slot type specific
callback which will convert given minimal tuple into the tuple type of
the slot.

Third option is to write tuplestore_gettupleslot* functions one for
each kind of TupleTableSlot type.

I don't have much preference for one over the other.

>
>> +#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?

Fixed.

>
>
>> +/* 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.

I have explained my position for this while replying to an earlier comment.

>
>
>> +/* 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...).
>

Removed.

>
>
>> 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?

I have placed it later since the tuple table slot abstraction work
exposes the bug, which otherwise is not visible. But yes, we can
commit that patch before the tuple table slot abstraction proper work.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company