Re: [BUG] wrong FK constraint name when colliding name on ATTACH

On Fri, Sep 9, 2022 at 4:54 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Andres Freund <andres@anarazel.de> writes:
> > Something here doesn't look to be quite right. Starting with this commit CI
> > [1] started to fail on freebsd (stack trace [2]), and in the meson branch I've
> > also seen the crash on windows (CI run[3], stack trace [4]).
>
> The crash seems 100% reproducible if I remove the early-exit optimization
> from GetForeignKeyActionTriggers:

Indeed, reproduced here.

> diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
> index 53b0f3a9c1..112ca77d97 100644
> --- a/src/backend/commands/tablecmds.c
> +++ b/src/backend/commands/tablecmds.c
> @@ -10591,8 +10591,6 @@ GetForeignKeyActionTriggers(Relation trigrel,
>             Assert(*updateTriggerOid == InvalidOid);
>             *updateTriggerOid = trgform->oid;
>         }
> -       if (OidIsValid(*deleteTriggerOid) && OidIsValid(*updateTriggerOid))
> -           break;
>     }
>
>     if (!OidIsValid(*deleteTriggerOid))
>
> With that in place, it's probabilistic whether the Asserts notice anything
> wrong, and mostly they don't.  But there are multiple matching triggers:
>
> regression=# select oid, tgconstraint, tgrelid,tgconstrrelid, tgtype, tgname from pg_trigger where tgconstraint = 104301;
>   oid   | tgconstraint | tgrelid | tgconstrrelid | tgtype |            tgname
> --------+--------------+---------+---------------+--------+-------------------------------
>  104302 |       104301 |  104294 |        104294 |      9 | RI_ConstraintTrigger_a_104302
>  104303 |       104301 |  104294 |        104294 |     17 | RI_ConstraintTrigger_a_104303
>  104304 |       104301 |  104294 |        104294 |      5 | RI_ConstraintTrigger_c_104304
>  104305 |       104301 |  104294 |        104294 |     17 | RI_ConstraintTrigger_c_104305
> (4 rows)
>
> I suspect that the filter conditions being applied are inadequate
> for the case of a self-referential FK, which this evidently is
> given that tgrelid and tgconstrrelid are equal.

Yes, the loop in GetForeignKeyActionTriggers() needs this:

+       /* Only ever look at "action" triggers on the PK side. */
+       if (RI_FKey_trigger_type(trgform->tgfoid) != RI_TRIGGER_PK)
+           continue;

Likewise, GetForeignKeyActionTriggers() needs this:

+       /* Only ever look at "check" triggers on the FK side. */
+       if (RI_FKey_trigger_type(trgform->tgfoid) != RI_TRIGGER_FK)
+           continue;

We evidently missed this in f4566345cf40b0.

> I'd counsel dropping the early-exit optimization; it doesn't
> save much I expect, and it evidently hides bugs.  Or maybe
> make it conditional on !USE_ASSERT_CHECKING.

While neither of these functions are called in hot paths, I am
inclined to keep the early-exit bit in non-assert builds.

Attached a patch.

-- 
Thanks, Amit Langote
EDB: http://www.enterprisedb.com