Обсуждение: Assertion failure with ALTER TABLE ATTACH PARTITION withlog_min_messages >= DEBUG1

Hi all,

Running installcheck on an instance with log_min_messages = DEBUG1, I
can bump into the following assertion failure:
#2  0x000056145231e82c in ExceptionalCondition
(conditionName=0x56145258ae0b "!(strvalue != ((void *)0))",
errorType=0x56145258adfb "FailedAssertion",
    fileName=0x56145258adf0 "snprintf.c", lineNumber=440) at assert.c:54
[...]
#7  0x000056145231f518 in errmsg (fmt=0x5614524dac60 "validating foreign
key constraint \"%s\"") at elog.c:796
#8  0x0000561451f6ab54 in validateForeignKeyConstraint (conname=0x0,
rel=0x7f12833ca750, pkrel=0x7f12833cc468, pkindOid=36449,
constraintOid=36466) at tablecmds.c:8566
#9  0x0000561451f61589 in ATRewriteTables (parsetree=0x561453bde5e0,
wqueue=0x7ffe8f1d55e8, lockmode=8) at tablecmds.c:4549

Looking at the stack trace there is this log in
validateForeignKeyConstraint:
ereport(DEBUG1,
        (errmsg("validating foreign key constraint \"%s\"", conname)));

However conname is set to NULL in this code path.

This test case allows to reproduce easily the failure:
CREATE TABLE fk_notpartitioned_pk (a int, b int, PRIMARY KEY (a, b));
CREATE TABLE fk_partitioned_fk (b int, a int) PARTITION BY RANGE (a, b);
CREATE TABLE fk_partitioned_fk_1 (b int, a int);
ALTER TABLE fk_partitioned_fk ADD FOREIGN KEY (a, b) REFERENCES
fk_notpartitioned_pk;
-- crash
ALTER TABLE fk_partitioned_fk ATTACH PARTITION fk_partitioned_fk_1 FOR
  VALUES FROM (1,1) TO (2,2);

From what I can see the problem comes from CloneForeignKeyConstraint
which forgets to assign the constraint name when cloning the FK
definition.  While looking at the ATTACH PARTITION code, I have noticed
that a variable gets overridden, which is in my opinion bad style.  So
the problem is rather close to what Tom has fixed in 3d0f68dd it seems.

Attached is a patch for all that, with which installcheck-world passes
for me.  I am surprised this was not noticed before, the recent snprintf
stanza is nicely helping, and this would need to be back-patched down to
v11.

Thanks,
--
Michael

On 2018-Oct-05, Michael Paquier wrote:

> Looking at the stack trace there is this log in
> validateForeignKeyConstraint:
> ereport(DEBUG1,
>         (errmsg("validating foreign key constraint \"%s\"", conname)));
> 
> However conname is set to NULL in this code path.

Ouch.  Thanks for catching this one.  I think the "this is all we need"
comment is just asking for trouble :-(

> From what I can see the problem comes from CloneForeignKeyConstraint
> which forgets to assign the constraint name when cloning the FK
> definition.  While looking at the ATTACH PARTITION code, I have noticed
> that a variable gets overridden, which is in my opinion bad style.

Ugh, yeah that looks bad.  I wish the compiler would warn about this :-(

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

On Fri, Oct 05, 2018 at 12:41:29PM -0300, Alvaro Herrera wrote:
> On 2018-Oct-05, Michael Paquier wrote:
>> Looking at the stack trace there is this log in
>> validateForeignKeyConstraint:
>> ereport(DEBUG1,
>>         (errmsg("validating foreign key constraint \"%s\"", conname)));
>>
>> However conname is set to NULL in this code path.
>
> Ouch.  Thanks for catching this one.  I think the "this is all we need"
> comment is just asking for trouble :-(

Would you reformulate it?  Like, say, if new fields are needed perhaps
we could just say instead "XXX: make sure to update the list of fields
copied if a new partition-relation command needs it."

>> From what I can see the problem comes from CloneForeignKeyConstraint
>> which forgets to assign the constraint name when cloning the FK
>> definition.  While looking at the ATTACH PARTITION code, I have noticed
>> that a variable gets overridden, which is in my opinion bad style.
>
> Ugh, yeah that looks bad.  I wish the compiler would warn about this :-(

Do you want me to take care of this one?  On this issue, I am way more
confident than the other thread for event triggers as I spent quite some
time on it.
--
Michael

On 2018-Oct-06, Michael Paquier wrote:

> On Fri, Oct 05, 2018 at 12:41:29PM -0300, Alvaro Herrera wrote:
> > On 2018-Oct-05, Michael Paquier wrote:
> >> Looking at the stack trace there is this log in
> >> validateForeignKeyConstraint:
> >> ereport(DEBUG1,
> >>         (errmsg("validating foreign key constraint \"%s\"", conname)));
> >> 
> >> However conname is set to NULL in this code path.
> > 
> > Ouch.  Thanks for catching this one.  I think the "this is all we need"
> > comment is just asking for trouble :-(
> 
> Would you reformulate it?  Like, say, if new fields are needed perhaps
> we could just say instead "XXX: make sure to update the list of fields
> copied if a new partition-relation command needs it."

Well, I think partially filling the struct is bad style.  I'm going to
be messing with that shortly anyway, when adding support for FKs
pointing to partitioned tables; maybe just leave it as is for now and
I'll see about that later.

> >> From what I can see the problem comes from CloneForeignKeyConstraint
> >> which forgets to assign the constraint name when cloning the FK
> >> definition.  While looking at the ATTACH PARTITION code, I have noticed
> >> that a variable gets overridden, which is in my opinion bad style.
> > 
> > Ugh, yeah that looks bad.  I wish the compiler would warn about this :-(
> 
> Do you want me to take care of this one?  On this issue, I am way more
> confident than the other thread for event triggers as I spent quite some
> time on it.

Please feel free if you have the time, thanks.

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

On Fri, Oct 05, 2018 at 11:27:59PM -0300, Alvaro Herrera wrote:
> Well, I think partially filling the struct is bad style.  I'm going to
> be messing with that shortly anyway, when adding support for FKs
> pointing to partitioned tables; maybe just leave it as is for now and
> I'll see about that later.

Yes, I agree that we had better change that on HEAD as that's a trap
waiting ahead.  I have let the comment as-is then.

> Please feel free if you have the time, thanks.

Okay, done and back-patched down to v11 where this was introduced.
--
Michael