Re: TupleTableSlot abstraction

On Thu, 4 Oct 2018 at 22:59, Amit Khandekar <amitdkhan.pg@gmail.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@anarazel.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.