Thread: Re: not null constraints, again

Re: not null constraints, again

From

Alvaro Herrera

Date:

07 January, 18:55:37

On 2024-Dec-12, jian he wrote:

> patch attached.
> 
> also change comments of heap_create_with_catalog,
> StoreConstraints, MergeAttributes.
> so we can clear idea what's kind of constraints we are dealing with
> in these functions.

Great catch!  The patch looks good, I have pushed it.  Thank you.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Pido que me den el Nobel por razones humanitarias" (Nicanor Parra)

Re: not null constraints, again

From

Tom Lane

Date:

14 April, 20:13:30

The attached script simply creates two partitioned tables that
are connected by a foreign key constraint, then pg_dumps that
setup and tries to do a parallel restore.  This works up until

14e87ffa5c543b5f30ead7413084c25f7735039f is the first bad commit
commit 14e87ffa5c543b5f30ead7413084c25f7735039f
Author: Álvaro Herrera <alvherre@alvh.no-ip.org>
Date:   Fri Nov 8 13:28:48 2024 +0100

    Add pg_constraint rows for not-null constraints

Since that commit, it fails every time (for me, anyway, on a couple
of different machines) with a deadlock error, typically between
ALTER ADD PRIMARY KEY and one of the table COPY commands:

2025-04-14 12:54:49.892 EDT [1278062] ERROR:  deadlock detected
2025-04-14 12:54:49.892 EDT [1278062] DETAIL:  Process 1278062 waits for AccessExclusiveLock on relation 47164 of
database47159; blocked by process 1278059. 
    Process 1278059 waits for AccessShareLock on relation 47160 of database 47159; blocked by process 1278062.
    Process 1278062: ALTER TABLE ONLY public.parent1
        ADD CONSTRAINT parent1_pkey PRIMARY KEY (id);
    Process 1278059: COPY public.c11 (id, b) FROM stdin;

I stumbled across this result after wondering why the repro
I'd devised at [1] didn't fail in v17.

The patch I propose there seems to prevent this, but I wonder if we
shouldn't look closer into why it's failing in the first place.
I would not have expected that adding pg_constraint rows implies
stronger locks than what ALTER ADD PRIMARY KEY was using before,
and I suspect that doing so will cause more problems than just
breaking parallel restore.

            regards, tom lane

[1] https://www.postgresql.org/message-id/flat/2045026.1743801143@sss.pgh.pa.us

psql postgres <<EOF
CREATE DATABASE src;
\c src

CREATE TABLE parent1 (
    id integer PRIMARY KEY,
    b text
)
PARTITION BY LIST (id);

CREATE TABLE c11 PARTITION OF parent1 FOR VALUES IN (1);
CREATE TABLE c12 PARTITION OF parent1 FOR VALUES IN (2);

CREATE TABLE parent2 (
    id integer PRIMARY KEY,
    ref integer REFERENCES parent1,
    b text
)
PARTITION BY LIST (id);

CREATE TABLE c21 PARTITION OF parent2 FOR VALUES IN (1);
CREATE TABLE c22 PARTITION OF parent2 FOR VALUES IN (2);

INSERT INTO parent1 VALUES(1, 'foo');
INSERT INTO parent2 VALUES(2, 1, 'bar');
EOF

pg_dump src -f src.dump -Fc
createdb target
pg_restore src.dump -d target -j10

Re: not null constraints, again

From

Alvaro Herrera

Date:

15 April, 00:33:03

On 2025-Apr-14, Tom Lane wrote:

> The patch I propose there seems to prevent this, but I wonder if we
> shouldn't look closer into why it's failing in the first place.
> I would not have expected that adding pg_constraint rows implies
> stronger locks than what ALTER ADD PRIMARY KEY was using before,
> and I suspect that doing so will cause more problems than just
> breaking parallel restore.

I wasn't aware of this side effect.  I'll investigate this in more
depth.  I suspect it might be a bug in the way we run through ALTER
TABLE for the primary key.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Los cuentos de hadas no dan al niño su primera idea sobre los monstruos.
Lo que le dan es su primera idea de la posible derrota del monstruo."
                                                   (G. K. Chesterton)

Re: not null constraints, again

From

Tom Lane

Date:

15 April, 00:39:01

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On 2025-Apr-14, Tom Lane wrote:
>> I would not have expected that adding pg_constraint rows implies
>> stronger locks than what ALTER ADD PRIMARY KEY was using before,
>> and I suspect that doing so will cause more problems than just
>> breaking parallel restore.

> I wasn't aware of this side effect.  I'll investigate this in more
> depth.  I suspect it might be a bug in the way we run through ALTER
> TABLE for the primary key.

After further thought it occurs to me that it might not be a case
of "we get stronger locks", but a case of "we accidentally get a
weaker lock earlier and then try to upgrade it", thus creating a
possibility of deadlock where before we'd just have blocked till
the other statement cleared.  Still worthy of being fixed if that's
true, though.

            regards, tom lane

Re: not null constraints, again

From

Tender Wang

Date:

15 April, 06:20:53

Tom Lane <tgl@sss.pgh.pa.us> 于2025年4月15日周二 05:39写道：

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On 2025-Apr-14, Tom Lane wrote:
>> I would not have expected that adding pg_constraint rows implies
>> stronger locks than what ALTER ADD PRIMARY KEY was using before,
>> and I suspect that doing so will cause more problems than just
>> breaking parallel restore.

