From: Andres Freund <andres@anarazel.de>
To: Amit Khandekar <amitdkhan.pg@gmail.com>
Cc: Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: Haribabu Kommi <kommi.haribabu@gmail.com>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>
Subject: Re: TupleTableSlot abstraction
Date: Fri, 12 Oct 2018 15:32:31 -0700
Message-ID: <20181012223231.3tedn53fzosm56id@alap3.anarazel.de> (raw)
In-Reply-To: <CAJ3gD9eidD6DFeLLpu4QMpbk_XNVOOQKz8KtawwwuuY0DZwrew@mail.gmail.com>
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>
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: andres@anarazel.de, amitdkhan.pg@gmail.com, ashutosh.bapat@enterprisedb.com, robertmhaas@gmail.com, kommi.haribabu@gmail.com, alvherre@2ndquadrant.com, amit.kapila16@gmail.com
Subject: Re: TupleTableSlot abstraction
In-Reply-To: <20181012223231.3tedn53fzosm56id@alap3.anarazel.de>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox