Bug in detaching a partition with a foreign key.

This is a bug indeed. I tried your patch, but it ends up in a seg fault.

I also see this was raised in another thread [0]/messages/by-id/CAHewXNm5rtfQZNv2uWkiHZVJeicFFa4x7p0=y-x2vAM0vorgNQ@mail.gmail.com.

It can be reproduced in a slightly simplified case, using only a
single level partition.

"""
CREATE TABLE bar(id int PRIMARY KEY) PARTITION BY RANGE(id);
CREATE TABLE bar_p0 PARTITION OF bar FOR VALUES FROM (0) TO (100);

CREATE TABLE foo(id int) PARTITION BY RANGE(id);
CREATE TABLE foo_p0 PARTITION OF foo FOR VALUES FROM (0) TO (100);

ALTER TABLE foo_p0 ADD CONSTRAINT child_fk_con FOREIGN KEY (id) REFERENCES bar;

ALTER TABLE foo DETACH PARTITION foo_p0 ;
"""

Here is what I I found.

In DetachPartitionFinalize, after the child is detached from
the parent, the FK's insert and update triggers are then
removed from pg_depend with TriggerSetParentTrigger

/*
* The constraint on this table must be marked no longer a child of
* the parent's constraint, as do its check triggers.
*/
ConstraintSetParentConstraint(fk->conoid, InvalidOid, InvalidOid);

/*
* Also, look up the partition's "check" triggers corresponding to the
* constraint being detached and detach them from the parent triggers.
*/
GetForeignKeyCheckTriggers(trigrel,
fk->conoid, fk->confrelid, fk->conrelid,
&insertTriggerOid, &updateTriggerOid);
Assert(OidIsValid(insertTriggerOid));
TriggerSetParentTrigger(trigrel, insertTriggerOid, InvalidOid,
RelationGetRelid(partRel));
Assert(OidIsValid(updateTriggerOid));
TriggerSetParentTrigger(trigrel, updateTriggerOid, InvalidOid,
RelationGetRelid(partRel));

Specifically, the dependency types DEPENDENCY_PARTITION_PRI and
DEPENDENCY_PARTITION_SEC are removed from pg_depend.

deleteDependencyRecordsForClass(TriggerRelationId, childTrigId,
TriggerRelationId,
DEPENDENCY_PARTITION_PRI);
deleteDependencyRecordsForClass(TriggerRelationId, childTrigId,
RelationRelationId,
DEPENDENCY_PARTITION_SEC);

In the repro case, an FK on a partition with a reference
to a partition parent table does not create a new dependency.

postgres=# SELECT count(*) total, deptype FROM pg_depend WHERE deptype
in ('P', 'S') group by deptype;
total | deptype
-------+---------
2 | P
2 | S
(2 rows)

postgres=# ALTER TABLE foo_p0 ADD CONSTRAINT child_fk_con FOREIGN KEY
(id) REFERENCES bar;
ALTER TABLE
postgres=#
postgres=# SELECT count(*) total, deptype FROM pg_depend WHERE deptype
in ('P', 'S') group by deptype;
total | deptype
-------+---------
2 | P
2 | S
(2 rows)

We also see that the FK riggers created are associated with the parent
constraint rather than the child constraint, i.e. tgconstraint = 17387

postgres=# ALTER TABLE foo_p0 ADD CONSTRAINT child_fk_con FOREIGN KEY
(id) REFERENCES bar;
ALTER TABLE
postgres=#
postgres=# SELECT oid, tgrelid::regclass relname, tgparentid, 'insert'
trigger_type, tgconstraint FROM pg_trigger WHERE tgfoid in (1644,
1645)
postgres-# and tgtype & (1 << 2) > 0
postgres-# union all
postgres-# SELECT oid, tgrelid::regclass relname, tgparentid, 'update'
trigger_type, tgconstraint FROM pg_trigger WHERE tgfoid in (1644,
1645)
postgres-# and tgtype & (1 << 4) > 0;
oid | relname | tgparentid | trigger_type | tgconstraint
-------+---------+------------+--------------+--------------
17393 | foo_p0 | 0 | insert | 17387
17394 | foo_p0 | 0 | update | 17387
(2 rows)

postgres=# ALTER TABLE foo DETACH PARTITION foo_p0 ;
ERROR: could not find ON INSERT check triggers of foreign key constraint 17390
postgres=#
postgres=# select oid, conparentid, conrelid::regclass,
confrelid::regclass from pg_constraint where oid = 17390;
oid | conparentid | conrelid | confrelid
-------+-------------+----------+-----------
17390 | 17387 | foo_p0 | bar_p0
(1 row)

