Skip to content

Commit 9cb8409

Browse files
tglsfdctglsfdc
committed
Repair problems occurring when multiple RI updates have to be done to the same
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.
1 parent 5ff95e6 commit 9cb8409

File tree

6 files changed

+170
-42
lines changed

6 files changed

+170
-42
lines changed

‎src/backend/commands/trigger.c

Lines changed: 32 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
* Portions Copyright (c) 1994, Regents of the University of California
88
*
99
* IDENTIFICATION
10-
* $PostgreSQL: pgsql/src/backend/commands/trigger.c,v 1.216 2007/07/17 17:45:28 tgl Exp $
10+
* $PostgreSQL: pgsql/src/backend/commands/trigger.c,v 1.217 2007/08/15 19:15:46 tgl Exp $
1111
*
1212
*-------------------------------------------------------------------------
1313
*/
@@ -2332,6 +2332,7 @@ afterTriggerInvokeEvents(AfterTriggerEventList *events,
23322332
AfterTriggerEvent event,
23332333
prev_event;
23342334
MemoryContext per_tuple_context;
2335+
bool locally_opened = false;
23352336
Relation rel = NULL;
23362337
TriggerDesc *trigdesc = NULL;
23372338
FmgrInfo *finfo = NULL;
@@ -2364,6 +2365,19 @@ afterTriggerInvokeEvents(AfterTriggerEventList *events,
23642365
*/
23652366
if (rel == NULL || rel->rd_id != event->ate_relid)
23662367
{
2368+
if (locally_opened)
2369+
{
2370+
/* close prior rel if any */
2371+
if (rel)
2372+
heap_close(rel, NoLock);
2373+
if (trigdesc)
2374+
FreeTriggerDesc(trigdesc);
2375+
if (finfo)
2376+
pfree(finfo);
2377+
Assert(instr == NULL); /* never used in this case */
2378+
}
2379+
locally_opened = true;
2380+
23672381
if (estate)
23682382
{
23692383
/* Find target relation among estate's result rels */
@@ -2375,28 +2389,22 @@ afterTriggerInvokeEvents(AfterTriggerEventList *events,
23752389
while (nr > 0)
23762390
{
23772391
if (rInfo->ri_RelationDesc->rd_id == event->ate_relid)
2392+
{
2393+
rel = rInfo->ri_RelationDesc;
2394+
trigdesc = rInfo->ri_TrigDesc;
2395+
finfo = rInfo->ri_TrigFunctions;
2396+
instr = rInfo->ri_TrigInstrument;
2397+
locally_opened = false;
23782398
break;
2399+
}
23792400
rInfo++;
23802401
nr--;
23812402
}
2382-
if (nr <= 0) /* should not happen */
2383-
elog(ERROR, "could not find relation %u among query result relations",
2384-
event->ate_relid);
2385-
rel = rInfo->ri_RelationDesc;
2386-
trigdesc = rInfo->ri_TrigDesc;
2387-
finfo = rInfo->ri_TrigFunctions;
2388-
instr = rInfo->ri_TrigInstrument;
23892403
}
2390-
else
2404+
2405+
if (locally_opened)
23912406
{
2392-
/* Hard way: we manage the resources for ourselves */
2393-
if (rel)
2394-
heap_close(rel, NoLock);
2395-
if (trigdesc)
2396-
FreeTriggerDesc(trigdesc);
2397-
if (finfo)
2398-
pfree(finfo);
2399-
Assert(instr == NULL); /* never used in this case */
2407+
/* Hard way: open target relation for ourselves */
24002408

24012409
/*
24022410
* We assume that an appropriate lock is still held by the
@@ -2421,6 +2429,7 @@ afterTriggerInvokeEvents(AfterTriggerEventList *events,
24212429
palloc0(trigdesc->numtriggers * sizeof(FmgrInfo));
24222430

24232431
/* Never any EXPLAIN info in this case */
2432+
instr = NULL;
24242433
}
24252434
}
24262435

@@ -2471,7 +2480,7 @@ afterTriggerInvokeEvents(AfterTriggerEventList *events,
24712480
events->tail = prev_event;
24722481

24732482
/* Release working resources */
2474-
if (!estate)
2483+
if (locally_opened)
24752484
{
24762485
if (rel)
24772486
heap_close(rel, NoLock);
@@ -2600,11 +2609,13 @@ AfterTriggerEndQuery(EState *estate)
26002609
* IMMEDIATE: all events we have decided to defer will be available for it
26012610
* to fire.
26022611
*
2612+
* We loop in case a trigger queues more events.
2613+
*
26032614
* If we find no firable events, we don't have to increment
26042615
* firing_counter.
26052616
*/
26062617
events = &afterTriggers->query_stack[afterTriggers->query_depth];
2607-
if (afterTriggerMarkEvents(events, &afterTriggers->events, true))
2618+
while (afterTriggerMarkEvents(events, &afterTriggers->events, true))
26082619
{
26092620
CommandId firing_id = afterTriggers->firing_counter++;
26102621

@@ -2648,7 +2659,7 @@ AfterTriggerFireDeferred(void)
26482659
ActiveSnapshot = CopySnapshot(GetTransactionSnapshot());
26492660

26502661
/*
2651-
* Run all the remaining triggers. Loop until they are all gone, just in
2662+
* Run all the remaining triggers. Loop until they are all gone, in
26522663
* case some trigger queues more for us to do.
26532664
*/
26542665
while (afterTriggerMarkEvents(events, NULL, false))
@@ -3211,7 +3222,7 @@ AfterTriggerSetState(ConstraintsSetStmt *stmt)
32113222
{
32123223
AfterTriggerEventList *events = &afterTriggers->events;
32133224

3214-
if (afterTriggerMarkEvents(events, NULL, true))
3225+
while (afterTriggerMarkEvents(events, NULL, true))
32153226
{
32163227
CommandId firing_id = afterTriggers->firing_counter++;
32173228

‎src/backend/executor/spi.c

Lines changed: 23 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $PostgreSQL: pgsql/src/backend/executor/spi.c,v 1.179 2007/04/27 22:05:47 tgl Exp $
11+
* $PostgreSQL: pgsql/src/backend/executor/spi.c,v 1.180 2007/08/15 19:15:46 tgl Exp $
1212
*
1313
*-------------------------------------------------------------------------
1414
*/
@@ -39,9 +39,9 @@ static void _SPI_prepare_plan(const char *src, SPIPlanPtr plan);
3939
static int _SPI_execute_plan(SPIPlanPtr plan,
4040
Datum *Values, const char *Nulls,
4141
Snapshot snapshot, Snapshot crosscheck_snapshot,
42-
bool read_only, long tcount);
42+
bool read_only, bool fire_triggers, long tcount);
4343

44-
static int _SPI_pquery(QueryDesc *queryDesc, long tcount);
44+
static int _SPI_pquery(QueryDesc *queryDesc, bool fire_triggers, long tcount);
4545

4646
static void _SPI_error_callback(void *arg);
4747

@@ -316,7 +316,7 @@ SPI_execute(const char *src, bool read_only, long tcount)
316316

317317
res = _SPI_execute_plan(&plan, NULL, NULL,
318318
InvalidSnapshot, InvalidSnapshot,
319-
read_only, tcount);
319+
read_only, true, tcount);
320320

321321
_SPI_end_call(true);
322322
return res;
@@ -349,7 +349,7 @@ SPI_execute_plan(SPIPlanPtr plan, Datum *Values, const char *Nulls,
349349
res = _SPI_execute_plan(plan,
350350
Values, Nulls,
351351
InvalidSnapshot, InvalidSnapshot,
352-
read_only, tcount);
352+
read_only, true, tcount);
353353

354354
_SPI_end_call(true);
355355
return res;
@@ -364,9 +364,12 @@ SPI_execp(SPIPlanPtr plan, Datum *Values, const char *Nulls, long tcount)
364364

365365
/*
366366
* SPI_execute_snapshot -- identical to SPI_execute_plan, except that we allow
367-
* the caller to specify exactly which snapshots to use. This is currently
368-
* not documented in spi.sgml because it is only intended for use by RI
369-
* triggers.
367+
* the caller to specify exactly which snapshots to use. Also, the caller
368+
* may specify that AFTER triggers should be queued as part of the outer
369+
* query rather than being fired immediately at the end of the command.
370+
*
371+
* This is currently not documented in spi.sgml because it is only intended
372+
* for use by RI triggers.
370373
*
371374
* Passing snapshot == InvalidSnapshot will select the normal behavior of
372375
* fetching a new snapshot for each query.
@@ -375,7 +378,7 @@ int
375378
SPI_execute_snapshot(SPIPlanPtr plan,
376379
Datum *Values, const char *Nulls,
377380
Snapshot snapshot, Snapshot crosscheck_snapshot,
378-
bool read_only, long tcount)
381+
bool read_only, bool fire_triggers, long tcount)
379382
{
380383
int res;
381384

@@ -392,7 +395,7 @@ SPI_execute_snapshot(SPIPlanPtr plan,
392395
res = _SPI_execute_plan(plan,
393396
Values, Nulls,
394397
snapshot, crosscheck_snapshot,
395-
read_only, tcount);
398+
read_only, fire_triggers, tcount);
396399

397400
_SPI_end_call(true);
398401
return res;
@@ -1428,12 +1431,14 @@ _SPI_prepare_plan(const char *src, SPIPlanPtr plan)
14281431
* behavior of taking a new snapshot for each query.
14291432
* crosscheck_snapshot: for RI use, all others pass InvalidSnapshot
14301433
* read_only: TRUE for read-only execution (no CommandCounterIncrement)
1434+
* fire_triggers: TRUE to fire AFTER triggers at end of query (normal case);
1435+
* FALSE means any AFTER triggers are postponed to end of outer query
14311436
* tcount: execution tuple-count limit, or 0 for none
14321437
*/
14331438
static int
14341439
_SPI_execute_plan(SPIPlanPtr plan, Datum *Values, const char *Nulls,
14351440
Snapshot snapshot, Snapshot crosscheck_snapshot,
1436-
bool read_only, long tcount)
1441+
bool read_only, bool fire_triggers, long tcount)
14371442
{
14381443
volatile int my_res = 0;
14391444
volatile uint32 my_processed = 0;
@@ -1589,7 +1594,8 @@ _SPI_execute_plan(SPIPlanPtr plan, Datum *Values, const char *Nulls,
15891594
crosscheck_snapshot,
15901595
dest,
15911596
paramLI, false);
1592-
res = _SPI_pquery(qdesc, canSetTag ? tcount : 0);
1597+
res = _SPI_pquery(qdesc, fire_triggers,
1598+
canSetTag ? tcount : 0);
15931599
FreeQueryDesc(qdesc);
15941600
}
15951601
else
@@ -1680,7 +1686,7 @@ _SPI_execute_plan(SPIPlanPtr plan, Datum *Values, const char *Nulls,
16801686
}
16811687

16821688
static int
1683-
_SPI_pquery(QueryDesc *queryDesc, long tcount)
1689+
_SPI_pquery(QueryDesc *queryDesc, bool fire_triggers, long tcount)
16841690
{
16851691
int operation = queryDesc->operation;
16861692
int res;
@@ -1726,7 +1732,8 @@ _SPI_pquery(QueryDesc *queryDesc, long tcount)
17261732
ResetUsage();
17271733
#endif
17281734

1729-
AfterTriggerBeginQuery();
1735+
if (fire_triggers)
1736+
AfterTriggerBeginQuery();
17301737

17311738
ExecutorStart(queryDesc, 0);
17321739

@@ -1743,7 +1750,8 @@ _SPI_pquery(QueryDesc *queryDesc, long tcount)
17431750
}
17441751

17451752
/* Take care of any queued AFTER triggers */
1746-
AfterTriggerEndQuery(queryDesc->estate);
1753+
if (fire_triggers)
1754+
AfterTriggerEndQuery(queryDesc->estate);
17471755

17481756
ExecutorEnd(queryDesc);
17491757

‎src/backend/utils/adt/ri_triggers.c

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515
*
1616
* Portions Copyright (c) 1996-2007, PostgreSQL Global Development Group
1717
*
18-
* $PostgreSQL: pgsql/src/backend/utils/adt/ri_triggers.c,v 1.95 2007/06/05 21:31:06 tgl Exp $
18+
* $PostgreSQL: pgsql/src/backend/utils/adt/ri_triggers.c,v 1.96 2007/08/15 19:15:46 tgl Exp $
1919
*
2020
* ----------
2121
*/
@@ -2774,7 +2774,7 @@ RI_Initial_Check(Trigger *trigger, Relation fk_rel, Relation pk_rel)
27742774
NULL, NULL,
27752775
CopySnapshot(GetLatestSnapshot()),
27762776
InvalidSnapshot,
2777-
true, 1);
2777+
true, false, 1);
27782778

27792779
/* Check result */
27802780
if (spi_result != SPI_OK_SELECT)
@@ -3308,7 +3308,7 @@ ri_PerformCheck(RI_QueryKey *qkey, SPIPlanPtr qplan,
33083308
spi_result = SPI_execute_snapshot(qplan,
33093309
vals, nulls,
33103310
test_snapshot, crosscheck_snapshot,
3311-
false, limit);
3311+
false, false, limit);
33123312

33133313
/* Restore UID */
33143314
SetUserId(save_uid);

‎src/include/executor/spi.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
* Portions Copyright (c) 1996-2007, PostgreSQL Global Development Group
77
* Portions Copyright (c) 1994, Regents of the University of California
88
*
9-
* $PostgreSQL: pgsql/src/include/executor/spi.h,v 1.62 2007/07/25 12:22:53 mha Exp $
9+
* $PostgreSQL: pgsql/src/include/executor/spi.h,v 1.63 2007/08/15 19:15:46 tgl Exp $
1010
*
1111
*-------------------------------------------------------------------------
1212
*/
@@ -104,7 +104,7 @@ extern int SPI_execute_snapshot(SPIPlanPtr plan,
104104
Datum *Values, const char *Nulls,
105105
Snapshot snapshot,
106106
Snapshot crosscheck_snapshot,
107-
bool read_only, long tcount);
107+
bool read_only, bool fire_triggers, long tcount);
108108
extern SPIPlanPtr SPI_prepare(const char *src, int nargs, Oid *argtypes);
109109
extern SPIPlanPtr SPI_prepare_cursor(const char *src, int nargs, Oid *argtypes,
110110
int cursorOptions);

‎src/test/regress/expected/foreign_key.out

Lines changed: 68 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -646,7 +646,6 @@ SELECT * from FKTABLE;
646646
UPDATE PKTABLE set ptest2=5 where ptest2=2;
647647
ERROR: insert or update on table "fktable" violates foreign key constraint "constrname3"
648648
DETAIL: Key (ftest1,ftest2,ftest3)=(1,-1,3) is not present in table "pktable".
649-
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""
650649
-- Try to update something that will set default
651650
UPDATE PKTABLE set ptest1=0, ptest2=5, ptest3=10 where ptest2=2;
652651
UPDATE PKTABLE set ptest2=10 where ptest2=4;
@@ -1230,3 +1229,71 @@ ROLLBACK TO savept1;
12301229
COMMIT;
12311230
ERROR: insert or update on table "fktable" violates foreign key constraint "fktable_fk_fkey"
12321231
DETAIL: Key (fk)=(20) is not present in table "pktable".
1232+
-- test order of firing of FK triggers when several RI-induced changes need to
1233+
-- be made to the same row. This was broken by subtransaction-related
1234+
-- changes in 8.0.
1235+
CREATE TEMP TABLE users (
1236+
id INT PRIMARY KEY,
1237+
name VARCHAR NOT NULL
1238+
);
1239+
NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "users_pkey" for table "users"
1240+
INSERT INTO users VALUES (1, 'Jozko');
1241+
INSERT INTO users VALUES (2, 'Ferko');
1242+
INSERT INTO users VALUES (3, 'Samko');
1243+
CREATE TEMP TABLE tasks (
1244+
id INT PRIMARY KEY,
1245+
owner INT REFERENCES users ON UPDATE CASCADE ON DELETE SET NULL,
1246+
worker INT REFERENCES users ON UPDATE CASCADE ON DELETE SET NULL,
1247+
checked_by INT REFERENCES users ON UPDATE CASCADE ON DELETE SET NULL
1248+
);
1249+
NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "tasks_pkey" for table "tasks"
1250+
INSERT INTO tasks VALUES (1,1,NULL,NULL);
1251+
INSERT INTO tasks VALUES (2,2,2,NULL);
1252+
INSERT INTO tasks VALUES (3,3,3,3);
1253+
SELECT * FROM tasks;
1254+
id | owner | worker | checked_by
1255+
----+-------+--------+------------
1256+
1 | 1 | |
1257+
2 | 2 | 2 |
1258+
3 | 3 | 3 | 3
1259+
(3 rows)
1260+
1261+
UPDATE users SET id = 4 WHERE id = 3;
1262+
SELECT * FROM tasks;
1263+
id | owner | worker | checked_by
1264+
----+-------+--------+------------
1265+
1 | 1 | |
1266+
2 | 2 | 2 |
1267+
3 | 4 | 4 | 4
1268+
(3 rows)
1269+
1270+
DELETE FROM users WHERE id = 4;
1271+
SELECT * FROM tasks;
1272+
id | owner | worker | checked_by
1273+
----+-------+--------+------------
1274+
1 | 1 | |
1275+
2 | 2 | 2 |
1276+
3 | | |
1277+
(3 rows)
1278+
1279+
-- could fail with only 2 changes to make, if row was already updated
1280+
BEGIN;
1281+
UPDATE tasks set id=id WHERE id=2;
1282+
SELECT * FROM tasks;
1283+
id | owner | worker | checked_by
1284+
----+-------+--------+------------
1285+
1 | 1 | |
1286+
3 | | |
1287+
2 | 2 | 2 |
1288+
(3 rows)
1289+
1290+
DELETE FROM users WHERE id = 2;
1291+
SELECT * FROM tasks;
1292+
id | owner | worker | checked_by
1293+
----+-------+--------+------------
1294+
1 | 1 | |
1295+
3 | | |
1296+
2 | | |
1297+
(3 rows)
1298+
1299+
COMMIT;

0 commit comments

Comments
 (0)