> I wasn't aware of this side effect. I'll investigate this in more
> depth. I suspect it might be a bug in the way we run through ALTER
> TABLE for the primary key.

After further thought it occurs to me that it might not be a case
of "we get stronger locks", but a case of "we accidentally get a
weaker lock earlier and then try to upgrade it", thus creating a
possibility of deadlock where before we'd just have blocked till
the other statement cleared. Still worthy of being fixed if that's
true, though.

I added sleep(1) in the DeadLockReport() before error report to display the status when a deadlock happened.

bool continue_sleep = true;
do
{
sleep(1);
} while (continue_sleep);
ereport(ERROR,
(errcode(ERRCODE_T_R_DEADLOCK_DETECTED),
errmsg("deadlock detected"),
errdetail_internal("%s", clientbuf.data),
errdetail_log("%s", logbuf.data),
errhint("See server log for query details.")));

ubuntu@VM-0-17-ubuntu:/workspace/postgres$ ps -ef|grep postgres
ubuntu 2911109 1 0 10:34 ? 00:00:00 /workspace/pgsql/bin/postgres -D ../data
ubuntu 2911110 2911109 0 10:34 ? 00:00:00 postgres: io worker 0
ubuntu 2911111 2911109 0 10:34 ? 00:00:00 postgres: io worker 1
ubuntu 2911112 2911109 0 10:34 ? 00:00:00 postgres: io worker 2
ubuntu 2911113 2911109 0 10:34 ? 00:00:00 postgres: checkpointer
ubuntu 2911114 2911109 0 10:34 ? 00:00:00 postgres: background writer
ubuntu 2911116 2911109 0 10:34 ? 00:00:00 postgres: walwriter
ubuntu 2911117 2911109 0 10:34 ? 00:00:00 postgres: autovacuum launcher
ubuntu 2911118 2911109 0 10:34 ? 00:00:00 postgres: logical replication launcher
ubuntu 2911180 2911109 0 10:34 ? 00:00:00 postgres: ubuntu target [local] COPY waiting
ubuntu 2911184 2911109 0 10:34 ? 00:00:00 postgres: ubuntu target [local] idle
ubuntu 2911187 2911109 0 10:34 ? 00:00:00 postgres: ubuntu target [local] idle
ubuntu 2911188 2911109 0 10:34 ? 00:00:00 postgres: ubuntu target [local] ALTER TABLE
ubuntu 2911189 2911109 0 10:34 ? 00:00:00 postgres: ubuntu target [local] SELECT waiting
ubuntu 2911190 2911109 0 10:34 ? 00:00:00 postgres: ubuntu target [local] idle
ubuntu 2911191 2911109 0 10:34 ? 00:00:00 postgres: ubuntu target [local] TRUNCATE TABLE waiting
ubuntu 2911192 2911109 0 10:34 ? 00:00:00 postgres: ubuntu target [local] idle
ubuntu 2911193 2911109 0 10:34 ? 00:00:00 postgres: ubuntu target [local] SELECT waiting
ubuntu 2911194 2911109 0 10:34 ? 00:00:00 postgres: ubuntu target [local] idle

gdb -p 2911188 // ALTER TABLE ONLY public.parent2 ADD CONSTRAINT parent2_pkey PRIMARY KEY (id);

