ON DELETE SET NULL clauses do error when more than two columns are referenced to one table

Hello

One question on www.abclinuxu.cz signalise bug in PostgreSQL RI
implementation. Detected on 8.0.x and verified on 8.3.

Regards
Pavel Stehule

CREATE TABLE users (
id integer NOT NULL,
name VARCHAR NOT NULL,
PRIMARY KEY (id)
);

INSERT INTO users VALUES (1, 'Jozko');
INSERT INTO users VALUES (2, 'Ferko');
INSERT INTO users VALUES (3, 'Samko');

CREATE TABLE tasks (
id integer NOT NULL,
owner INT REFERENCES users (id) ON UPDATE CASCADE ON DELETE SET NULL,
worker INT REFERENCES users (id) ON UPDATE CASCADE ON DELETE SET NULL,
checked_by INT REFERENCES users (id) ON UPDATE CASCADE ON DELETE SET NULL,
PRIMARY KEY (id)
);
INSERT INTO tasks VALUES (1,1,NULL,NULL);
INSERT INTO tasks VALUES (2,2,2,NULL);
INSERT INTO tasks VALUES (3,3,3,3);

DELETE FROM users WHERE id = 1; -- works simple
DELETE FROM users WHERE id = 2; -- works ok
DELETE FROM users WHERE id = 3; -- doesn't work, why?

ERROR: insert or update on table "tasks" violates foreign key
constraint "tasks_checked_by_fkey"
DETAIL: Key (checked_by)=(3) is not present in table "users".
CONTEXT: SQL statement "UPDATE ONLY "public"."tasks" SET "worker" =
NULL WHERE $1 OPERATOR(pg_catalog.=) "worker""

Pavel Stehule wrote:
> One question on www.abclinuxu.cz signalise bug in PostgreSQL RI
> implementation. Detected on 8.0.x and verified on 8.3.

What seems to happen under the hood is:

