Horton

Re: TupleTableSlot abstraction

Andres Freund <andres@anarazel.de>

View thread View on postgr.es
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>
Date: 2018-08-31T15:20:01Z
Lists: pgsql-hackers 
Hi,

On 2018-08-31 10:05:05 +0530, Amit Khandekar wrote:
> On 28 August 2018 at 22:43, Ashutosh Bapat
> >> I think I was wrong at saying that we should remove this. I think you
> >> were right that it should become a callback...
> 
> > We have replaced all slot_getsysattrs() with heap_getsysattr(). Do you
> > want to reinstantiate those as well? If so, slot_getsysattr() becomes
> > a wrapper around getsysattr() callback.

Right.

> One option is that the getsysattr() callback function returns false if
> system attributes are not supported for that slot type. Other option
> is that in the not-supported case, the function errors out, meaning
> that the caller should be aware that the slot type is the one that
> supports system attributes.

> I had prepared changes for the first option, but Ashutosh Bapat
> offlist made me realize that it's worth considering the 2nd option(
> i.e. erroring out).

I think we should error out.


> >> I still think this is an optimization with a negative benefit,
> >> especially as it requires an extra callback. We should just rely on
> >> slot_deform_tuple and then access that. That'll also just access the
> >> null bitmap for the relevant column, and it'll make successive accesses
> >> cheaper.
> >
> > I don't understand why we have differing implementations for
> > slot_attisnull(), slot_getsomeattrs(), slot_getattr() in HEAD. If what
> > you are saying is true, we should have implemented all the first and
> > last as a call to slot_getsomeattrs() followed by returing values from
> > tts_values and tts_isnull. Since this is refactoring work, I am trying
> > not to change the existing functionality of those functions.
> 
> I agree that we should not change the way in which slot_getattr()
> finds the attr; i.e. first call att_isnull(), and only then try to
> deform the tuple. I mean there must be some good reason that is done
> on HEAD. Maybe we can change this separately after investigation, but
> not as part of the tuple abstraction patch.

There's really no good reason for the behaviour as it exists on HEAD. It
already can cause worse performance there. The price to pay for
continuing to have an optimization which isn't actually optimizing
anything is way too high if it requires us to have multiple functionally
unnecessary callbacks.

Greetings,

Andres Freund

Commits

  1. Fix memory leak when inserting tuple at relation creation for CTAS

  2. Build HashState's hashkeys expression with the correct parent.

  3. Move TupleTableSlots boolean member into one flag variable.

  4. Move generic slot support functions from heaptuple.c into execTuples.c.

  5. Remove absolete function TupleDescGetSlot().

  6. Remove function list from prologue of execTuples.c.

  7. Split ExecStoreTuple into ExecStoreHeapTuple and ExecStoreBufferHeapTuple.

  8. Change TupleTableSlot->tts_nvalid to type AttrNumber.

  9. Deduplicate code between slot_getallattrs() and slot_getsomeattrs().

  10. Mention ownership requirements for REFRESH MATERIALIZED VIEW in docs

  11. Spell "partitionwise" consistently.

  12. Fix WHERE CURRENT OF when the referenced cursor uses an index-only scan.