Date: Fri, 12 Oct 2018 15:32:31 -0700
From: Andres Freund <andres@anarazel.de>
To: Amit Khandekar <amitdkhan.pg@gmail.com>
Cc: Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>,
	pgsql-hackers <pgsql-hackers@postgresql.org>,
	Robert Haas <robertmhaas@gmail.com>,
	Haribabu Kommi <kommi.haribabu@gmail.com>,
	Alvaro Herrera <alvherre@2ndquadrant.com>,
	Amit Kapila <amit.kapila16@gmail.com>
Subject: Re: TupleTableSlot abstraction
Message-ID: <20181012223231.3tedn53fzosm56id@alap3.anarazel.de>
References: <20180817080706.a4hh4fluilejkjdi@alap3.anarazel.de>
 <20180818152310.6quymlphu276mpwu@alap3.anarazel.de>
 <CAFjFpRfScLMD6RkC=mkw3z-LNfa+K+=vY7JgXNXSgD9RBTMKcg@mail.gmail.com>
 <20180820122822.uag4nlmslvajaacv@alap3.anarazel.de>
 <CAFjFpRcNPQ1oOL41-HQYaEF=Nq6Vbg0eHeFgopJhHw_X2usA5w@mail.gmail.com>
 <20180824011634.q2lrc6b26mjwoi5s@alap3.anarazel.de>
 <CAFjFpRe1TZrDHwPq-EzTWd=N6keGNHj_+GWe_zyQ5ZooQ+4LeA@mail.gmail.com>
 <CAJ3gD9fLTn0U5N5iNPkSKFCWLDKfFO7AKq7L8ozxZ2QcuJEuiw@mail.gmail.com>
 <20180926000511.ivjuepi7cqy2c3ns@alap3.anarazel.de>
 <CAJ3gD9eidD6DFeLLpu4QMpbk_XNVOOQKz8KtawwwuuY0DZwrew@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CAJ3gD9eidD6DFeLLpu4QMpbk_XNVOOQKz8KtawwwuuY0DZwrew@mail.gmail.com>
Precedence: bulk

On 2018-10-09 20:46:04 +0530, Amit Khandekar wrote:
> On Wed, 26 Sep 2018 at 05:35, Andres Freund <andres@anarazel.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