Обсуждение: Trigger violates foreign key constraint

CREATE TABLE parent (id integer PRIMARY KEY);

CREATE TABLE child (id integer REFERENCES parent ON DELETE CASCADE);

CREATE FUNCTION silly() RETURNS trigger LANGUAGE plpgsql AS 'BEGIN RETURN NULL; END;';

CREATE TRIGGER silly BEFORE DELETE ON child FOR EACH ROW EXECUTE FUNCTION silly();

INSERT INTO parent VALUES (1);

INSERT INTO child VALUES (1);

DELETE FROM parent WHERE id = 1;

TABLE child;
 id
════
  1
(1 row)

The trigger function cancels the cascaded delete on "child", and we are left with
a row in "child" that references no row in "parent".

Yours,
Laurenz Albe

Perhaps it would be enough to run "RI_FKey_noaction_del" after
"RI_FKey_cascade_del", although that would impact the performance.

Yours,
Laurenz Albe

Laurenz Albe <laurenz.albe@cybertec.at> writes:
> CREATE FUNCTION silly() RETURNS trigger LANGUAGE plpgsql AS 'BEGIN RETURN NULL; END;';
> CREATE TRIGGER silly BEFORE DELETE ON child FOR EACH ROW EXECUTE FUNCTION silly();

> The trigger function cancels the cascaded delete on "child", and we are left with
> a row in "child" that references no row in "parent".

Yes.  This is by design: triggers operate at a lower level than
foreign keys, so an ill-conceived trigger can break an FK constraint.
That's documented somewhere, though maybe not visibly enough.

There are good reasons to want triggers to be able to see and
react to FK-driven updates, so it's unlikely that we'd want to
revisit that design decision, even if it hadn't already stood
for decades.

            regards, tom lane

On Mon, 2023-10-02 at 09:49 -0400, Tom Lane wrote:
> Laurenz Albe <laurenz.albe@cybertec.at> writes:
> > CREATE FUNCTION silly() RETURNS trigger LANGUAGE plpgsql AS 'BEGIN RETURN NULL; END;';
> > CREATE TRIGGER silly BEFORE DELETE ON child FOR EACH ROW EXECUTE FUNCTION silly();
>
> > The trigger function cancels the cascaded delete on "child", and we are left with
> > a row in "child" that references no row in "parent".
>
> Yes.  This is by design: triggers operate at a lower level than
> foreign keys, so an ill-conceived trigger can break an FK constraint.
> That's documented somewhere, though maybe not visibly enough.
>
> There are good reasons to want triggers to be able to see and
> react to FK-driven updates, so it's unlikely that we'd want to
> revisit that design decision, even if it hadn't already stood
> for decades.

Thanks for the clarification.  I keep learning.

I didn't find anything about that in the documentation or the
READMEs in the source, but perhaps I didn't look well enough.

Yours,
Laurenz Albe

On Mon, 2023-10-02 at 09:49 -0400, Tom Lane wrote:
> This is by design: triggers operate at a lower level than
> foreign keys, so an ill-conceived trigger can break an FK constraint.
> That's documented somewhere, though maybe not visibly enough.

Not having found any documentation, I propose the attached caution.

Yours,
Laurenz Albe

On Mon, Oct 02, 2023 at 09:49:53AM -0400, Tom Lane wrote:
> Laurenz Albe <laurenz.albe@cybertec.at> writes:
> > CREATE FUNCTION silly() RETURNS trigger LANGUAGE plpgsql AS 'BEGIN RETURN NULL; END;';
> > CREATE TRIGGER silly BEFORE DELETE ON child FOR EACH ROW EXECUTE FUNCTION silly();
> 
> > The trigger function cancels the cascaded delete on "child", and we are left with
> > a row in "child" that references no row in "parent".
> 
> Yes.  This is by design: triggers operate at a lower level than
> foreign keys, so an ill-conceived trigger can break an FK constraint.
> That's documented somewhere, though maybe not visibly enough.
> 
> There are good reasons to want triggers to be able to see and
> react to FK-driven updates,