(gdb) bt
#0 0x00007f2f6910878a in __GI___clock_nanosleep (clock_id=clock_id@entry=0, flags=flags@entry=0, req=req@entry=0x7ffc0e560e60, rem=rem@entry=0x7ffc0e560e60) at ../sysdeps/unix/sysv/linux/clock_nanosleep.c:78
#1 0x00007f2f6910d677 in __GI___nanosleep (req=req@entry=0x7ffc0e560e60, rem=rem@entry=0x7ffc0e560e60) at ../sysdeps/unix/sysv/linux/nanosleep.c:25
#2 0x00007f2f6910d5ae in __sleep (seconds=0) at ../sysdeps/posix/sleep.c:55
#3 0x0000561cd9386100 in DeadLockReport () at deadlock.c:1136
#4 0x0000561cd9389df8 in LockAcquireExtended (locktag=0x7ffc0e5610b0, lockmode=8, sessionLock=false, dontWait=false, reportMemoryError=true, locallockp=0x7ffc0e5610a8, logLockFailure=false) at lock.c:1232
#5 0x0000561cd93864bc in LockRelationOid (relid=16473, lockmode=8) at lmgr.c:115
#6 0x0000561cd8f21b20 in find_inheritance_children_extended (parentrelId=16463, omit_detached=true, lockmode=8, detached_exist=0x0, detached_xmin=0x0) at pg_inherits.c:213
#7 0x0000561cd8f217c1 in find_inheritance_children (parentrelId=16463, lockmode=8) at pg_inherits.c:60
#8 0x0000561cd904dd73 in ATPrepAddPrimaryKey (wqueue=0x7ffc0e561348, rel=0x7f2f5d7d6240, cmd=0x561cf1d2ee38, recurse=false, lockmode=8, context=0x7ffc0e561540) at tablecmds.c:9463
#9 0x0000561cd9043906 in ATPrepCmd (wqueue=0x7ffc0e561348, rel=0x7f2f5d7d6240, cmd=0x561cf1d2ee38, recurse=false, recursing=false, lockmode=8, context=0x7ffc0e561540) at tablecmds.c:5079
#10 0x0000561cd90432aa in ATController (parsetree=0x561cf1d062e0, rel=0x7f2f5d7d6240, cmds=0x561cf1d06290, recurse=false, lockmode=8, context=0x7ffc0e561540) at tablecmds.c:4871
#11 0x0000561cd9042f3b in AlterTable (stmt=0x561cf1d062e0, lockmode=8, context=0x7ffc0e561540) at tablecmds.c:4533
#12 0x0000561cd93bb7a8 in ProcessUtilitySlow (pstate=0x561cf1d2f9e0, pstmt=0x561cf1d06390, queryString=0x561cf1d05570 "ALTER TABLE ONLY public.parent2\n ADD CONSTRAINT parent2_pkey PRIMARY KEY (id);\n\n\n",
context=PROCESS_UTILITY_TOPLEVEL, params=0x0, queryEnv=0x0, dest=0x561cf1d06750, qc=0x7ffc0e561ba0) at utility.c:1321
#13 0x0000561cd93bb04e in standard_ProcessUtility (pstmt=0x561cf1d06390, queryString=0x561cf1d05570 "ALTER TABLE ONLY public.parent2\n ADD CONSTRAINT parent2_pkey PRIMARY KEY (id);\n\n\n", readOnlyTree=false,
context=PROCESS_UTILITY_TOPLEVEL, params=0x0, queryEnv=0x0, dest=0x561cf1d06750, qc=0x7ffc0e561ba0) at utility.c:1070
#14 0x0000561cd93b9f4c in ProcessUtility (pstmt=0x561cf1d06390, queryString=0x561cf1d05570 "ALTER TABLE ONLY public.parent2\n ADD CONSTRAINT parent2_pkey PRIMARY KEY (id);\n\n\n", readOnlyTree=false,
context=PROCESS_UTILITY_TOPLEVEL, params=0x0, queryEnv=0x0, dest=0x561cf1d06750, qc=0x7ffc0e561ba0) at utility.c:523
#15 0x0000561cd93b87a9 in PortalRunUtility (portal=0x561cf1d85d90, pstmt=0x561cf1d06390, isTopLevel=true, setHoldSnapshot=false, dest=0x561cf1d06750, qc=0x7ffc0e561ba0) at pquery.c:1185
#16 0x0000561cd93b8a52 in PortalRunMulti (portal=0x561cf1d85d90, isTopLevel=true, setHoldSnapshot=false, dest=0x561cf1d06750, altdest=0x561cf1d06750, qc=0x7ffc0e561ba0) at pquery.c:1349
#17 0x0000561cd93b7e73 in PortalRun (portal=0x561cf1d85d90, count=9223372036854775807, isTopLevel=true, dest=0x561cf1d06750, altdest=0x561cf1d06750, qc=0x7ffc0e561ba0) at pquery.c:820
#18 0x0000561cd93b037e in exec_simple_query (query_string=0x561cf1d05570 "ALTER TABLE ONLY public.parent2\n ADD CONSTRAINT parent2_pkey PRIMARY KEY (id);\n\n\n") at postgres.c:1274
#19 0x0000561cd93b5b46 in PostgresMain (dbname=0x561cf1d3f580 "target", username=0x561cf1d3f568 "ubuntu") at postgres.c:4771
#20 0x0000561cd93abab7 in BackendMain (startup_data=0x7ffc0e561e50, startup_data_len=24) at backend_startup.c:124
#21 0x0000561cd92abf09 in postmaster_child_launch (child_type=B_BACKEND, child_slot=2, startup_data=0x7ffc0e561e50, startup_data_len=24, client_sock=0x7ffc0e561eb0) at launch_backend.c:290
#22 0x0000561cd92b2946 in BackendStartup (client_sock=0x7ffc0e561eb0) at postmaster.c:3580
#23 0x0000561cd92afeb1 in ServerLoop () at postmaster.c:1702
#24 0x0000561cd92af7a2 in PostmasterMain (argc=3, argv=0x561cf1cffb00) at postmaster.c:1400
#25 0x0000561cd914cf06 in main (argc=3, argv=0x561cf1cffb00) at main.c:227

gdb -p 2911180 // COPY public.c22 (id, ref, b) FROM stdin;

