From: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Tender Wang <tndrwang@gmail.com>
Cc: Junwang Zhao <zhjwpku@gmail.com>
Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Cc: Alexander Lakhin <exclusion@gmail.com>
Cc: Guillaume Lelarge <guillaume@lelarge.info>
Cc: Michael Paquier <michael@paquier.xyz>
Subject: Re: [BUG] Fix DETACH with FK pointing to a partitioned table fails
Date: Tue, 22 Oct 2024 16:54:45 +0200
Message-ID: <20241022165445.23b3b712@karst> (raw)
In-Reply-To: <202410212152.ppnqfvqvxu3z@alvherre.pgsql>
References: <CAHewXN=pPz-8_qxUJuD5jhm3evXBeta1LgzW4PHArJ-iYz7qHQ@mail.gmail.com>
<202410212152.ppnqfvqvxu3z@alvherre.pgsql>
On Mon, 21 Oct 2024 23:52:18 +0200
Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> On 2024-Oct-21, Tender Wang wrote:
>
> > 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 != conform->conrelid)
> > {
> > ...
> > }
> >
> > The above contidion is always true according to my test.
> > I haven't figured out an anti-case.
>
> 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 always
true or not.
Maybe this was a leftover of some refactoring…
> 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 justified.
Regards,
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: jgdr@dalibo.com, alvherre@alvh.no-ip.org, tndrwang@gmail.com, zhjwpku@gmail.com, exclusion@gmail.com, guillaume@lelarge.info, michael@paquier.xyz
Subject: Re: [BUG] Fix DETACH with FK pointing to a partitioned table fails
In-Reply-To: <20241022165445.23b3b712@karst>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox