Re: ALTER TABLE ADD COLUMN fast default - Mailing list pgsql-hackers
| From | Zhihong Yu |
|---|---|
| Subject | Re: ALTER TABLE ADD COLUMN fast default |
| Date | |
| Msg-id | CALNJ-vShzp9Nrh-e-_Xrzrgq+CkQrk9eyrUa3OzgO5t=FkUqWg@mail.gmail.com Whole thread Raw |
| In response to | Re: ALTER TABLE ADD COLUMN fast default (Tom Lane <tgl@sss.pgh.pa.us>) |
| Responses |
Re: ALTER TABLE ADD COLUMN fast default
(Tom Lane <tgl@sss.pgh.pa.us>)
|
| List | pgsql-hackers |
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
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@sss.pgh.pa.us> 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.
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
pgsql-hackers by date: