Re: ALTER TABLE ADD COLUMN fast default

On 4/4/21 12:05 PM, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> On 4/3/21 10:09 PM, Tom Lane wrote:
>>> Looking around at the other touches of AttrDefault.adbin in the backend
>>> (of which there are not that many), some of them are prepared for it to be
>>> NULL and some are not.  I don't immediately have a strong opinion whether
>>> that should be an allowed state; but if it is not allowed then it's not
>>> okay for AttrDefaultFetch to leave such a state behind.  Alternatively,
>>> if it is allowed then equalTupleDesc is in the wrong.  We should choose
>>> one definition and make all the relevant code match it.
>> There's already a warning if it sets an explicit NULL value, so I'm
>> inclined to say your suggestion "it's not okay for AttrDefaultFetch to
>> leave such a state behind" is what we should go with.
> Yeah, I came to the same conclusion after looking around a bit more.
> There are two places in tupdesc.c that defend against adbin being NULL,
> which seems a bit silly when their sibling equalTupleDesc does not.
> Every other touch in the backend will segfault on a NULL value,
> if it doesn't Assert first :-(
>
> Another thing that annoyed me while I was looking at this is the
> potentially O(N^2) behavior in equalTupleDesc due to not wanting
> to assume that the AttrDefault arrays are in the same order.
> It seems far smarter to have AttrDefaultFetch sort the arrays.


+1 The O(N^2) loops also bothered me.


>
> So attached is a proposed patch to clean all this up.
>
> * Don't link the AttrDefault array into the relcache entry until
> it's fully built and valid.
>
> * elog, don't just Assert, if we fail to find an expected default
> value.  (Perhaps there's a case for ereport instead.)
>
> * Remove now-useless null checks in tupdesc.c.
>
> * Sort the AttrDefault array, remove double loops in equalTupleDesc.


This all looks a lot cleaner and simpler to follow. I like not
allocating the array until AttrDefaultFetch.


>
> I made CheckConstraintFetch likewise not install its array until
> it's done.  I notice that it is throwing elog(ERROR) not WARNING
> for the equivalent cases of not finding the right number of
> entries.  I wonder if we ought to back that off to a WARNING too.
> elog(ERROR) during relcache entry load is kinda nasty, because
> it prevents essentially *all* use of the relation.  On the other
> hand, failing to enforce check constraints isn't lovely either.
> Anybody have opinions about that?



None of this is supposed to happen, is it? If you can't fetch the
constraints (and I think of a default as almost a kind of constraint)
your database is already somehow corrupted. So maybe if any of these
things happen we should ERROR out. Anything else seems likely to corrupt
the database further.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com