(gdb) bt
#0 0x00007f2f69148dea in epoll_wait (epfd=5, events=0x561cf1d003a8, maxevents=1, timeout=-1) at ../sysdeps/unix/sysv/linux/epoll_wait.c:30
#1 0x0000561cd938106f in WaitEventSetWaitBlock (set=0x561cf1d00340, cur_timeout=-1, occurred_events=0x7ffc0e561000, nevents=1) at waiteventset.c:1190
#2 0x0000561cd9380f74 in WaitEventSetWait (set=0x561cf1d00340, timeout=-1, occurred_events=0x7ffc0e561000, nevents=1, wait_event_info=50331648) at waiteventset.c:1138
#3 0x0000561cd936f6a5 in WaitLatch (latch=0x7f2f665629e4, wakeEvents=33, timeout=0, wait_event_info=50331648) at latch.c:194
#4 0x0000561cd939fdbc in ProcSleep (locallock=0x561cf1d63560) at proc.c:1454
#5 0x0000561cd938b2c6 in WaitOnLock (locallock=0x561cf1d63560, owner=0x561cf1d44040) at lock.c:1968
#6 0x0000561cd9389dbd in LockAcquireExtended (locktag=0x7ffc0e5613f0, lockmode=1, sessionLock=false, dontWait=false, reportMemoryError=true, locallockp=0x7ffc0e5613e8, logLockFailure=false) at lock.c:1217
#7 0x0000561cd93864bc in LockRelationOid (relid=16463, lockmode=1) at lmgr.c:115
#8 0x0000561cd8daf922 in relation_open (relationId=16463, lockmode=1) at relation.c:55
#9 0x0000561cd95a29f3 in generate_partition_qual (rel=0x7f2f5d7d6640) at partcache.c:362
#10 0x0000561cd95a28b8 in RelationGetPartitionQual (rel=0x7f2f5d7d6640) at partcache.c:283
#11 0x0000561cd90bd59f in ExecPartitionCheck (resultRelInfo=0x561cf1d2ead8, slot=0x561cf1d33af8, estate=0x561cf1dde450, emitError=true) at execMain.c:1952
#12 0x0000561cd8fd1c9f in CopyFrom (cstate=0x561cf1de23a8) at copyfrom.c:1368
#13 0x0000561cd8fccd30 in DoCopy (pstate=0x561cf1d2f9e0, stmt=0x561cf1d06140, stmt_location=0, stmt_len=39, processed=0x7ffc0e561830) at copy.c:306
#14 0x0000561cd93ba623 in standard_ProcessUtility (pstmt=0x561cf1d06210, queryString=0x561cf1d05570 "COPY public.c22 (id, ref, b) FROM stdin;\n", readOnlyTree=false, context=PROCESS_UTILITY_TOPLEVEL, params=0x0,
queryEnv=0x0, dest=0x561cf1d065d0, qc=0x7ffc0e561ba0) at utility.c:738
#15 0x0000561cd93b9f4c in ProcessUtility (pstmt=0x561cf1d06210, queryString=0x561cf1d05570 "COPY public.c22 (id, ref, b) FROM stdin;\n", readOnlyTree=false, context=PROCESS_UTILITY_TOPLEVEL, params=0x0, queryEnv=0x0,
dest=0x561cf1d065d0, qc=0x7ffc0e561ba0) at utility.c:523
#16 0x0000561cd93b87a9 in PortalRunUtility (portal=0x561cf1d85d90, pstmt=0x561cf1d06210, isTopLevel=true, setHoldSnapshot=false, dest=0x561cf1d065d0, qc=0x7ffc0e561ba0) at pquery.c:1185
#17 0x0000561cd93b8a52 in PortalRunMulti (portal=0x561cf1d85d90, isTopLevel=true, setHoldSnapshot=false, dest=0x561cf1d065d0, altdest=0x561cf1d065d0, qc=0x7ffc0e561ba0) at pquery.c:1349
#18 0x0000561cd93b7e73 in PortalRun (portal=0x561cf1d85d90, count=9223372036854775807, isTopLevel=true, dest=0x561cf1d065d0, altdest=0x561cf1d065d0, qc=0x7ffc0e561ba0) at pquery.c:820
#19 0x0000561cd93b037e in exec_simple_query (query_string=0x561cf1d05570 "COPY public.c22 (id, ref, b) FROM stdin;\n") at postgres.c:1274
#20 0x0000561cd93b5b46 in PostgresMain (dbname=0x561cf1d3f580 "target", username=0x561cf1d3f568 "ubuntu") at postgres.c:4771
#21 0x0000561cd93abab7 in BackendMain (startup_data=0x7ffc0e561e50, startup_data_len=24) at backend_startup.c:124
#22 0x0000561cd92abf09 in postmaster_child_launch (child_type=B_BACKEND, child_slot=1, startup_data=0x7ffc0e561e50, startup_data_len=24, client_sock=0x7ffc0e561eb0) at launch_backend.c:290
#23 0x0000561cd92b2946 in BackendStartup (client_sock=0x7ffc0e561eb0) at postmaster.c:3580
#24 0x0000561cd92afeb1 in ServerLoop () at postmaster.c:1702
#25 0x0000561cd92af7a2 in PostmasterMain (argc=3, argv=0x561cf1cffb00) at postmaster.c:1400
#26 0x0000561cd914cf06 in main (argc=3, argv=0x561cf1cffb00) at main.c:227

The alter table session do check its children not-null constraint using lockmod=8, and the copy session do get partition_qual to lock parent using lockmode =1.

I wonder if we have to use the same lockmode for checking children's not-null constraint.

Thanks,

Tender Wang

Re: not null constraints, again

From

Tender Wang

Date:

15 April, 08:08:24

Tender Wang <tndrwang@gmail.com> 于2025年4月15日周二 11:20写道：

Tom Lane <tgl@sss.pgh.pa.us> 于2025年4月15日周二 05:39写道：
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On 2025-Apr-14, Tom Lane wrote:
>> I would not have expected that adding pg_constraint rows implies
>> stronger locks than what ALTER ADD PRIMARY KEY was using before,
>> and I suspect that doing so will cause more problems than just
>> breaking parallel restore.

> I wasn't aware of this side effect. I'll investigate this in more
> depth. I suspect it might be a bug in the way we run through ALTER
> TABLE for the primary key.

After further thought it occurs to me that it might not be a case
of "we get stronger locks", but a case of "we accidentally get a
weaker lock earlier and then try to upgrade it", thus creating a
possibility of deadlock where before we'd just have blocked till
the other statement cleared. Still worthy of being fixed if that's
true, though.

