On Mon, Aug 6, 2018 at 10:15 AM, Andres Freund <and...@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