Re: ALTER TABLE ADD COLUMN fast default

Hi,
Thanks for the cleanup.

if (found != ncheck)
elog(ERROR, "%d constraint record(s) missing for rel %s",
ncheck - found, RelationGetRelationName(relation));

Since there is check on found being smaller than ncheck inside the loop,
the if condition can be written as:
if (found < ncheck)

Actually the above is implied by the expression, ncheck - found.

One minor comment w.r.t. the variable name is that, found is normally a
bool variable.
Maybe rename the variable nfound which would be clearer in its meaning.

+ if (found != ndef)
+ elog(WARNING, "%d attrdef record(s) missing for rel %s",
+ ndef - found, RelationGetRelationName(relation));

Since only warning is logged, there seems to be some wasted space in
attrdef. Would such space accumulate, resulting in some memory leak ?
I think the attrdef should be copied to another array of the proper size so
that there is no chance of memory leak.

Thanks

On Sun, Apr 4, 2021 at 9:05 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Andrew Dunstan <andrew(at)dunslane(dot)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.
>
> 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.
>
> 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?
>
> regards, tom lane
>
>