postgres=# select oid, conparentid, conrelid::regclass,
confrelid::regclass from pg_constraint where oid = 17387;
oid | conparentid | conrelid | confrelid
-------+-------------+----------+-----------
17387 | 0 | foo_p0 | bar
(1 row)

This is not the case when the constraint is created on
the parent table,
i.e. ALTER TABLE foo ADD CONSTRAINT child_fk_con FOREIGN KEY (id) REFERENCES bar

In this case we also see a dependency.

postgres=# SELECT count(*) total, deptype FROM pg_depend WHERE deptype
in ('P', 'S') group by deptype;
total | deptype
-------+---------
2 | P
2 | S
(2 rows)

postgres=# ALTER TABLE foo ADD CONSTRAINT child_fk_con FOREIGN KEY
(id) REFERENCES bar;
ALTER TABLE
postgres=# SELECT count(*) total, deptype FROM pg_depend WHERE deptype
in ('P', 'S') group by deptype;
total | deptype
-------+---------
3 | P
3 | S
(2 rows)

Also, the constraint relname in the parent constraint is that of the
parent table and the child constraint is that of the child table.

postgres=*# ALTER TABLE foo DETACH PARTITION foo_p0 ;
ALTER TABLE
postgres=*# ROLLBACK;
ROLLBACK
^
postgres=# select oid, conparentid, conrelid::regclass,
confrelid::regclass from pg_constraint where oid = 17455;
oid | conparentid | conrelid | confrelid
-------+-------------+----------+-----------
17455 | 17447 | foo_p0 | bar
(1 row)