I agree with that, but I also think it's a bug that other triggers can
invalidate the constraint, without even going out of their way to do so.
Ideally, triggers would be able to react, yet when all non-superuser-defined
code settles, the constraint would still hold.  While UNIQUE indexes over
expressions aren't that strict, at least for those you need to commit the
clear malfeasance of redefining an IMMUTABLE function.

On Mon, Oct 02, 2023 at 12:02:17PM +0200, Laurenz Albe wrote:
> Perhaps it would be enough to run "RI_FKey_noaction_del" after
> "RI_FKey_cascade_del", although that would impact the performance.

Yes.  A cure that doubles the number of heap fetches would be worse than the
disease, but a more-optimized version of this idea could work.  The FK system
could use a broader optimization-oriented rewrite, to deal with the unbounded
memory usage and redundant I/O.  If that happens, it could be a good time to
plan for closing the trigger hole.

The following review has been posted through the commitfest application:
make installcheck-world:  not tested
Implements feature:       not tested
Spec compliant:           not tested
Documentation:            tested, passed

It seems like people have been talking about this problem since 2010
(https://stackoverflow.com/questions/3961825/foreign-keys-in-postgresql-can-be-violated-by-trigger),and we finally got
ouract together and put it in the official document today, only 13 years later. 

The new status of this patch is: Ready for Committer

Thanks for having a look at my patch!

On Mon, 2023-10-30 at 15:03 -0700, David G. Johnston wrote:
> On Mon, Oct 30, 2023 at 2:50 PM David G. Johnston <david.g.johnston@gmail.com> wrote:
> > On Tue, Oct 3, 2023 at 12:52 AM Laurenz Albe <laurenz.albe@cybertec.at> wrote:
> > > On Mon, 2023-10-02 at 09:49 -0400, Tom Lane wrote:
> > > > This is by design: triggers operate at a lower level than
> > > > foreign keys, so an ill-conceived trigger can break an FK constraint.
> > > > That's documented somewhere, though maybe not visibly enough.
> > >
> > > Not having found any documentation, I propose the attached caution.
> >
> > I dislike scaring the user like this without providing any context on what
> > conditions or actions are problematic.

I specifically *want* to scare^H^H^H^H^Halert the user, and I thought I
provided sufficient context and a link to a more detailed description of
how triggers behave.
What is unclear or lacking in the proposed wording?

  In particular, other triggers
  defined on the referencing table can cancel or modify the effects of
  cascading delete or update, thereby breaking referential integrity.

> > The ON DELETE and ON UPDATE clauses of foreign keys are implemented as system triggers
> > on the referenced table that invoke additional delete or update commands on the
> > referencing table.  The final outcome of these additional commands are not checked -
> > it is the responsibility of the DBA to ensure that the user triggers on the
> > referencing table actually remove the rows they are requested to remove, or
> > update to NULL any referencing foreign key columns.  In particular, before row
> > triggers that return NULL will prevent the delete/update from occurring and thus
> > result in a violated foreign key constraint.

I didn't plan to write a novel on the topic... and I don't think your wording is
clearer than mine.  I went over my text again with the intent to add clarity, but
apart from a few minor modifications ("other triggers" -> "user-defined triggers")
I couldn't make it clearer.  I'd have to write an example to make it clearer,
and that would certainly be out of scope.

> > Add sgml as needed, note the original patch missed adding "<productname>" to PostgreSQL.

Ah, thanks for noticing!  Fixed.

>
> Additionally, the existing place this is covered is here:
>
> [https://www.postgresql.org/docs/current/trigger-definition.html]
>
> We should probably add a note pointing back to the DDL chapter and that more concisely says.
>
> "Note: If this table also contains any foreign key constraints with on update
> or on delete clauses, then a failure to return the same row that was passed in
> for update and delete triggers is going to result in broken referential integrity
> for the affected row."

My patch already contains a link to this very section.

I tried to understand your sentence and had to read it several times.  I don't
think that it adds clarity to my patch.



Attached is a slightly modified version of the patch.

Yours,
Laurenz Albe

Hi,

> Attached is a slightly modified version of the patch.

The patch was marked as "Needs Review" so I decided to take a look.

I believe it's a good patch. The text is well written, has the
necessary references, it warns the user but doesn't scare him/her too
much.

> I couldn't make it clearer.  I'd have to write an example to make it clearer,
> and that would certainly be out of scope.

Personally I don't think we should document particular steps to break
FK constraints. In a similar situation when we documented the
limitations about XID wraparound we didn't document particular steps
to trigger XID wraparound. I don't recall encountering such examples
in the documentation, so I don't think we should add them here, at
least for consistency.

All in all the patch seems to be as good as it will get. I suggest merging it.

-- 
Best regards,
Aleksander Alekseev

I fully support this addition to the documentation. The legal 
possibility of breaking data consistency must be documented at least.

Please, consider small suggestion to replace last sentence.

- This is not considered a bug, and it is the responsibility of the user 
to write triggers so that such problems are avoided.
+ It is the trigger programmer's responsibility to avoid such scenarios.

To be consistent with the sentence about recursive trigger calls: [1]
"It is the trigger programmer's responsibility to avoid infinite 
recursion in such scenarios."

Also I don't really like "This is not considered a bug" part, since it 
looks like an excuse.


1. https://www.postgresql.org/docs/devel/trigger-definition.html

-- 
Pavel Luzanov
Postgres Professional: https://postgrespro.com

One more not documented issue with system triggers. It might be worth 
considering together.

CREATE ROLE app_owner;

CREATE TABLE t (
     id        int PRIMARY KEY,
     parent_id int REFERENCES t(id)
);

ALTER TABLE t OWNER TO app_owner;

-- No actions by application owner
REVOKE ALL ON t FROM app_owner;

INSERT INTO t VALUES (1,NULL);

DELETE FROM t;
ERROR:  permission denied for table t
CONTEXT:  SQL statement "SELECT 1 FROM ONLY "public"."t" x WHERE "id" 
OPERATOR(pg_catalog.=) $1 FOR KEY SHARE OF x"

It is not at all obvious why the superuser cannot delete the row that he 
just added. The reason is that system triggers are executed with the 
rights of the table owner, not the current role. But I can't find a 
description of this behavior in the documentation.

-- 
Pavel Luzanov
Postgres Professional: https://postgrespro.com

On Fri, 2023-12-22 at 10:59 +0300, Pavel Luzanov wrote:
> Please, consider small suggestion to replace last sentence.
>
> - This is not considered a bug, and it is the responsibility of the user
> to write triggers so that such problems are avoided.
> + It is the trigger programmer's responsibility to avoid such scenarios.
>
> To be consistent with the sentence about recursive trigger calls: [1]
> "It is the trigger programmer's responsibility to avoid infinite
> recursion in such scenarios."

Yes, that is better - shorter and avoids passive mode.  Changed.

> Also I don't really like "This is not considered a bug" part, since it
> looks like an excuse.

In a way, it is an excuse, so why not be honest about it.

The example you provided in your other message (cascading triggers
fail if the table ovner has revoked the required permissions from
herself) is not really about breaking foreign keys.  You hit a
surprising error, but referential integrity will be maintained.

Patch v3 is attached.

Yours,
Laurenz Albe

Yes, that is better - shorter and avoids passive mode.  Changed.

Also I don't really like "This is not considered a bug" part, since it 
looks like an excuse.

In a way, it is an excuse, so why not be honest about it.

The example you provided in your other message (cascading triggers
fail if the table ovner has revoked the required permissions from
herself) is not really about breaking foreign keys.  You hit a
surprising error, but referential integrity will be maintained.

Patch v3 is attached.

-- 
Pavel Luzanov
Postgres Professional: https://postgrespro.com

Laurenz Albe <laurenz.albe@cybertec.at> writes:
> Patch v3 is attached.

I agree with documenting this hazard, but I think it'd be better
to do so in the "Triggers" chapter.  There is no hazard unless
you are writing user-defined triggers, which is surely far fewer
people than use foreign keys.  So I suggest something like the
attached.

            regards, tom lane

diff --git a/doc/src/sgml/trigger.sgml b/doc/src/sgml/trigger.sgml
index a5390ff644..99d8b75fc5 100644
--- a/doc/src/sgml/trigger.sgml
+++ b/doc/src/sgml/trigger.sgml
@@ -354,6 +354,20 @@
     to avoid infinite recursion in such scenarios.
    </para>
 
+   <para>
+    <productname>PostgreSQL</productname> implements foreign key
+    constraints via system-defined <literal>AFTER</literal> triggers.
+    If a foreign key constraint specifies referential actions (that is,
+    cascading updates or deletes), these triggers issue ordinary SQL
+    update or delete commands on the referencing table to perform the
+    actions.  In particular, any user-defined triggers on the referencing
+    table will be fired.  If such a trigger modifies or blocks the effect
+    of one of these commands, the end result could be to break referential
+    integrity.  It is the trigger programmer's responsibility to avoid
+    that.  (Similar problems can arise in any case where different
+    triggers are working at cross-purposes.)
+   </para>
+
    <para>
     <indexterm>
      <primary>trigger</primary>

Hi,

> Laurenz Albe <laurenz.albe@cybertec.at> writes:
> > Patch v3 is attached.
>
> I agree with documenting this hazard, but I think it'd be better
> to do so in the "Triggers" chapter.  There is no hazard unless
> you are writing user-defined triggers, which is surely far fewer
> people than use foreign keys.  So I suggest something like the
> attached.

I don't mind changing the chapter, but I prefer the wording chosen in
v3. The explanation in v4 is somewhat hard to follow IMO.

-- 
Best regards,
Aleksander Alekseev

Aleksander Alekseev <aleksander@timescale.com> writes:
>> I agree with documenting this hazard, but I think it'd be better
>> to do so in the "Triggers" chapter.  There is no hazard unless
>> you are writing user-defined triggers, which is surely far fewer
>> people than use foreign keys.  So I suggest something like the
>> attached.

> I don't mind changing the chapter, but I prefer the wording chosen in
> v3. The explanation in v4 is somewhat hard to follow IMO.

Well, I didn't like v3 because I thought it was wrong, or at least
fairly misleading.  The fact that we use triggers rather than some
other mechanism to invoke referential updates isn't really relevant.
The issue is that the update actions fire user-defined triggers,
and it's those that are the (potential) problem.

Perhaps we should leave the system triggers out of the discussion
entirely?  More or less like:

    If a foreign key constraint specifies referential actions (that
    is, cascading updates or deletes), those actions are performed via
    ordinary SQL update or delete commands on the referencing table.
    In particular, any triggers that exist on the referencing table
    will be fired for those changes.  If such a trigger modifies or
    blocks the effect of one of these commands, the end result could
    be to break referential integrity.  It is the trigger programmer's
    responsibility to avoid that.

            regards, tom lane

Hi,

> Perhaps we should leave the system triggers out of the discussion
> entirely?  More or less like:
>
>     If a foreign key constraint specifies referential actions (that
>     is, cascading updates or deletes), those actions are performed via
>     ordinary SQL update or delete commands on the referencing table.
>     In particular, any triggers that exist on the referencing table
>     will be fired for those changes.  If such a trigger modifies or
>     blocks the effect of one of these commands, the end result could
>     be to break referential integrity.  It is the trigger programmer's
>     responsibility to avoid that.

That's perfect!

-- 
Best regards,
Aleksander Alekseev

Aleksander Alekseev <aleksander@timescale.com> writes:
>> Perhaps we should leave the system triggers out of the discussion
>> entirely?  More or less like:
>> 
>> If a foreign key constraint specifies referential actions (that
>> is, cascading updates or deletes), those actions are performed via
>> ordinary SQL update or delete commands on the referencing table.
>> In particular, any triggers that exist on the referencing table
>> will be fired for those changes.  If such a trigger modifies or
>> blocks the effect of one of these commands, the end result could
>> be to break referential integrity.  It is the trigger programmer's
>> responsibility to avoid that.

> That's perfect!

Hearing no further comments, done like that.

            regards, tom lane