Thread: Another FK violation when referencing a multi-level partitionedtable

Hello,

When working on the patch to fix another FK violation [1], I found that FK
constraints were not properly cloned to partition not directly hooked to the
root table.

CloneFkReferenced takes care to avoid inherited constraints to clone top-level
constraints only:

    /*
     * Search for any constraints where this partition is in the referenced
     * side.  However, we must ignore any constraint whose parent constraint
     * is also going to be cloned, to avoid duplicates. [...]
     */

But it seems the top-level constraints are actually never cloned neither.

Surprisingly, the comment explains how this should be done in two steps, but
the code corrupted the first step by skipping inherited constraints and lacks
the second step:


     * [...]to avoid duplicates.  So do it in two
     * steps: first construct the list of constraints to clone, then go over
     * that list cloning those whose parents are not in the list.  (We must
     * not rely on the parent being seen first, since the catalog scan could
     * return children first.)
     */

Please, find in attachment a proposal patch to fix this FK violation.

Regards,

[1] https://www.postgresql.org/message-id/20200204183906.115f693e%40firost

On Thu, Feb 06, 2020 at 12:49:48AM +0100, Jehan-Guillaume de Rorthais wrote:
> Please, find in attachment a proposal patch to fix this FK violation.

Indeed.  I would have expected the deletion of the referenced rows to
happen as in your test.  At quick glance, your fix looks right, but I
need to study more this code.  Alvaro, would you like to weigh in as
the author of f56f8f8?
--
Michael

On 2020-Feb-06, Michael Paquier wrote:

> On Thu, Feb 06, 2020 at 12:49:48AM +0100, Jehan-Guillaume de Rorthais wrote:
> > Please, find in attachment a proposal patch to fix this FK violation.
> 
> Indeed.  I would have expected the deletion of the referenced rows to
> happen as in your test.  At quick glance, your fix looks right, but I
> need to study more this code.  Alvaro, would you like to weigh in as
> the author of f56f8f8?

Sure, I'll have a look at this this afternoon.

Kindly do CC me when mentioning me in messages.  I don't read every
mailing list message ...

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2020-Feb-06, Jehan-Guillaume de Rorthais wrote:

Hello,

> When working on the patch to fix another FK violation [1], I found that FK
> constraints were not properly cloned to partition not directly hooked to the
> root table.

Uh.

> Surprisingly, the comment explains how this should be done in two steps, but
> the code corrupted the first step by skipping inherited constraints and lacks
> the second step:
> 
>      * [...]to avoid duplicates.  So do it in two
>      * steps: first construct the list of constraints to clone, then go over
>      * that list cloning those whose parents are not in the list.  (We must
>      * not rely on the parent being seen first, since the catalog scan could
>      * return children first.)
>      */

Strange that this escaped testing previously.

> Please, find in attachment a proposal patch to fix this FK violation.

You fix looks correct to me, so pushed.  I took a minute to apply some
minor corrections to the comments, too.

Thanks for reporting!

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Fri, 7 Feb 2020 18:30:51 -0300
Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
...
> > Please, find in attachment a proposal patch to fix this FK violation.  
> 
> You fix looks correct to me, so pushed.  I took a minute to apply some
> minor corrections to the comments, too.
> 
> Thanks for reporting!

Oh, you actually pushed it already. Thank you!

Regards,

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2020-Feb-06, Jehan-Guillaume de Rorthais wrote:
>> Please, find in attachment a proposal patch to fix this FK violation.

> You fix looks correct to me, so pushed.  I took a minute to apply some
> minor corrections to the comments, too.

FWIW, I think the test query is too smart for its own good:

SELECT fk.fk_a, pk.a
FROM fkpart9.fk
LEFT JOIN fkpart9.pk ON fk.fk_a = pk.a
WHERE fk.fk_a=35;

The trouble with this is that the planner would be fully within its
rights to conclude that the FK constraint means a pk row must exist
for every fk row, and hence strength-reduce the left join to a
plain join.  That would mask this bug, if it ever reappeared.
There's no such optimization today, but people have proposed patches
that do that or very similar things, so I have no confidence in the
long-term safety of this test case.  I'd replace the above SELECT
with just

SELECT * FROM fkpart9.fk;

and maybe also similarly select all of fkpart9.pk, if you wanted
to verify that the right thing had happened on that side.

            regards, tom lane

Hello,
On Sun, 09 Feb 2020 12:48:49 -0500
Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > On 2020-Feb-06, Jehan-Guillaume de Rorthais wrote:  
> >> Please, find in attachment a proposal patch to fix this FK violation.  
> 
> > You fix looks correct to me, so pushed.  I took a minute to apply some
> > minor corrections to the comments, too.  
> 
> FWIW, I think the test query is too smart for its own good:
> [...]

Indeed, thank you. Please find in attachment a patch making the test simpler.

Regards,

On 2020-Feb-10, Jehan-Guillaume de Rorthais wrote:

> > FWIW, I think the test query is too smart for its own good:
> > [...]
> 
> Indeed, thank you. Please find in attachment a patch making the test simpler.

Thanks!  Pushed.

I also signed this commit, as an experiment.  Our gitweb doesn't show
anything, but github adds a "verified" badge to it.  (I had to upload my
public GPG key for this.)  Nothing in the git command line shows
anything about the commit being signed either; you have to ask for it
explicitly.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Thu, 20 Feb 2020 16:43:54 -0300
Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

> On 2020-Feb-10, Jehan-Guillaume de Rorthais wrote:
> 
>  [...]  
>  [...]  
> 
> Thanks!  Pushed.

Thanks you Alvaro and Tom.