I added sleep(1) in the DeadLockReport() before error report to display the status when a deadlock happened.
bool continue_sleep = true;
do
{
sleep(1);
} while (continue_sleep);
ereport(ERROR,
(errcode(ERRCODE_T_R_DEADLOCK_DETECTED),
errmsg("deadlock detected"),
errdetail_internal("%s", clientbuf.data),
errdetail_log("%s", logbuf.data),
errhint("See server log for query details.")));

ubuntu@VM-0-17-ubuntu:/workspace/postgres$ ps -ef|grep postgres
ubuntu 2911109 1 0 10:34 ? 00:00:00 /workspace/pgsql/bin/postgres -D ../data
ubuntu 2911110 2911109 0 10:34 ? 00:00:00 postgres: io worker 0
ubuntu 2911111 2911109 0 10:34 ? 00:00:00 postgres: io worker 1
ubuntu 2911112 2911109 0 10:34 ? 00:00:00 postgres: io worker 2
ubuntu 2911113 2911109 0 10:34 ? 00:00:00 postgres: checkpointer
ubuntu 2911114 2911109 0 10:34 ? 00:00:00 postgres: background writer
ubuntu 2911116 2911109 0 10:34 ? 00:00:00 postgres: walwriter
ubuntu 2911117 2911109 0 10:34 ? 00:00:00 postgres: autovacuum launcher
ubuntu 2911118 2911109 0 10:34 ? 00:00:00 postgres: logical replication launcher
ubuntu 2911180 2911109 0 10:34 ? 00:00:00 postgres: ubuntu target [local] COPY waiting
ubuntu 2911184 2911109 0 10:34 ? 00:00:00 postgres: ubuntu target [local] idle
ubuntu 2911187 2911109 0 10:34 ? 00:00:00 postgres: ubuntu target [local] idle
ubuntu 2911188 2911109 0 10:34 ? 00:00:00 postgres: ubuntu target [local] ALTER TABLE
ubuntu 2911189 2911109 0 10:34 ? 00:00:00 postgres: ubuntu target [local] SELECT waiting
ubuntu 2911190 2911109 0 10:34 ? 00:00:00 postgres: ubuntu target [local] idle
ubuntu 2911191 2911109 0 10:34 ? 00:00:00 postgres: ubuntu target [local] TRUNCATE TABLE waiting
ubuntu 2911192 2911109 0 10:34 ? 00:00:00 postgres: ubuntu target [local] idle
ubuntu 2911193 2911109 0 10:34 ? 00:00:00 postgres: ubuntu target [local] SELECT waiting
ubuntu 2911194 2911109 0 10:34 ? 00:00:00 postgres: ubuntu target [local] idle

