On Mon, Aug 6, 2018 at 10:15 AM, Andres Freund <andres@anarazel.de> wrote:
> Hi,
>
> On 2018-08-06 10:08:24 +0530, Ashutosh Bapat wrote:
>> I think, I explained why getattr() needs to be a separate callback.
>> There's a reason why slot_getattr() more code than just calling
>> slot_getsomeattrs() and return the required one - the cases when the
>> requested attribute is NULL or is missing from a tuple. Depending
>> upon the tuple layout access to a simple attribute can be optimized
>> without spending cycles to extract attributes prior to that one.
>> Columnar store (I haven't seen Haribabu's patch), for example, stores
>> columns from multiple rows together and thus doesn't have a compact
>> version of tuple. In such cases extracting an individual attribute
>> directly is far cheaper than extracting all the previous attributes.
>
> OTOH, it means we continually access the null bitmap instead of just
> tts_isnull[i].
Nope. The slot_getattr implementation in these patches as well as in
the current master return from tts_isnull if the array has the
information.
>
> Your logic about not deforming columns in this case would hold for *any*
> deforming of previous columns as well. That's an optimization that we
> probably want to implement at some point (e.g. by building a bitmap of
> needed columns in the planner), but I don't think we should do it
> together with this already large patchset.
Agree about optimizing using a separate bitmap indicating validity of
a particular value.
>
>
>> Why should we force the storage API to extract all the attributes in
>> such a case?
>
> Because there's no need yet, and it complicates the API without
> corresponding benefit.
The earlier version of slot_getattr() was optimized to access a given
attribute. That must have been done for some reason. Leaving that
optimization aside for this work will cause regression in the cases
where that optimization matters.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company