Re: TupleTableSlot abstraction

On 28 August 2018 at 22:43, Ashutosh Bapat
<ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote:
> On Fri, Aug 24, 2018 at 6:46 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>>
>>> -/*
>>> - * slot_getsysattr
>>> - * This function fetches a system attribute of the slot's current tuple.
>>> - * Unlike slot_getattr, if the slot does not contain system attributes,
>>> - * this will return false (with a NULL attribute value) instead of
>>> - * throwing an error.
>>> - */
>>> -bool
>>> -slot_getsysattr(TupleTableSlot *slot, int attnum,
>>> - Datum *value, bool *isnull)
>>> -{
>>> - HeapTuple tuple = slot->tts_tuple;
>>> -
>>> - Assert(attnum < 0); /* else caller error */
>>> - if (tuple == NULL ||
>>> - tuple == &(slot->tts_minhdr))
>>> - {
>>> - /* No physical tuple, or minimal tuple, so fail */
>>> - *value = (Datum) 0;
>>> - *isnull = true;
>>> - return false;
>>> - }
>>> - *value = heap_getsysattr(tuple, attnum, slot->tts_tupleDescriptor, isnull);
>>> - return true;
>>> -}
>>
>> 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.

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

The only use case where slot_getsysattr() is called right now is
execCurrentOf(), and it currently checks the bool return value of
slot_getsysattr() and prints a user-friendly message if false is
returned. Looking at the code (commit 8f5ac440430ab), it seems that we
want to continue to have a user-friendly message for some unhandled
cases like custom scans. So perhaps for now it's ok to go with the
first option where getsysattr callback returns false for slot types
that don't have system attributes.

>
>>
>>
>>> +/*
>>> + * 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)
>>> +{
>>> + HeapTupleHeader tup = tuple->t_data;
>>> + Assert(slot->tts_nvalid < attnum);
>>> +
>>> + Assert(attnum > 0);
>>> +
>>> + if (attnum > HeapTupleHeaderGetNatts(tup))
>>> + return getmissingattr(slot->tts_tupleDescriptor, attnum, isnull);
>>> +
>>> + /*
>>> + * check if target attribute is null: no point in groveling through tuple
>>> + */
>>> + if (HeapTupleHasNulls(tuple) && att_isnull(attnum - 1, tup->t_bits))
>>> + {
>>> + *isnull = true;
>>> + return (Datum) 0;
>>> + }
>>
>> 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.

----------

BTW, on HEAD, for dropped attribute slot_getattr() returns null datum,
which hasn't been done in the patch series. That needs to be done.