1. The row in users is deleted
2. The setnull trigger on owner is fired, which executes "UPDATE ONLY
users SET owner = NULL WHERE owner = 3"
3. That update fires the update-triggers on tasks-table, to check that
the referenced row exists in users. Because owner is now null, it's not
checked. Worker and checked_by are not checked, because
AfterTriggerSaveEvent sees that those columns were not modified.
4. The setnull trigger on worker is fired, which executes "UPDATE ONLY
users SET worker = NULL where worker = 3".
5. That update again fires the update-triggers on tasks, to check that
the referenced row exists in users. Owner and worker are now null, so
they're not checked. However, checked_by is not null, so a the trigger
to check that the referenced row in the users table exists ("SELECT 1
FROM users WHERE id=3 FOR SHARE"), which fails.

The ON UPDATE trigger is not fired in step 2, because of the
optimization in AfterTriggerSaveEvent to skip UPDATE triggers if the FK
column was not changed. However, we don't do the optimization if the old
tuple was created in the same transaction:

case RI_TRIGGER_FK:
/*
* Update on FK table
*
* There is one exception when updating FK tables: if the
* updated row was inserted by our own transaction and the
* FK is deferred, we still need to fire the trigger. This
* is because our UPDATE will invalidate the INSERT so the
* end-of-transaction INSERT RI trigger will not do
* anything, so we have to do the check for the UPDATE
* anyway.
*/
if (HeapTupleHeaderGetXmin(oldtup->t_data) !=
GetCurrentTransactionId() &&
RI_FKey_keyequal_upd_fk(trigger, rel, oldtup, newtup))
{
continue;
}
break;

which is why we do fire the trigger in step 5.

ISTM the real problem is that the RI triggers are fired in wrong order.
The above optimization saves the day in simple scenarios, but it seems
to be an unintended side-effect.

In fact, you can trigger the problem with a child table with just two ON
DELETE SET NULL columns, if you do a dummy update of the child row
before the delete of the parent, in the same transaction:

CREATE TABLE users (
id integer NOT NULL,
name VARCHAR NOT NULL,
PRIMARY KEY (id)
);

INSERT INTO users VALUES (3, 'Samko');

CREATE TABLE tasks (
id integer NOT NULL,
a INT REFERENCES users (id) ON DELETE SET NULL,
b INT REFERENCES users (id) ON DELETE SET NULL
);
INSERT INTO tasks VALUES (3,3,3);

BEGIN;
UPDATE tasks set id=id WHERE id=3;
DELETE FROM users WHERE id = 3; -- doesn't work, why?
COMMIT;

I'm not sure what to do about this. We could change the order the
triggers are fired to breadth-first. If all the setnull triggers were
executed first, there would be no problem. But that seems like a pretty
big change, and I'm afraid it might have other unintended consequences.

Ideas?

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

"Heikki Linnakangas" <heikki(at)enterprisedb(dot)com> writes:

> I'm not sure what to do about this. We could change the order the
> triggers are fired to breadth-first. If all the setnull triggers were
> executed first, there would be no problem. But that seems like a pretty
> big change, and I'm afraid it might have other unintended consequences.

We could also check the queued triggers to see if there really is a trigger
which will be invalidated by the second update and therefore needs to be
rechecked (or even just change the tid of the existing queued check). I don't
know what it would take to make that efficient though.

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com

Heikki Linnakangas <heikki(at)enterprisedb(dot)com> writes:
> I'm not sure what to do about this. We could change the order the
> triggers are fired to breadth-first. If all the setnull triggers were
> executed first, there would be no problem. But that seems like a pretty
> big change, and I'm afraid it might have other unintended consequences.

I think it's not so much that they should be "breadth first" as that the
updates generated by the triggers shouldn't count as their own
sub-statements. The trigger events generated by those updates need to
go at the end of the outer statement's trigger queue. We'll need to
change the API of SPI_execute_snapshot for this, but since that's only
for the use of the RI triggers anyway, it doesn't seem like a problem.

I also notice that only one of the
afterTriggerMarkEvents/afterTriggerInvokeEvents pairs in trigger.c
is coded as a "while" ... they probably all must be if we expect that RI
triggers will generate events at the same trigger level.

regards, tom lane

Tom Lane wrote:
> Heikki Linnakangas <heikki(at)enterprisedb(dot)com> writes:
>> I'm not sure what to do about this. We could change the order the
>> triggers are fired to breadth-first. If all the setnull triggers were
>> executed first, there would be no problem. But that seems like a pretty
>> big change, and I'm afraid it might have other unintended consequences.
>
> I think it's not so much that they should be "breadth first" as that the
> updates generated by the triggers shouldn't count as their own
> sub-statements. The trigger events generated by those updates need to
> go at the end of the outer statement's trigger queue. We'll need to
> change the API of SPI_execute_snapshot for this, but since that's only
> for the use of the RI triggers anyway, it doesn't seem like a problem.
>
> I also notice that only one of the
> afterTriggerMarkEvents/afterTriggerInvokeEvents pairs in trigger.c
> is coded as a "while" ... they probably all must be if we expect that RI
> triggers will generate events at the same trigger level.

I can take a stab at writing a patch.

Does this need to be backported? It's a bug, but I feel uncomfortable
backporting. I'm afraid it'll break some other corner cases, and it
doesn't seem to be a huge problem in practice since no-one's ran into it
until now.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

I wrote:
> Heikki Linnakangas <heikki(at)enterprisedb(dot)com> writes:
>> I'm not sure what to do about this. We could change the order the
>> triggers are fired to breadth-first. If all the setnull triggers were
>> executed first, there would be no problem. But that seems like a pretty
>> big change, and I'm afraid it might have other unintended consequences.

> I think it's not so much that they should be "breadth first" as that the
> updates generated by the triggers shouldn't count as their own
> sub-statements. The trigger events generated by those updates need to
> go at the end of the outer statement's trigger queue.

Actually, Heikki's description is isomorphic to mine ...

I coded this up (patch attached, sans regression test additions) and
it seems to work: Pavel's and Heikki's test cases do what is expected.
The regression tests pass modulo this diff:

*** ./expected/foreign_key.out Tue Jul 17 13:45:28 2007
--- ./results/foreign_key.out Tue Aug 14 14:39:38 2007
***************
*** 646,652 ****
UPDATE PKTABLE set ptest2=5 where ptest2=2;
ERROR: insert or update on table "fktable" violates foreign key constraint "constrname3"
DETAIL: Key (ftest1,ftest2,ftest3)=(1,-1,3) is not present in table "pktable".
- CONTEXT: SQL statement "UPDATE ONLY "public"."fktable" SET "ftest2" = DEFAULT WHERE $1 OPERATOR(pg_catalog.=) "ftest1" AND $2 OPERATOR(pg_catalog.=) "ftest2" AND $3 OPERATOR(pg_catalog.=) "ftest3""
-- Try to update something that will set default
UPDATE PKTABLE set ptest1=0, ptest2=5, ptest3=10 where ptest2=2;
UPDATE PKTABLE set ptest2=10 where ptest2=4;
--- 646,651 ----

which is happening because the error is no longer detected while we are
inside the first RI trigger's UPDATE command, but only while handling
triggers queued for the outer query. This is a little bit annoying IMHO
because the connection of the error message to the original query isn't
very obvious, and seeing the RI update query might help make it a bit
clearer. OTOH the RI update query is ugly enough that I'm not sure the
average user would be helped by the CONTEXT line. In any case it seems
difficult to avoid this change.

Another problem is that, because the indirectly-fired RI triggers are
ultimately run in a context where their target table is not the direct
target of the current query, trigger.c doesn't have any rangetable entry
to connect them to (this is why the patch has to touch trigger.c, as it
formerly considered that an error case). This has a performance cost
(extra heap_opens) that is probably usually negligible, but maybe not
always. Also, as the patch stands EXPLAIN ANALYZE would fail to report
the runtime of such triggers separately --- they'd be included in the
bottom-line total runtime but not listed by themselves. (In current
code they aren't listed separately either ... but they'd be charged to
the parent trigger that caused them to be fired, which is better than
nothing.)

I am tempted to try to fix these last two problems by having
afterTriggerInvokeEvents() dynamically add tables to the executor's
range table when it's told to fire triggers for tables that aren't
already listed there. That's getting a bit hairy for a back-patchable
fix though. I'm thinking of trying that in HEAD but applying this
patch as-is to 8.0 through 8.2. (Note that 7.x doesn't exhibit the
bug, mainly because it never tried to fire any triggers earlier than
end-of-interactive-command.)

Comments?

regards, tom lane

Heikki Linnakangas <heikki(at)enterprisedb(dot)com> writes:
> I can take a stab at writing a patch.

I beat you to it, barely.

> Does this need to be backported?

I think it does, seeing that it's a regression from 7.4.

regards, tom lane