gdb -p 2911188 // ALTER TABLE ONLY public.parent2 ADD CONSTRAINT parent2_pkey PRIMARY KEY (id);
(gdb) bt
#0 0x00007f2f6910878a in __GI___clock_nanosleep (clock_id=clock_id@entry=0, flags=flags@entry=0, req=req@entry=0x7ffc0e560e60, rem=rem@entry=0x7ffc0e560e60) at ../sysdeps/unix/sysv/linux/clock_nanosleep.c:78
#1 0x00007f2f6910d677 in __GI___nanosleep (req=req@entry=0x7ffc0e560e60, rem=rem@entry=0x7ffc0e560e60) at ../sysdeps/unix/sysv/linux/nanosleep.c:25
#2 0x00007f2f6910d5ae in __sleep (seconds=0) at ../sysdeps/posix/sleep.c:55
#3 0x0000561cd9386100 in DeadLockReport () at deadlock.c:1136
#4 0x0000561cd9389df8 in LockAcquireExtended (locktag=0x7ffc0e5610b0, lockmode=8, sessionLock=false, dontWait=false, reportMemoryError=true, locallockp=0x7ffc0e5610a8, logLockFailure=false) at lock.c:1232
#5 0x0000561cd93864bc in LockRelationOid (relid=16473, lockmode=8) at lmgr.c:115
#6 0x0000561cd8f21b20 in find_inheritance_children_extended (parentrelId=16463, omit_detached=true, lockmode=8, detached_exist=0x0, detached_xmin=0x0) at pg_inherits.c:213
#7 0x0000561cd8f217c1 in find_inheritance_children (parentrelId=16463, lockmode=8) at pg_inherits.c:60
#8 0x0000561cd904dd73 in ATPrepAddPrimaryKey (wqueue=0x7ffc0e561348, rel=0x7f2f5d7d6240, cmd=0x561cf1d2ee38, recurse=false, lockmode=8, context=0x7ffc0e561540) at tablecmds.c:9463
#9 0x0000561cd9043906 in ATPrepCmd (wqueue=0x7ffc0e561348, rel=0x7f2f5d7d6240, cmd=0x561cf1d2ee38, recurse=false, recursing=false, lockmode=8, context=0x7ffc0e561540) at tablecmds.c:5079
#10 0x0000561cd90432aa in ATController (parsetree=0x561cf1d062e0, rel=0x7f2f5d7d6240, cmds=0x561cf1d06290, recurse=false, lockmode=8, context=0x7ffc0e561540) at tablecmds.c:4871
#11 0x0000561cd9042f3b in AlterTable (stmt=0x561cf1d062e0, lockmode=8, context=0x7ffc0e561540) at tablecmds.c:4533
#12 0x0000561cd93bb7a8 in ProcessUtilitySlow (pstate=0x561cf1d2f9e0, pstmt=0x561cf1d06390, queryString=0x561cf1d05570 "ALTER TABLE ONLY public.parent2\n ADD CONSTRAINT parent2_pkey PRIMARY KEY (id);\n\n\n",
context=PROCESS_UTILITY_TOPLEVEL, params=0x0, queryEnv=0x0, dest=0x561cf1d06750, qc=0x7ffc0e561ba0) at utility.c:1321
#13 0x0000561cd93bb04e in standard_ProcessUtility (pstmt=0x561cf1d06390, queryString=0x561cf1d05570 "ALTER TABLE ONLY public.parent2\n ADD CONSTRAINT parent2_pkey PRIMARY KEY (id);\n\n\n", readOnlyTree=false,
context=PROCESS_UTILITY_TOPLEVEL, params=0x0, queryEnv=0x0, dest=0x561cf1d06750, qc=0x7ffc0e561ba0) at utility.c:1070
#14 0x0000561cd93b9f4c in ProcessUtility (pstmt=0x561cf1d06390, queryString=0x561cf1d05570 "ALTER TABLE ONLY public.parent2\n ADD CONSTRAINT parent2_pkey PRIMARY KEY (id);\n\n\n", readOnlyTree=false,
context=PROCESS_UTILITY_TOPLEVEL, params=0x0, queryEnv=0x0, dest=0x561cf1d06750, qc=0x7ffc0e561ba0) at utility.c:523
#15 0x0000561cd93b87a9 in PortalRunUtility (portal=0x561cf1d85d90, pstmt=0x561cf1d06390, isTopLevel=true, setHoldSnapshot=false, dest=0x561cf1d06750, qc=0x7ffc0e561ba0) at pquery.c:1185
#16 0x0000561cd93b8a52 in PortalRunMulti (portal=0x561cf1d85d90, isTopLevel=true, setHoldSnapshot=false, dest=0x561cf1d06750, altdest=0x561cf1d06750, qc=0x7ffc0e561ba0) at pquery.c:1349
#17 0x0000561cd93b7e73 in PortalRun (portal=0x561cf1d85d90, count=9223372036854775807, isTopLevel=true, dest=0x561cf1d06750, altdest=0x561cf1d06750, qc=0x7ffc0e561ba0) at pquery.c:820
#18 0x0000561cd93b037e in exec_simple_query (query_string=0x561cf1d05570 "ALTER TABLE ONLY public.parent2\n ADD CONSTRAINT parent2_pkey PRIMARY KEY (id);\n\n\n") at postgres.c:1274
#19 0x0000561cd93b5b46 in PostgresMain (dbname=0x561cf1d3f580 "target", username=0x561cf1d3f568 "ubuntu") at postgres.c:4771
#20 0x0000561cd93abab7 in BackendMain (startup_data=0x7ffc0e561e50, startup_data_len=24) at backend_startup.c:124
#21 0x0000561cd92abf09 in postmaster_child_launch (child_type=B_BACKEND, child_slot=2, startup_data=0x7ffc0e561e50, startup_data_len=24, client_sock=0x7ffc0e561eb0) at launch_backend.c:290
#22 0x0000561cd92b2946 in BackendStartup (client_sock=0x7ffc0e561eb0) at postmaster.c:3580
#23 0x0000561cd92afeb1 in ServerLoop () at postmaster.c:1702
#24 0x0000561cd92af7a2 in PostmasterMain (argc=3, argv=0x561cf1cffb00) at postmaster.c:1400
#25 0x0000561cd914cf06 in main (argc=3, argv=0x561cf1cffb00) at main.c:227

