Re: TupleTableSlot abstraction

On Thu, 4 Oct 2018 at 22:59, Amit Khandekar <amitdkhan(dot)pg(at)gmail(dot)com> wrote:
>
> I have only done the below two changes yet. After doing that and
> rebasing with latest master, in the regression I got crashes, and I
> suspect the reason being that I have used Virtual tuple slot for the
> destination slot of execute_attr_map_slot(). I am analyzing it. I am
> anyway attaching the patches (v12) to give you an idea of how I have
> handled the below two items.

It seems to be some corruption here :

@@ -956,17 +978,39 @@ ExecUpdate(ModifyTableState *mtstate,

- 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);

After the slotification of partition tuple conversion, the input slot
is a virtual tuple, and the above code seems to result in some
corruption which I have not finished analyzing. It only happens for
INSERT ON CONFLICT case with partitions.

On Wed, 26 Sep 2018 at 05:35, Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> > @@ -185,6 +1020,7 @@ ExecResetTupleTable(List *tupleTable, /* tuple table */
> > {
> > TupleTableSlot *slot = lfirst_node(TupleTableSlot, lc);
> >
> > + slot->tts_cb->release(slot);
> > /* Always release resources and reset the slot to empty */
> > ExecClearTuple(slot);
> > if (slot->tts_tupleDescriptor)
> > @@ -240,6 +1076,7 @@ void
> > ExecDropSingleTupleTableSlot(TupleTableSlot *slot)
> > {
> > /* This should match ExecResetTupleTable's processing of one slot */
> > + slot->tts_cb->release(slot);
> > Assert(IsA(slot, TupleTableSlot));
> > ExecClearTuple(slot);
> > if (slot->tts_tupleDescriptor)
>
> ISTM that release should be called *after* clearing the slot.

I am not sure what was release() designed to do. Currently all of the
implementers of this function are empty. Was it meant for doing
ReleaseTupleDesc(slot->tts_tupleDescriptor) ? Or
ReleaseBuffer(bslot->buffer) ? I think the purpose of keeping this
*before* clearing the tuple might be because the clear() might have
already cleared some handles that release() might need.

>
>
> > @@ -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;
> > + }
>
> This seems like a bad idea to me. We shouldn't hardcode slots like
> this. I've previously argued that we should instead allow
> ExecFetchSlotTuple() for all types of tuples, but add a new bool
> *shouldFree argument, which will then allow the caller to free the
> tuple. We gain zilch by having this kind of logic in multiple callers.

How about having a separate ExecFetchSlotHeapTuple() for many of the
callers where it is known that the tuple is a heap/buffer tuple ? And
in rare places such as above where slot type is not known, we can have
ExecFetchSlotTuple() which would have an extra shouldFree parameter.