Repair problems occurring when multiple RI updates have to be done to the same
authorTom Lane <tgl@sss.pgh.pa.us>
Wed, 15 Aug 2007 19:15:47 +0000 (19:15 +0000)
committerTom Lane <tgl@sss.pgh.pa.us>
Wed, 15 Aug 2007 19:15:47 +0000 (19:15 +0000)
row within one query: we were firing check triggers before all the updates
were done, leading to bogus failures.  Fix by making the triggers queued by
an RI update go at the end of the outer query's trigger event list, thereby
effectively making the processing "breadth-first".  This was indeed how it
worked pre-8.0, so the bug does not occur in the 7.x branches.
Per report from Pavel Stehule.

src/backend/commands/trigger.c
src/backend/executor/spi.c
src/backend/utils/adt/ri_triggers.c
src/include/executor/spi.h
src/test/regress/expected/foreign_key.out
src/test/regress/sql/foreign_key.sql

index 80b28f5e4b40f1cc465bb3c91898504a9cc23ab9..2788684d1abd81a6cb80629a3d6910231570291a 100644 (file)
@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/commands/trigger.c,v 1.216 2007/07/17 17:45:28 tgl Exp $
+ *       $PostgreSQL: pgsql/src/backend/commands/trigger.c,v 1.217 2007/08/15 19:15:46 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -2332,6 +2332,7 @@ afterTriggerInvokeEvents(AfterTriggerEventList *events,
        AfterTriggerEvent event,
                                prev_event;
        MemoryContext per_tuple_context;
+       bool            locally_opened = false;
        Relation        rel = NULL;
        TriggerDesc *trigdesc = NULL;
        FmgrInfo   *finfo = NULL;
@@ -2364,6 +2365,19 @@ afterTriggerInvokeEvents(AfterTriggerEventList *events,
                         */
                        if (rel == NULL || rel->rd_id != event->ate_relid)
                        {
+                               if (locally_opened)
+                               {
+                                       /* close prior rel if any */
+                                       if (rel)
+                                               heap_close(rel, NoLock);
+                                       if (trigdesc)
+                                               FreeTriggerDesc(trigdesc);
+                                       if (finfo)
+                                               pfree(finfo);
+                                       Assert(instr == NULL);          /* never used in this case */
+                               }
+                               locally_opened = true;
+
                                if (estate)
                                {
                                        /* Find target relation among estate's result rels */
@@ -2375,28 +2389,22 @@ afterTriggerInvokeEvents(AfterTriggerEventList *events,
                                        while (nr > 0)
                                        {
                                                if (rInfo->ri_RelationDesc->rd_id == event->ate_relid)
+                                               {
+                                                       rel = rInfo->ri_RelationDesc;
+                                                       trigdesc = rInfo->ri_TrigDesc;
+                                                       finfo = rInfo->ri_TrigFunctions;
+                                                       instr = rInfo->ri_TrigInstrument;
+                                                       locally_opened = false;
                                                        break;
+                                               }
                                                rInfo++;
                                                nr--;
                                        }
-                                       if (nr <= 0)    /* should not happen */
-                                               elog(ERROR, "could not find relation %u among query result relations",
-                                                        event->ate_relid);
-                                       rel = rInfo->ri_RelationDesc;
-                                       trigdesc = rInfo->ri_TrigDesc;
-                                       finfo = rInfo->ri_TrigFunctions;
-                                       instr = rInfo->ri_TrigInstrument;
                                }
-                               else
+
+                               if (locally_opened)
                                {
-                                       /* Hard way: we manage the resources for ourselves */
-                                       if (rel)
-                                               heap_close(rel, NoLock);
-                                       if (trigdesc)
-                                               FreeTriggerDesc(trigdesc);
-                                       if (finfo)
-                                               pfree(finfo);
-                                       Assert(instr == NULL);          /* never used in this case */
+                                       /* Hard way: open target relation for ourselves */
 
                                        /*
                                         * We assume that an appropriate lock is still held by the
@@ -2421,6 +2429,7 @@ afterTriggerInvokeEvents(AfterTriggerEventList *events,
                                                palloc0(trigdesc->numtriggers * sizeof(FmgrInfo));
 
                                        /* Never any EXPLAIN info in this case */
+                                       instr = NULL;
                                }
                        }
 
@@ -2471,7 +2480,7 @@ afterTriggerInvokeEvents(AfterTriggerEventList *events,
        events->tail = prev_event;
 
        /* Release working resources */
-       if (!estate)
+       if (locally_opened)
        {
                if (rel)
                        heap_close(rel, NoLock);
@@ -2600,11 +2609,13 @@ AfterTriggerEndQuery(EState *estate)
         * IMMEDIATE: all events we have decided to defer will be available for it
         * to fire.
         *
+        * We loop in case a trigger queues more events.
+        *
         * If we find no firable events, we don't have to increment
         * firing_counter.
         */
        events = &afterTriggers->query_stack[afterTriggers->query_depth];
-       if (afterTriggerMarkEvents(events, &afterTriggers->events, true))
+       while (afterTriggerMarkEvents(events, &afterTriggers->events, true))
        {
                CommandId       firing_id = afterTriggers->firing_counter++;
 
@@ -2648,7 +2659,7 @@ AfterTriggerFireDeferred(void)
                ActiveSnapshot = CopySnapshot(GetTransactionSnapshot());
 
        /*
-        * Run all the remaining triggers.      Loop until they are all gone, just in
+        * Run all the remaining triggers.      Loop until they are all gone, in
         * case some trigger queues more for us to do.
         */
        while (afterTriggerMarkEvents(events, NULL, false))
@@ -3211,7 +3222,7 @@ AfterTriggerSetState(ConstraintsSetStmt *stmt)
        {
                AfterTriggerEventList *events = &afterTriggers->events;
 
-               if (afterTriggerMarkEvents(events, NULL, true))
+               while (afterTriggerMarkEvents(events, NULL, true))
                {
                        CommandId       firing_id = afterTriggers->firing_counter++;
 
index f97a86fe214662b2508ad0fe5bd8f93be29a9500..af94ad1a3b81ac39d62dbfcef0ff5765b611369f 100644 (file)
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/executor/spi.c,v 1.179 2007/04/27 22:05:47 tgl Exp $
+ *       $PostgreSQL: pgsql/src/backend/executor/spi.c,v 1.180 2007/08/15 19:15:46 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -39,9 +39,9 @@ static void _SPI_prepare_plan(const char *src, SPIPlanPtr plan);
 static int _SPI_execute_plan(SPIPlanPtr plan,
                                  Datum *Values, const char *Nulls,
                                  Snapshot snapshot, Snapshot crosscheck_snapshot,
-                                 bool read_only, long tcount);
+                                 bool read_only, bool fire_triggers, long tcount);
 
-static int     _SPI_pquery(QueryDesc *queryDesc, long tcount);
+static int     _SPI_pquery(QueryDesc *queryDesc, bool fire_triggers, long tcount);
 
 static void _SPI_error_callback(void *arg);
 
@@ -316,7 +316,7 @@ SPI_execute(const char *src, bool read_only, long tcount)
 
        res = _SPI_execute_plan(&plan, NULL, NULL,
                                                        InvalidSnapshot, InvalidSnapshot,
-                                                       read_only, tcount);
+                                                       read_only, true, tcount);
 
        _SPI_end_call(true);
        return res;
@@ -349,7 +349,7 @@ SPI_execute_plan(SPIPlanPtr plan, Datum *Values, const char *Nulls,
        res = _SPI_execute_plan(plan,
                                                        Values, Nulls,
                                                        InvalidSnapshot, InvalidSnapshot,
-                                                       read_only, tcount);
+                                                       read_only, true, tcount);
 
        _SPI_end_call(true);
        return res;
@@ -364,9 +364,12 @@ SPI_execp(SPIPlanPtr plan, Datum *Values, const char *Nulls, long tcount)
 
 /*
  * SPI_execute_snapshot -- identical to SPI_execute_plan, except that we allow
- * the caller to specify exactly which snapshots to use.  This is currently
- * not documented in spi.sgml because it is only intended for use by RI
- * triggers.
+ * the caller to specify exactly which snapshots to use.  Also, the caller
+ * may specify that AFTER triggers should be queued as part of the outer
+ * query rather than being fired immediately at the end of the command.
+ *
+ * This is currently not documented in spi.sgml because it is only intended
+ * for use by RI triggers.
  *
  * Passing snapshot == InvalidSnapshot will select the normal behavior of
  * fetching a new snapshot for each query.
@@ -375,7 +378,7 @@ int
 SPI_execute_snapshot(SPIPlanPtr plan,
                                         Datum *Values, const char *Nulls,
                                         Snapshot snapshot, Snapshot crosscheck_snapshot,
-                                        bool read_only, long tcount)
+                                        bool read_only, bool fire_triggers, long tcount)
 {
        int                     res;
 
@@ -392,7 +395,7 @@ SPI_execute_snapshot(SPIPlanPtr plan,
        res = _SPI_execute_plan(plan,
                                                        Values, Nulls,
                                                        snapshot, crosscheck_snapshot,
-                                                       read_only, tcount);
+                                                       read_only, fire_triggers, tcount);
 
        _SPI_end_call(true);
        return res;
@@ -1428,12 +1431,14 @@ _SPI_prepare_plan(const char *src, SPIPlanPtr plan)
  *             behavior of taking a new snapshot for each query.
  * crosscheck_snapshot: for RI use, all others pass InvalidSnapshot
  * read_only: TRUE for read-only execution (no CommandCounterIncrement)
+ * fire_triggers: TRUE to fire AFTER triggers at end of query (normal case);
+ *             FALSE means any AFTER triggers are postponed to end of outer query
  * tcount: execution tuple-count limit, or 0 for none
  */
 static int
 _SPI_execute_plan(SPIPlanPtr plan, Datum *Values, const char *Nulls,
                                  Snapshot snapshot, Snapshot crosscheck_snapshot,
-                                 bool read_only, long tcount)
+                                 bool read_only, bool fire_triggers, long tcount)
 {
        volatile int my_res = 0;
        volatile uint32 my_processed = 0;
@@ -1589,7 +1594,8 @@ _SPI_execute_plan(SPIPlanPtr plan, Datum *Values, const char *Nulls,
                                                                                        crosscheck_snapshot,
                                                                                        dest,
                                                                                        paramLI, false);
-                                       res = _SPI_pquery(qdesc, canSetTag ? tcount : 0);
+                                       res = _SPI_pquery(qdesc, fire_triggers,
+                                                                         canSetTag ? tcount : 0);
                                        FreeQueryDesc(qdesc);
                                }
                                else
@@ -1680,7 +1686,7 @@ fail:
 }
 
 static int
-_SPI_pquery(QueryDesc *queryDesc, long tcount)
+_SPI_pquery(QueryDesc *queryDesc, bool fire_triggers, long tcount)
 {
        int                     operation = queryDesc->operation;
        int                     res;
@@ -1726,7 +1732,8 @@ _SPI_pquery(QueryDesc *queryDesc, long tcount)
                ResetUsage();
 #endif
 
-       AfterTriggerBeginQuery();
+       if (fire_triggers)
+               AfterTriggerBeginQuery();
 
        ExecutorStart(queryDesc, 0);
 
@@ -1743,7 +1750,8 @@ _SPI_pquery(QueryDesc *queryDesc, long tcount)
        }
 
        /* Take care of any queued AFTER triggers */
-       AfterTriggerEndQuery(queryDesc->estate);
+       if (fire_triggers)
+               AfterTriggerEndQuery(queryDesc->estate);
 
        ExecutorEnd(queryDesc);
 
index 3f257f375f431f1e109acaf13fc220edc2427453..9add8f934d853b741d7d877c3ec65f471fde2532 100644 (file)
@@ -15,7 +15,7 @@
  *
  * Portions Copyright (c) 1996-2007, PostgreSQL Global Development Group
  *
- * $PostgreSQL: pgsql/src/backend/utils/adt/ri_triggers.c,v 1.95 2007/06/05 21:31:06 tgl Exp $
+ * $PostgreSQL: pgsql/src/backend/utils/adt/ri_triggers.c,v 1.96 2007/08/15 19:15:46 tgl Exp $
  *
  * ----------
  */
@@ -2774,7 +2774,7 @@ RI_Initial_Check(Trigger *trigger, Relation fk_rel, Relation pk_rel)
                                                                          NULL, NULL,
                                                                          CopySnapshot(GetLatestSnapshot()),
                                                                          InvalidSnapshot,
-                                                                         true, 1);
+                                                                         true, false, 1);
 
        /* Check result */
        if (spi_result != SPI_OK_SELECT)
@@ -3308,7 +3308,7 @@ ri_PerformCheck(RI_QueryKey *qkey, SPIPlanPtr qplan,
        spi_result = SPI_execute_snapshot(qplan,
                                                                          vals, nulls,
                                                                          test_snapshot, crosscheck_snapshot,
-                                                                         false, limit);
+                                                                         false, false, limit);
 
        /* Restore UID */
        SetUserId(save_uid);
index 20beefe4401240a23add99dfcd0d3dfe4c8bea5b..3ef50993c6245c1eb0146568fee3d6e73d7965a3 100644 (file)
@@ -6,7 +6,7 @@
  * Portions Copyright (c) 1996-2007, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/executor/spi.h,v 1.62 2007/07/25 12:22:53 mha Exp $
+ * $PostgreSQL: pgsql/src/include/executor/spi.h,v 1.63 2007/08/15 19:15:46 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -104,7 +104,7 @@ extern int  SPI_execute_snapshot(SPIPlanPtr plan,
                                         Datum *Values, const char *Nulls,
                                         Snapshot snapshot,
                                         Snapshot crosscheck_snapshot,
-                                        bool read_only, long tcount);
+                                        bool read_only, bool fire_triggers, long tcount);
 extern SPIPlanPtr SPI_prepare(const char *src, int nargs, Oid *argtypes);
 extern SPIPlanPtr SPI_prepare_cursor(const char *src, int nargs, Oid *argtypes,
                                                                         int cursorOptions);
index 56411f06a35fa7522df813198665278fd98c9dbf..e24ceb51a87f480507abb2328cb983c5727107f2 100644 (file)
@@ -646,7 +646,6 @@ SELECT * from FKTABLE;
 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;
@@ -1230,3 +1229,71 @@ ROLLBACK TO savept1;
 COMMIT;
 ERROR:  insert or update on table "fktable" violates foreign key constraint "fktable_fk_fkey"
 DETAIL:  Key (fk)=(20) is not present in table "pktable".
+-- test order of firing of FK triggers when several RI-induced changes need to
+-- be made to the same row.  This was broken by subtransaction-related
+-- changes in 8.0.
+CREATE TEMP TABLE users (
+  id INT PRIMARY KEY,
+  name VARCHAR NOT NULL
+);
+NOTICE:  CREATE TABLE / PRIMARY KEY will create implicit index "users_pkey" for table "users"
+INSERT INTO users VALUES (1, 'Jozko');
+INSERT INTO users VALUES (2, 'Ferko');
+INSERT INTO users VALUES (3, 'Samko');
+CREATE TEMP TABLE tasks (
+  id INT PRIMARY KEY,
+  owner INT REFERENCES users ON UPDATE CASCADE ON DELETE SET NULL,
+  worker INT REFERENCES users ON UPDATE CASCADE ON DELETE SET NULL,
+  checked_by INT REFERENCES users ON UPDATE CASCADE ON DELETE SET NULL
+);
+NOTICE:  CREATE TABLE / PRIMARY KEY will create implicit index "tasks_pkey" for table "tasks"
+INSERT INTO tasks VALUES (1,1,NULL,NULL);
+INSERT INTO tasks VALUES (2,2,2,NULL);
+INSERT INTO tasks VALUES (3,3,3,3);
+SELECT * FROM tasks;
+ id | owner | worker | checked_by 
+----+-------+--------+------------
+  1 |     1 |        |           
+  2 |     2 |      2 |           
+  3 |     3 |      3 |          3
+(3 rows)
+
+UPDATE users SET id = 4 WHERE id = 3;
+SELECT * FROM tasks;
+ id | owner | worker | checked_by 
+----+-------+--------+------------
+  1 |     1 |        |           
+  2 |     2 |      2 |           
+  3 |     4 |      4 |          4
+(3 rows)
+
+DELETE FROM users WHERE id = 4;
+SELECT * FROM tasks;
+ id | owner | worker | checked_by 
+----+-------+--------+------------
+  1 |     1 |        |           
+  2 |     2 |      2 |           
+  3 |       |        |           
+(3 rows)
+
+-- could fail with only 2 changes to make, if row was already updated
+BEGIN;
+UPDATE tasks set id=id WHERE id=2;
+SELECT * FROM tasks;
+ id | owner | worker | checked_by 
+----+-------+--------+------------
+  1 |     1 |        |           
+  3 |       |        |           
+  2 |     2 |      2 |           
+(3 rows)
+
+DELETE FROM users WHERE id = 2;
+SELECT * FROM tasks;
+ id | owner | worker | checked_by 
+----+-------+--------+------------
+  1 |     1 |        |           
+  3 |       |        |           
+  2 |       |        |           
+(3 rows)
+
+COMMIT;
index 1ab557b9bf30d31fa5ce48dffd9945e2dd34f377..519a5056af04906e37eec9ea69c6faeff9241452 100644 (file)
@@ -879,3 +879,45 @@ ROLLBACK TO savept1;
 
 -- should catch error from initial INSERT
 COMMIT;
+
+-- test order of firing of FK triggers when several RI-induced changes need to
+-- be made to the same row.  This was broken by subtransaction-related
+-- changes in 8.0.
+
+CREATE TEMP TABLE users (
+  id INT PRIMARY KEY,
+  name VARCHAR NOT NULL
+);
+
+INSERT INTO users VALUES (1, 'Jozko');
+INSERT INTO users VALUES (2, 'Ferko');
+INSERT INTO users VALUES (3, 'Samko');
+
+CREATE TEMP TABLE tasks (
+  id INT PRIMARY KEY,
+  owner INT REFERENCES users ON UPDATE CASCADE ON DELETE SET NULL,
+  worker INT REFERENCES users ON UPDATE CASCADE ON DELETE SET NULL,
+  checked_by INT REFERENCES users ON UPDATE CASCADE ON DELETE SET NULL
+);
+
+INSERT INTO tasks VALUES (1,1,NULL,NULL);
+INSERT INTO tasks VALUES (2,2,2,NULL);
+INSERT INTO tasks VALUES (3,3,3,3);
+
+SELECT * FROM tasks;
+
+UPDATE users SET id = 4 WHERE id = 3;
+
+SELECT * FROM tasks;
+
+DELETE FROM users WHERE id = 4;
+
+SELECT * FROM tasks;
+
+-- could fail with only 2 changes to make, if row was already updated
+BEGIN;
+UPDATE tasks set id=id WHERE id=2;
+SELECT * FROM tasks;
+DELETE FROM users WHERE id = 2;
+SELECT * FROM tasks;
+COMMIT;