gdb -p 2911180 // COPY public.c22 (id, ref, b) FROM stdin;
(gdb) bt
#0 0x00007f2f69148dea in epoll_wait (epfd=5, events=0x561cf1d003a8, maxevents=1, timeout=-1) at ../sysdeps/unix/sysv/linux/epoll_wait.c:30
#1 0x0000561cd938106f in WaitEventSetWaitBlock (set=0x561cf1d00340, cur_timeout=-1, occurred_events=0x7ffc0e561000, nevents=1) at waiteventset.c:1190
#2 0x0000561cd9380f74 in WaitEventSetWait (set=0x561cf1d00340, timeout=-1, occurred_events=0x7ffc0e561000, nevents=1, wait_event_info=50331648) at waiteventset.c:1138
#3 0x0000561cd936f6a5 in WaitLatch (latch=0x7f2f665629e4, wakeEvents=33, timeout=0, wait_event_info=50331648) at latch.c:194
#4 0x0000561cd939fdbc in ProcSleep (locallock=0x561cf1d63560) at proc.c:1454
#5 0x0000561cd938b2c6 in WaitOnLock (locallock=0x561cf1d63560, owner=0x561cf1d44040) at lock.c:1968
#6 0x0000561cd9389dbd in LockAcquireExtended (locktag=0x7ffc0e5613f0, lockmode=1, sessionLock=false, dontWait=false, reportMemoryError=true, locallockp=0x7ffc0e5613e8, logLockFailure=false) at lock.c:1217
#7 0x0000561cd93864bc in LockRelationOid (relid=16463, lockmode=1) at lmgr.c:115
#8 0x0000561cd8daf922 in relation_open (relationId=16463, lockmode=1) at relation.c:55
#9 0x0000561cd95a29f3 in generate_partition_qual (rel=0x7f2f5d7d6640) at partcache.c:362
#10 0x0000561cd95a28b8 in RelationGetPartitionQual (rel=0x7f2f5d7d6640) at partcache.c:283
#11 0x0000561cd90bd59f in ExecPartitionCheck (resultRelInfo=0x561cf1d2ead8, slot=0x561cf1d33af8, estate=0x561cf1dde450, emitError=true) at execMain.c:1952
#12 0x0000561cd8fd1c9f in CopyFrom (cstate=0x561cf1de23a8) at copyfrom.c:1368
#13 0x0000561cd8fccd30 in DoCopy (pstate=0x561cf1d2f9e0, stmt=0x561cf1d06140, stmt_location=0, stmt_len=39, processed=0x7ffc0e561830) at copy.c:306
#14 0x0000561cd93ba623 in standard_ProcessUtility (pstmt=0x561cf1d06210, queryString=0x561cf1d05570 "COPY public.c22 (id, ref, b) FROM stdin;\n", readOnlyTree=false, context=PROCESS_UTILITY_TOPLEVEL, params=0x0,
queryEnv=0x0, dest=0x561cf1d065d0, qc=0x7ffc0e561ba0) at utility.c:738
#15 0x0000561cd93b9f4c in ProcessUtility (pstmt=0x561cf1d06210, queryString=0x561cf1d05570 "COPY public.c22 (id, ref, b) FROM stdin;\n", readOnlyTree=false, context=PROCESS_UTILITY_TOPLEVEL, params=0x0, queryEnv=0x0,
dest=0x561cf1d065d0, qc=0x7ffc0e561ba0) at utility.c:523
#16 0x0000561cd93b87a9 in PortalRunUtility (portal=0x561cf1d85d90, pstmt=0x561cf1d06210, isTopLevel=true, setHoldSnapshot=false, dest=0x561cf1d065d0, qc=0x7ffc0e561ba0) at pquery.c:1185
#17 0x0000561cd93b8a52 in PortalRunMulti (portal=0x561cf1d85d90, isTopLevel=true, setHoldSnapshot=false, dest=0x561cf1d065d0, altdest=0x561cf1d065d0, qc=0x7ffc0e561ba0) at pquery.c:1349
#18 0x0000561cd93b7e73 in PortalRun (portal=0x561cf1d85d90, count=9223372036854775807, isTopLevel=true, dest=0x561cf1d065d0, altdest=0x561cf1d065d0, qc=0x7ffc0e561ba0) at pquery.c:820
#19 0x0000561cd93b037e in exec_simple_query (query_string=0x561cf1d05570 "COPY public.c22 (id, ref, b) FROM stdin;\n") at postgres.c:1274
#20 0x0000561cd93b5b46 in PostgresMain (dbname=0x561cf1d3f580 "target", username=0x561cf1d3f568 "ubuntu") at postgres.c:4771
#21 0x0000561cd93abab7 in BackendMain (startup_data=0x7ffc0e561e50, startup_data_len=24) at backend_startup.c:124
#22 0x0000561cd92abf09 in postmaster_child_launch (child_type=B_BACKEND, child_slot=1, startup_data=0x7ffc0e561e50, startup_data_len=24, client_sock=0x7ffc0e561eb0) at launch_backend.c:290
#23 0x0000561cd92b2946 in BackendStartup (client_sock=0x7ffc0e561eb0) at postmaster.c:3580
#24 0x0000561cd92afeb1 in ServerLoop () at postmaster.c:1702
#25 0x0000561cd92af7a2 in PostmasterMain (argc=3, argv=0x561cf1cffb00) at postmaster.c:1400
#26 0x0000561cd914cf06 in main (argc=3, argv=0x561cf1cffb00) at main.c:227

The alter table session do check its children not-null constraint using lockmod=8, and the copy session do get partition_qual to lock parent using lockmode =1.
I wonder if we have to use the same lockmode for checking children's not-null constraint.

I thought further about the lockmode calling find_inheritance_children in ATPrepAddPrimaryKey.

What we do here? We first get oids of children, then check the if the column of children has marked not-null, if not, report an error.

No operation here on children. I check other places that call find_inheritance_children, if we have operation on children, we usually pass

Lockmode to find_inheritance_children, otherwise pass NoLock.

I try NoLock, then restore-deadlock.sh will have no error.

Thanks,

Tender Wang

Re: not null constraints, again

From

Alvaro Herrera

Date:

15 April, 22:11:49

On 2025-Apr-15, Tender Wang wrote:

> I thought further about the lockmode calling find_inheritance_children
> in ATPrepAddPrimaryKey.
> What we do here? We first get oids of children, then check the if the
> column of children has marked not-null,  if not, report an error.
> No operation here on children.  I check other places that call
> find_inheritance_children, if we have operation on children, we usually pass
> Lockmode to find_inheritance_children, otherwise pass NoLock.

Hmm, I'm wary of doing this, although you're perhaps right that there's
no harm.  If we do need to add a not-null constraint on the children,
surely we'll acquire a stronger lock further down the execution chain.
In principle this sounds a good idea though.  (I'm not sure about doing
SearchSysCacheAttName() on a relation that might be dropped
concurrently; does dropping the child acquire lock on its parent?  I
suppose so, in which case this is okay; but still icky.  What about
DETACH CONCURRENTLY?)

However, I've also been looking at this and realized that this code can
have different structure which may allows us to skip the
find_inheritance_children() altogether.  The reason is that we already
scan the parent's list of columns searching for not-null constraints on
each of them; we only need to run this verification on children for
columns where there is none in the parent, and then only in the case
where recursion is turned off.

So I propose the attached patch, which also has some comments to
hopefully explain what is going on and why.  I ran Tom's test script a
few hundred times in a loop and I see no deadlock anymore.

