Re: TupleTableSlot abstraction

On Sat, 13 Oct 2018 at 04:02, Andres Freund <andres@anarazel.de> wrote:
> > > > @@ -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.
Can you explain more why you think there should be a ExecEvalSysVar()
definition ? As I mentioned earlier it would just call
slot_getsysattr() and do nothing else.

> And it's simpler for JIT than the alternative.

You mean it would be simpler for JIT to call ExecEvalSysVar() than
slot_getsysattr() ? I didn't get why it is simpler.

Or are you talking considering build_ExecEvalSysVar() ? I am ok with
retaining build_ExecEvalSysVar() , but I was saying even inside this
function, we could do :
LLVMBuildCall(.... , llvm_get_decl(mod, FuncSlotGetsysattr) , .....)
rather than:
LLVMFunctionType(,...)
LLVMAddFunction("ExecEvalSysVar", ....)
LLVMBuildCall(...)


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

Ok.

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

I haven't gone ahead because I wanted to know if you are ok with the names.

-- 
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company