Re: TupleTableSlot abstraction

On 2018-10-09 20:46:04 +0530, Amit Khandekar wrote:
> On Wed, 26 Sep 2018 at 05:35, Andres Freund <andres(at)anarazel(dot)de> wrote:
> > > +
> > > +/*
> > > + * 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)
> >
> > I'm still *vehemently* opposed to the introduction of this.
>
> You mean, you want to remove the att_isnull() optimization, right ?

Yes.

> Removed that code now. Directly deforming the tuple regardless of the
> null attribute.

Good, thanks.

> > > @@ -2024,7 +2024,18 @@ FormIndexDatum(IndexInfo *indexInfo,
> > > Datum iDatum;
> > > bool isNull;
> > >
> > > - if (keycol != 0)
> > > + if (keycol < 0)
> > > + {
> > > + HeapTupleTableSlot *hslot = (HeapTupleTableSlot *)slot;
> > > +
> > > + /* Only heap tuples have system attributes. */
> > > + Assert(TTS_IS_HEAPTUPLE(slot) || TTS_IS_BUFFERTUPLE(slot));
> > > +
> > > + iDatum = heap_getsysattr(hslot->tuple, keycol,
> > > + slot->tts_tupleDescriptor,
> > > + &isNull);
> > > + }
> > > + else if (keycol != 0)
> > > {
> > > /*
> > > * Plain index column; get the value we need directly from the
> >
> > This now should access the system column via the slot, right? There's
> > other places like this IIRC.
>
> Done. In FormIndexDatum() and ExecInterpExpr(), directly calling
> slot_getsysattr() now.
>
> In ExecInterpExpr (), I am no longer using ExecEvalSysVar() now. I am
> planning to remove this definition since it would be a single line
> function just calling slot_getsysattr().
>
> In build_ExecEvalSysVar(), ExecEvalSysVar() is still used, so I
> haven't removed the definition yet. I am planning to create a new
> LLVMValueRef FuncSlotGetsysattr, and use that instead, in
> build_ExecEvalSysVar(), or for that matter, I am thinking to revert
> back build_ExecEvalSysVar() and instead have that code inline as in
> HEAD.

I'd just have ExecInterpExpr() continue calling ExecEvalSysVar. There's
no reason for it to be inline. And it's simpler for JIT than the
alternative.

> > > @@ -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 copying here what I discussed about this in the earlier reply:
>
> I am not sure what was release() designed to do. Currently all of the
> implementers of this function are empty.

So additional deallocations can happen in a slot. We might need this
e.g. at some point for zheap which needs larger, longer-lived, buffers
in slots.

> Was it meant for doing
> ReleaseTupleDesc(slot->tts_tupleDescriptor) ? Or
> ReleaseBuffer(bslot->buffer) ?

No. The former lives in generic code, the latter is in ClearTuple.

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

It's just plainly wrong to call it this way round.

> I went ahead and did these changes, but for now, I haven't replaced
> ExecFetchSlotTuple() with ExecFetchSlotHeapTuple(). Instead, I
> retained ExecFetchSlotTuple() to be called for heap tuples, and added
> a new ExecFetchGenericSlotTuple() to be used with shouldFree. I don't
> like these names, but until we have concluded, I don't want to go
> ahead and replace all the numerous ExecFetchSlotTuple() calls with
> ExecFetchSlotHeapTuple().

Why not?

Greetings,

Andres Freund