From: Tom Lane <tgl@sss.pgh.pa.us>
To: Adrian Klaver <adrian.klaver@aklaver.com>
cc: Alvaro Herrera <alvherre@alvh.no-ip.org>, user@pidu.dev,
        pgsql-general@lists.postgresql.org
Subject: Re: Fwd: Fwd: Postgres attach partition: AccessExclusive lock set on different tables depending on how attaching is performed
In-reply-to: <19dbfe75-8b98-49af-bf03-0b7d72aa02e8@aklaver.com>
References: <CAPDhG9aVAt6ed7Lji=G3XhcGRFz0FxHkGiuxmRu=hZgWW9D0OA@mail.gmail.com> <254c31c2-ff15-4720-a4fc-f0969d677d72@aklaver.com> <CAPDhG9aZMS_nocLJ86bFHHqd1t=QOdztcR0Uy8BC5xpFrdes3A@mail.gmail.com> <CAPDhG9bvTyiCG_Z0pmuD_57z7xCbSn2ExkCvAbcgtkmMws9u8A@mail.gmail.com> <4ebbbcf5-9c35-4711-bd7c-06a56a31aeff@aklaver.com> <CAPDhG9YM2B-P4ozEznSKXas+LGe6yDW4-QE=eKcxtvnis9eOvQ@mail.gmail.com> <CAPDhG9YqwVzXFFZ+19XggHYsYaRVROGxoTn=E6GS_J=t5tpAdQ@mail.gmail.com> <a38e4e1b-36ae-423a-8b06-0023a7e2630a@aklaver.com> <CAPDhG9YaMonzBGb5PTG7_a_n2-hfGvZfiLszEdvQ3oBFJx+5kA@mail.gmail.com> <19dbfe75-8b98-49af-bf03-0b7d72aa02e8@aklaver.com>
Comments: In-reply-to Adrian Klaver <adrian.klaver@aklaver.com>
	message dated "Sun, 10 Nov 2024 14:16:46 -0800"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <174129.1731282279.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Sun, 10 Nov 2024 18:44:39 -0500
Message-ID: <174130.1731282279@sss.pgh.pa.us>
Archived-At: <https://www.postgresql.org/message-id/174130.1731282279%40sss.pgh.pa.us>
Precedence: bulk

Adrian Klaver <adrian.klaver@aklaver.com> writes:
> 3) Because of 1 & 2, this needs further analysis by someone or someones =

> more knowledgeable.

This surprised me a bit too, because I thought we took a
slightly-less-than-exclusive lock for FK additions or deletions.
Tracing through it, I find that CloneFkReferencing opens the
referenced relation with ShareRowExclusiveLock as I expected.
But then we conclude that we can drop the existing FK enforcement
triggers for the table being attached.  That causes us to take
AccessExclusiveLock on the trigger itself, which is fine because
nobody's really paying attention to that.  But then RemoveTriggerById
takes AccessExclusiveLock on the trigger's table.  We already had
that on the table being attached, but not on the other table.

This is quite bad, not just because the AccessExclusiveLock might
block other stuff but because we are doing a lock upgrade on the
referenced table, greatly increasing the risk of deadlock.

I wonder whether it'd be all right for RemoveTriggerById to take
only ShareRowExclusiveLock on the trigger's table.  This seems
OK in terms of basic semantics: that's enough to lock out
anything that might want to fire triggers on the table.  However,
this comment for AlterTableGetLockLevel gives me pause:

 * Also note that pg_dump uses only an AccessShareLock, meaning that anyth=
ing
 * that takes a lock less than AccessExclusiveLock can change object defin=
itions
 * while pg_dump is running. Be careful to check that the appropriate data=
 is
 * derived by pg_dump using an MVCC snapshot, rather than syscache lookups=
,
 * otherwise we might end up with an inconsistent dump that can't restore.

I think pg_dump uses pg_get_triggerdef, which is probably not
safe in these terms.

An alternative answer might be what Alvaro was muttering about
the other day: redesign FKs for partitioned tables so that we
do not have to change the set of triggers when attaching/detaching.

			regards, tom lane