Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
> On 12/06/2017 10:16 AM, Tom Lane wrote:
>> I'm quite disturbed both by the sheer invasiveness of this patch
>> (eg, changing the API of heap_attisnull()) and by the amount of logic
>> it adds to fundamental tuple-deconstruction code paths. I think
>> we'll need to ask some hard questions about whether the feature gained
>> is worth the distributed performance loss that will ensue.
> I'm sure we can reduce the footprint some.
> w.r.t. heap_attisnull, only a handful of call sites actually need the
> new functionality, 8 or possibly 10 (I need to check more on those in
> relcache.c). So we could instead of changing the function provide a new
> one to be used at those sites. I'll work on that. and submit a new patch
> fairly shortly.
Meh, I'm not at all sure that's an improvement. A version of
heap_attisnull that ignores the possibility of a "missing" attribute value
will give the *wrong answer* if the other version should have been used
instead. Furthermore it'd be an attractive nuisance because people would
fail to notice if an existing call were now unsafe. If we're to do this
I think we have no choice but to impose that compatibility break on
third-party code.
In general, you need to be thinking about this in terms of making sure
that it's impossible to accidentally not update code that needs to be
updated. Another example is that I noticed that some places the patch
has s/CreateTupleDescCopy/CreateTupleDescCopyConstr/, evidently because
the former will fail to copy the missing-attribute support data.
I think that's an astonishingly bad idea. There is basically no situation
in which CreateTupleDescCopy defined in that way will ever be safe to use.
The missing-attribute info is now a fundamental part of a tupdesc and it
has to be copied always, just as much as e.g. atttypid.
I would opine that it's a mistake to design TupleDesc as though the
missing-attribute data had anything to do with default values. It may
have originated from the same place but it's a distinct thing. Better
to store it in a separate sub-structure.
regards, tom lane