postgres=# select oid, conparentid, conrelid::regclass,
confrelid::regclass from pg_constraint where oid = 17447;
oid | conparentid | conrelid | confrelid
-------+-------------+----------+-----------
17447 | 0 | foo | bar
(1 row

If the relation on the parent and child constraint match, that
tells us we don't have inheritance.
So, I am thinking we should add another condition for checking
if a foreign key is inherited by checking if the parent constraint
relation is different from the child constraint relation.

I am attaching an unpolished patch ( we need test coverage as well ) that
implements the above. All tests pass with this patch.

Regards,

Sami Imseih
Amazon Web Services (AWS)

[0]: /messages/by-id/CAHewXNm5rtfQZNv2uWkiHZVJeicFFa4x7p0=y-x2vAM0vorgNQ@mail.gmail.com

On Sat, Jan 18, 2025 at 2:14 AM Sami Imseih <samimseih@gmail.com> wrote:

This is a bug indeed. I tried your patch, but it ends up in a seg fault.

[...]
If the relation on the parent and child constraint match, that
tells us we don't have inheritance.
So, I am thinking we should add another condition for checking
if a foreign key is inherited by checking if the parent constraint
relation is different from the child constraint relation.

I am attaching an unpolished patch ( we need test coverage as well ) that
implements the above. All tests pass with this patch.

Thanks for confirming the bug. TBH, I haven't had a chance to look into the
issue in detail yet, but my understanding of the detach operation is that it
should cut the link between the parent constraint and the constraint on
the partition being detached. The trial patch I posted was meant to prevent
cutting the link between the partition's constraint and the constraint that
inherits it. Your patch seems to achieve the same, AFAIUC, but I'm not sure I
like the way it's doing, nor the trial patch I posted. A crash is certainly
possible with that patch, but it would be helpful if you could provide more
context -- such as crash detail and/or a test case.

Regards,
Amul

should cut the link between the parent constraint and the constraint on
the partition being detached.

correct by setting the conparentid to 0 in pg_constraint and to delete
the pg_depend record for partition dependency. But in the repro case,
we don't have a dependency as the table the foreign key is on is a
partition.

The trial patch I posted was meant to prevent
cutting the link between the partition's constraint and the constraint that
inherits it. Your patch seems to achieve the same, AFAIUC,

Yes, exactly.

but it would be helpful if you could provide more
context -- such as crash detail and/or a test case.

Below is the repro I used. Similar as you original repro,
but without subpartition on foo_p0. This also results in the segfault
with your attached patch.

""
CREATE TABLE bar(id int PRIMARY KEY) PARTITION BY RANGE(id);
CREATE TABLE bar_p0 PARTITION OF bar FOR VALUES FROM (0) TO (100);
CREATE TABLE foo(id int) PARTITION BY RANGE(id);
CREATE TABLE foo_p0 PARTITION OF foo FOR VALUES FROM (0) TO (100);
ALTER TABLE foo_p0 ADD CONSTRAINT child_fk_con FOREIGN KEY (id) REFERENCES bar;
ALTER TABLE foo DETACH PARTITION foo_p0;
"""

Here is the core dump
"""
Using host libthread_db library "/lib64/libthread_db.so.1".
Core was generated by `postgres: postgres postgres [local] ALTER T'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0 GetMemoryChunkMethodID (pointer=0x0) at mcxt.c:205
205 header = *((const uint64 *) ((const char *) pointer -
sizeof(uint64)));

Thread 1 (Thread 0xffffb6e34f00 (LWP 17508)):
#0 GetMemoryChunkMethodID (pointer=0x0) at mcxt.c:205
header = 12273896
#1 0x0000000000c05edc in repalloc (pointer=0x0, size=128) at mcxt.c:1566
ret = 0x10b5b98380
#2 0x00000000007fbf24 in enlarge_list (list=0x18cd1490, min_size=1)
at list.c:209
new_max_len = 16
#3 0x00000000007fc22c in new_tail_cell (list=0x18cd1490) at list.c:327
No locals.
#4 0x00000000007fc334 in lappend_oid (list=0x18cd1490, datum=16404)
at list.c:382
No locals.
"""

I was able to get your patch working by changing the way the fksid
is built with the below:

/* Collect all the constraint ids */
foreach(cell, fks)
{
ForeignKeyCacheInfo *fk = lfirst(cell);
fksids = lappend_oid(fksids, fk->conoid);
}

Regards,

Sami

On 2025-Jan-20, Sami Imseih wrote:

Below is the repro I used. Similar as you original repro,
but without subpartition on foo_p0. This also results in the segfault
with your attached patch.

I think the issue in Amul's patch is just that the list was not
initialized to NIL.

Other than the lack of comments, which I'm in the process of writing,
the initial patch Amul submitted seems largely correct to me, in my
understanding of the situation. I'll throw in a few tests, hoping that
it'll prove correct, and if so then I'll get it pushed soon.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"El hombre nunca sabe de lo que es capaz hasta que lo intenta" (C. Dickens)

The patch that Amul and I wrote both achieve the same result.
The approach that Amul took builds a list of constraint OIDs,
which could grow with the number of partitions and foreign keys
on those partitions. Maybe not a big deal?

In my suggestion [1]/messages/by-id/CAA5RZ0vk4SJ9PiD2RyG-CKOYvOFewz6QweKcp2_EegBKns=dOA@mail.gmail.com, I just do one extra pg_constraint lookup
to determine If the relation on the parent and child constraint match,
and in that case we can skip the rest of the work to cut the
link as it's not needed.

Regards,

Sami

[1]: /messages/by-id/CAA5RZ0vk4SJ9PiD2RyG-CKOYvOFewz6QweKcp2_EegBKns=dOA@mail.gmail.com

On 2025-Jan-20, Sami Imseih wrote:

The patch that Amul and I wrote both achieve the same result.
The approach that Amul took builds a list of constraint OIDs,
which could grow with the number of partitions and foreign keys
on those partitions. Maybe not a big deal?

Nope, not a big deal. It would be a big deal if we were talking about
268 million partitions (>1GB palloc size), but that's impractical for
other reasons.

In my suggestion [1], I just do one extra pg_constraint lookup
to determine If the relation on the parent and child constraint match,
and in that case we can skip the rest of the work to cut the
link as it's not needed.

A pg_constraint lookup is going to be a lot slower.

I attach Amul's patch again with the comments I added and your test
case. Needs adaptation for backpatching to 16 and 15.

Thanks!

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/

The patch that Amul and I wrote both achieve the same result.
The approach that Amul took builds a list of constraint OIDs,
which could grow with the number of partitions and foreign keys
on those partitions. Maybe not a big deal?

Nope, not a big deal. It would be a big deal if we were talking about
268 million partitions (>1GB palloc size), but that's impractical for
other reasons.

that's fair.

Patch looks good to me, but I am not sure about this part of the comment:

"Only the topmost one is to be considered here; the child constraints
must be left alone,"

In this case, none of the pg_constraint entries are actually considered. right?

Regards,

Sami

On 2025-Jan-20, Sami Imseih wrote:

Patch looks good to me, but I am not sure about this part of the comment:

"Only the topmost one is to be considered here; the child constraints
must be left alone,"

In this case, none of the pg_constraint entries are actually considered. right?

Right. The parent one because it has conparentid = 0, the others
because their parent constraint is in the list.

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/

On 2025-Jan-20, Sami Imseih wrote:

Patch looks good to me,

Thanks, pushed.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/

On Tue, Jan 21, 2025 at 7:25 PM Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:

On 2025-Jan-20, Sami Imseih wrote:

Patch looks good to me,

Thanks, pushed.

A big thanks to Álvaro and Sami for getting it fixed!

Regards,
Amul