Note that I also considered the idea of just not doing the check at all;
that is, if a child table doesn't have a not-null constraint, then let
  ALTER TABLE ONLY parent ADD PRIMARY KEY ( ... )
create the not-null constraint.  This works fine (it breaks one
regression test query though, would be easily fixed).  But I don't like
this very much, because it means the user could be surprised by the
lengthy unexpected runtime of creating the primary key, only to realize
that the server is checking the child table for nulls.  This is
especially bad if the user says ONLY.  I think it's better if they have
the chance to create the not-null constraint on their own volition.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Learn about compilers. Then everything looks like either a compiler or
a database, and now you have two problems but one of them is fun."
            https://twitter.com/thingskatedid/status/1456027786158776329

Attachment

0001-Fix-verification-of-not-null-constraints-on-children.patch

Re: not null constraints, again

From

Tom Lane

Date:

15 April, 22:51:14

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> However, I've also been looking at this and realized that this code can
> have different structure which may allows us to skip the
> find_inheritance_children() altogether.  The reason is that we already
> scan the parent's list of columns searching for not-null constraints on
> each of them; we only need to run this verification on children for
> columns where there is none in the parent, and then only in the case
> where recursion is turned off.

+1.  Fundamentally the problem here is that pg_restore needs

ALTER TABLE ONLY foo ADD PRIMARY KEY

to not recurse to child tables at all.  It is expecting this command
to acquire a lock on foo and nothing else; and it has already taken
care of making foo's PK column(s) NOT NULL, so there is no reason we
should have to examine the children.

Looking at the patch itself, it doesn't seem like the got_children
flag is accomplishing anything; I guess that was leftover from an
earlier version?  You could declare "List *children" inside the
block where it's used, too.  Basically, this patch is just moving
the check-the-children logic from one place to another.

Also I find the comments still a bit confusing, but maybe that's
on me.

            regards, tom lane

Re: not null constraints, again

From

Alvaro Herrera

Date:

16 April, 00:10:26

On 2025-Apr-15, Tom Lane wrote:

> +1.  Fundamentally the problem here is that pg_restore needs
> 
> ALTER TABLE ONLY foo ADD PRIMARY KEY
> 
> to not recurse to child tables at all.  It is expecting this command
> to acquire a lock on foo and nothing else; and it has already taken
> care of making foo's PK column(s) NOT NULL, so there is no reason we
> should have to examine the children.

Right.

> Looking at the patch itself, it doesn't seem like the got_children
> flag is accomplishing anything; I guess that was leftover from an
> earlier version?  You could declare "List *children" inside the
> block where it's used, too.  Basically, this patch is just moving
> the check-the-children logic from one place to another.

Ah yes, I forgot to set got_children when reading the children list.
This happens within the loop for columns, so the idea is to obtain that
list just once instead of once per column.  I don't think there's any
ill effect from doing it multiple times, but it's wasted work and that's
what led me to adding got_children.  I'll add the assignment.

> Also I find the comments still a bit confusing, but maybe that's
> on me.

I'll review tomorrow morning, maybe I can find some improvements for
them.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"La conclusión que podemos sacar de esos estudios es que
no podemos sacar ninguna conclusión de ellos" (Tanenbaum)

Re: not null constraints, again

From

Tom Lane

Date:

16 April, 00:12:42

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On 2025-Apr-15, Tom Lane wrote:
>> Looking at the patch itself, it doesn't seem like the got_children
>> flag is accomplishing anything;

> Ah yes, I forgot to set got_children when reading the children list.
> This happens within the loop for columns, so the idea is to obtain that
> list just once instead of once per column.

Ah, got it.  Makes sense as long as you actually avoid the work ;-)

            regards, tom lane

Re: not null constraints, again

From

Tender Wang

Date:

16 April, 09:28:27

Alvaro Herrera <alvherre@alvh.no-ip.org> 于2025年4月16日周三 03:12写道：

On 2025-Apr-15, Tender Wang wrote:

> I thought further about the lockmode calling find_inheritance_children
> in ATPrepAddPrimaryKey.
> What we do here? We first get oids of children, then check the if the
> column of children has marked not-null, if not, report an error.
> No operation here on children. I check other places that call
> find_inheritance_children, if we have operation on children, we usually pass
> Lockmode to find_inheritance_children, otherwise pass NoLock.

Hmm, I'm wary of doing this, although you're perhaps right that there's
no harm. If we do need to add a not-null constraint on the children,
surely we'll acquire a stronger lock further down the execution chain.
In principle this sounds a good idea though. (I'm not sure about doing
SearchSysCacheAttName() on a relation that might be dropped
concurrently; does dropping the child acquire lock on its parent? I
suppose so, in which case this is okay; but still icky. What about
DETACH CONCURRENTLY?)

Yes, I'm also wary of doing this.

Although NoLock may fix this issue, I feel it will trigger other problems, such as the scenario you listed above.

However, I've also been looking at this and realized that this code can
have different structure which may allows us to skip the
find_inheritance_children() altogether. The reason is that we already
scan the parent's list of columns searching for not-null constraints on
each of them; we only need to run this verification on children for
columns where there is none in the parent, and then only in the case
where recursion is turned off.

So I propose the attached patch, which also has some comments to
hopefully explain what is going on and why. I ran Tom's test script a
few hundred times in a loop and I see no deadlock anymore.

No objection from me. The comments may need a little polishing.

Thanks,

Tender Wang