Date: Tue, 22 Oct 2024 16:54:45 +0200
From: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Tender Wang <tndrwang@gmail.com>, Junwang Zhao <zhjwpku@gmail.com>,
 "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>, Alexander
 Lakhin <exclusion@gmail.com>, Guillaume Lelarge <guillaume@lelarge.info>,
 Michael Paquier <michael@paquier.xyz>
Subject: Re: [BUG] Fix DETACH with FK pointing to a partitioned table fails
Message-ID: <20241022165445.23b3b712@karst>
In-Reply-To: <202410212152.ppnqfvqvxu3z@alvherre.pgsql>
References: 
 <CAHewXN=pPz-8_qxUJuD5jhm3evXBeta1LgzW4PHArJ-iYz7qHQ@mail.gmail.com>
	<202410212152.ppnqfvqvxu3z@alvherre.pgsql>
Organization: Dalibo
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/20241022165445.23b3b712%40karst>
Precedence: bulk

On Mon, 21 Oct 2024 23:52:18 +0200
Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

> On 2024-Oct-21, Tender Wang wrote:
>=20
> > I suspect that we don't need the below if
> > statement anymore.
> > /*
> > * If the referenced side is partitioned (which we know because our
> > * parent's constraint points to a different relation than ours) then
> > * we must, in addition to the above, create pg_constraint rows that
> > * point to each partition, each with its own action triggers.
> > */
> > if (parentConForm->conrelid !=3D conform->conrelid)
> > {
> > ...
> > }
> >=20
> > The above contidion is always true according to my test.
> > I haven't figured out an anti-case. =20
>=20
> You're right, this is useless, we can remove the 'if' line.  I kept the
> block though, to have a place for all those local variable declarations
> (otherwise the code looks messier than it needs to).

I'm confused the original patch was considering the trigger creation
on the referenced side ONLY when it was partitioned, which was the point of
this conditional block (as the comment said), no matter if the test was alw=
ays
true or not.

Maybe this was a leftover of some refactoring=E2=80=A6

> I also noticed that addFkRecurseReferenced() is uselessly taking a List
> **wqueue argument but doesn't use it, so I removed it (as fallout, other
> routines don't need it either, especially DetachPartitionFinalize).  I
> added some commentary on the creation of dependencies in
> addFkConstraint().

Good catch.

Looking at this, on a side note, I realize now addFkRecurseReferenced() and
addFkRecurseReferencing() are not exactly mirroring each other as the later
is doing more than the former.

Either this is fine, or maybe this is the sign something might need some
refactoring as addFkRecurseReferencing() carry more than it should. I'm not=
 sure
it deserve more than a comment for futur work/study, if only this is justif=
ied.

Regards,