Re: ALTER TABLE ADD COLUMN fast default - Mailing list pgsql-hackers

On 12/06/2017 11:05 AM, Tom Lane wrote:
> 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.


Fair point.

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


OK, will work on all that. Thanks again.

cheers

andrew

            

-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services