diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index 5a4591e045..570cf95eaf 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -2352,27 +2352,26 @@ simple_heap_insert(Relation relation, HeapTuple tup) * * relation - table to be modified (caller must hold suitable lock) * tid - TID of tuple to be deleted - * ctid - output parameter, used only for failure case (see below) - * update_xmax - output parameter, used only for failure case (see below) * cid - delete command ID (used for visibility test, and stored into * cmax if successful) * crosscheck - if not InvalidSnapshot, also check tuple against this * wait - true if should wait for any conflicting update to commit/abort + * hufd - output parameter, filled in failure cases (see below) * * Normal, successful return value is HeapTupleMayBeUpdated, which * actually means we did delete it. Failure return codes are * HeapTupleSelfUpdated, HeapTupleUpdated, or HeapTupleBeingUpdated * (the last only possible if wait == false). * - * In the failure cases, the routine returns the tuple's t_ctid and t_xmax. - * If t_ctid is the same as tid, the tuple was deleted; if different, the - * tuple was updated, and t_ctid is the location of the replacement tuple. - * (t_xmax is needed to verify that the replacement tuple matches.) + * In the failure cases, the routine fills *hufd with the tuple's t_ctid, + * t_xmax, and t_cmax (the last only for HeapTupleSelfUpdated, since we + * cannot obtain cmax from a combocid generated by another transaction). + * See comments for struct HeapUpdateFailureData for additional info. */ HTSU_Result heap_delete(Relation relation, ItemPointer tid, - ItemPointer ctid, TransactionId *update_xmax, - CommandId cid, Snapshot crosscheck, bool wait) + CommandId cid, Snapshot crosscheck, bool wait, + HeapUpdateFailureData *hufd) { HTSU_Result result; TransactionId xid = GetCurrentTransactionId(); @@ -2533,8 +2532,12 @@ l1: result == HeapTupleUpdated || result == HeapTupleBeingUpdated); Assert(!(tp.t_data->t_infomask & HEAP_XMAX_INVALID)); - *ctid = tp.t_data->t_ctid; - *update_xmax = HeapTupleHeaderGetXmax(tp.t_data); + hufd->ctid = tp.t_data->t_ctid; + hufd->xmax = HeapTupleHeaderGetXmax(tp.t_data); + if (result == HeapTupleSelfUpdated) + hufd->cmax = HeapTupleHeaderGetCmax(tp.t_data); + else + hufd->cmax = 0; /* for lack of an InvalidCommandId value */ UnlockReleaseBuffer(buffer); if (have_tuple_lock) UnlockTuple(relation, &(tp.t_self), ExclusiveLock); @@ -2666,13 +2669,12 @@ void simple_heap_delete(Relation relation, ItemPointer tid) { HTSU_Result result; - ItemPointerData update_ctid; - TransactionId update_xmax; + HeapUpdateFailureData hufd; result = heap_delete(relation, tid, - &update_ctid, &update_xmax, GetCurrentCommandId(true), InvalidSnapshot, - true /* wait for commit */ ); + true /* wait for commit */, + &hufd); switch (result) { case HeapTupleSelfUpdated: @@ -2703,12 +2705,11 @@ simple_heap_delete(Relation relation, ItemPointer tid) * relation - table to be modified (caller must hold suitable lock) * otid - TID of old tuple to be replaced * newtup - newly constructed tuple data to store - * ctid - output parameter, used only for failure case (see below) - * update_xmax - output parameter, used only for failure case (see below) * cid - update command ID (used for visibility test, and stored into * cmax/cmin if successful) * crosscheck - if not InvalidSnapshot, also check old tuple against this * wait - true if should wait for any conflicting update to commit/abort + * hufd - output parameter, filled in failure cases (see below) * * Normal, successful return value is HeapTupleMayBeUpdated, which * actually means we *did* update it. Failure return codes are @@ -2721,15 +2722,15 @@ simple_heap_delete(Relation relation, ItemPointer tid) * update was done. However, any TOAST changes in the new tuple's * data are not reflected into *newtup. * - * In the failure cases, the routine returns the tuple's t_ctid and t_xmax. - * If t_ctid is the same as otid, the tuple was deleted; if different, the - * tuple was updated, and t_ctid is the location of the replacement tuple. - * (t_xmax is needed to verify that the replacement tuple matches.) + * In the failure cases, the routine fills *hufd with the tuple's t_ctid, + * t_xmax, and t_cmax (the last only for HeapTupleSelfUpdated, since we + * cannot obtain cmax from a combocid generated by another transaction). + * See comments for struct HeapUpdateFailureData for additional info. */ HTSU_Result heap_update(Relation relation, ItemPointer otid, HeapTuple newtup, - ItemPointer ctid, TransactionId *update_xmax, - CommandId cid, Snapshot crosscheck, bool wait) + CommandId cid, Snapshot crosscheck, bool wait, + HeapUpdateFailureData *hufd) { HTSU_Result result; TransactionId xid = GetCurrentTransactionId(); @@ -2908,8 +2909,12 @@ l2: result == HeapTupleUpdated || result == HeapTupleBeingUpdated); Assert(!(oldtup.t_data->t_infomask & HEAP_XMAX_INVALID)); - *ctid = oldtup.t_data->t_ctid; - *update_xmax = HeapTupleHeaderGetXmax(oldtup.t_data); + hufd->ctid = oldtup.t_data->t_ctid; + hufd->xmax = HeapTupleHeaderGetXmax(oldtup.t_data); + if (result == HeapTupleSelfUpdated) + hufd->cmax = HeapTupleHeaderGetCmax(oldtup.t_data); + else + hufd->cmax = 0; /* for lack of an InvalidCommandId value */ UnlockReleaseBuffer(buffer); if (have_tuple_lock) UnlockTuple(relation, &(oldtup.t_self), ExclusiveLock); @@ -3379,13 +3384,12 @@ void simple_heap_update(Relation relation, ItemPointer otid, HeapTuple tup) { HTSU_Result result; - ItemPointerData update_ctid; - TransactionId update_xmax; + HeapUpdateFailureData hufd; result = heap_update(relation, otid, tup, - &update_ctid, &update_xmax, GetCurrentCommandId(true), InvalidSnapshot, - true /* wait for commit */ ); + true /* wait for commit */, + &hufd); switch (result) { case HeapTupleSelfUpdated: @@ -3423,18 +3427,17 @@ simple_heap_update(Relation relation, ItemPointer otid, HeapTuple tup) * Output parameters: * *tuple: all fields filled in * *buffer: set to buffer holding tuple (pinned but not locked at exit) - * *ctid: set to tuple's t_ctid, but only in failure cases - * *update_xmax: set to tuple's xmax, but only in failure cases + * *hufd: filled in failure cases (see below) * * Function result may be: * HeapTupleMayBeUpdated: lock was successfully acquired * HeapTupleSelfUpdated: lock failed because tuple updated by self * HeapTupleUpdated: lock failed because tuple updated by other xact * - * In the failure cases, the routine returns the tuple's t_ctid and t_xmax. - * If t_ctid is the same as t_self, the tuple was deleted; if different, the - * tuple was updated, and t_ctid is the location of the replacement tuple. - * (t_xmax is needed to verify that the replacement tuple matches.) + * In the failure cases, the routine fills *hufd with the tuple's t_ctid, + * t_xmax, and t_cmax (the last only for HeapTupleSelfUpdated, since we + * cannot obtain cmax from a combocid generated by another transaction). + * See comments for struct HeapUpdateFailureData for additional info. * * * NOTES: because the shared-memory lock table is of finite size, but users @@ -3470,9 +3473,9 @@ simple_heap_update(Relation relation, ItemPointer otid, HeapTuple tup) * conflict for a tuple, we don't incur any extra overhead. */ HTSU_Result -heap_lock_tuple(Relation relation, HeapTuple tuple, Buffer *buffer, - ItemPointer ctid, TransactionId *update_xmax, - CommandId cid, LockTupleMode mode, bool nowait) +heap_lock_tuple(Relation relation, HeapTuple tuple, + CommandId cid, LockTupleMode mode, bool nowait, + Buffer *buffer, HeapUpdateFailureData *hufd) { HTSU_Result result; ItemPointer tid = &(tuple->t_self); @@ -3657,8 +3660,12 @@ l3: { Assert(result == HeapTupleSelfUpdated || result == HeapTupleUpdated); Assert(!(tuple->t_data->t_infomask & HEAP_XMAX_INVALID)); - *ctid = tuple->t_data->t_ctid; - *update_xmax = HeapTupleHeaderGetXmax(tuple->t_data); + hufd->ctid = tuple->t_data->t_ctid; + hufd->xmax = HeapTupleHeaderGetXmax(tuple->t_data); + if (result == HeapTupleSelfUpdated) + hufd->cmax = HeapTupleHeaderGetCmax(tuple->t_data); + else + hufd->cmax = 0; /* for lack of an InvalidCommandId value */ LockBuffer(*buffer, BUFFER_LOCK_UNLOCK); if (have_tuple_lock) UnlockTuple(relation, tid, tuple_lock_type); diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index 4d3ed9cb62..98b82074b5 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -2571,8 +2571,7 @@ GetTupleForTrigger(EState *estate, if (newSlot != NULL) { HTSU_Result test; - ItemPointerData update_ctid; - TransactionId update_xmax; + HeapUpdateFailureData hufd; *newSlot = NULL; @@ -2584,13 +2583,27 @@ GetTupleForTrigger(EState *estate, */ ltrmark:; tuple.t_self = *tid; - test = heap_lock_tuple(relation, &tuple, &buffer, - &update_ctid, &update_xmax, + test = heap_lock_tuple(relation, &tuple, estate->es_output_cid, - LockTupleExclusive, false); + LockTupleExclusive, false /* wait */, + &buffer, &hufd); switch (test) { case HeapTupleSelfUpdated: + /* + * The target tuple was already updated or deleted by the + * current command, or by a later command in the current + * transaction. We ignore the tuple in the former case, and + * throw error in the latter case, for the same reasons + * enumerated in ExecUpdate and ExecDelete in + * nodeModifyTable.c. + */ + if (hufd.cmax != estate->es_output_cid) + ereport(ERROR, + (errcode(ERRCODE_TRIGGERED_DATA_CHANGE_VIOLATION), + errmsg("tuple to be updated was already modified by an operation triggered by the current command"), + errhint("Consider using an AFTER trigger instead of a BEFORE trigger to propagate changes to other rows."))); + /* treat it as deleted; do not process */ ReleaseBuffer(buffer); return NULL; @@ -2604,7 +2617,7 @@ ltrmark:; ereport(ERROR, (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE), errmsg("could not serialize access due to concurrent update"))); - if (!ItemPointerEquals(&update_ctid, &tuple.t_self)) + if (!ItemPointerEquals(&hufd.ctid, &tuple.t_self)) { /* it was updated, so look at the updated version */ TupleTableSlot *epqslot; @@ -2613,11 +2626,11 @@ ltrmark:; epqstate, relation, relinfo->ri_RangeTableIndex, - &update_ctid, - update_xmax); + &hufd.ctid, + hufd.xmax); if (!TupIsNull(epqslot)) { - *tid = update_ctid; + *tid = hufd.ctid; *newSlot = epqslot; /* diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index d966be543e..dbd3755b1b 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -1802,8 +1802,7 @@ EvalPlanQualFetch(EState *estate, Relation relation, int lockmode, if (heap_fetch(relation, &SnapshotDirty, &tuple, &buffer, true, NULL)) { HTSU_Result test; - ItemPointerData update_ctid; - TransactionId update_xmax; + HeapUpdateFailureData hufd; /* * If xmin isn't what we're expecting, the slot must have been @@ -1838,13 +1837,13 @@ EvalPlanQualFetch(EState *estate, Relation relation, int lockmode, /* * If tuple was inserted by our own transaction, we have to check * cmin against es_output_cid: cmin >= current CID means our - * command cannot see the tuple, so we should ignore it. Without - * this we are open to the "Halloween problem" of indefinitely - * re-updating the same tuple. (We need not check cmax because - * HeapTupleSatisfiesDirty will consider a tuple deleted by our - * transaction dead, regardless of cmax.) We just checked that - * priorXmax == xmin, so we can test that variable instead of - * doing HeapTupleHeaderGetXmin again. + * command cannot see the tuple, so we should ignore it. + * Otherwise heap_lock_tuple() will throw an error, and so would + * any later attempt to update or delete the tuple. (We need not + * check cmax because HeapTupleSatisfiesDirty will consider a + * tuple deleted by our transaction dead, regardless of cmax.) + * Wee just checked that priorXmax == xmin, so we can test that + * variable instead of doing HeapTupleHeaderGetXmin again. */ if (TransactionIdIsCurrentTransactionId(priorXmax) && HeapTupleHeaderGetCmin(tuple.t_data) >= estate->es_output_cid) @@ -1856,17 +1855,29 @@ EvalPlanQualFetch(EState *estate, Relation relation, int lockmode, /* * This is a live tuple, so now try to lock it. */ - test = heap_lock_tuple(relation, &tuple, &buffer, - &update_ctid, &update_xmax, + test = heap_lock_tuple(relation, &tuple, estate->es_output_cid, - lockmode, false); + lockmode, false /* wait */, + &buffer, &hufd); /* We now have two pins on the buffer, get rid of one */ ReleaseBuffer(buffer); switch (test) { case HeapTupleSelfUpdated: - /* treat it as deleted; do not process */ + /* + * The target tuple was already updated or deleted by the + * current command, or by a later command in the current + * transaction. We *must* ignore the tuple in the former + * case, so as to avoid the "Halloween problem" of + * repeated update attempts. In the latter case it might + * be sensible to fetch the updated tuple instead, but + * doing so would require changing heap_lock_tuple as well + * as heap_update and heap_delete to not complain about + * updating "invisible" tuples, which seems pretty scary. + * So for now, treat the tuple as deleted and do not + * process. + */ ReleaseBuffer(buffer); return NULL; @@ -1880,12 +1891,12 @@ EvalPlanQualFetch(EState *estate, Relation relation, int lockmode, ereport(ERROR, (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE), errmsg("could not serialize access due to concurrent update"))); - if (!ItemPointerEquals(&update_ctid, &tuple.t_self)) + if (!ItemPointerEquals(&hufd.ctid, &tuple.t_self)) { /* it was updated, so look at the updated version */ - tuple.t_self = update_ctid; + tuple.t_self = hufd.ctid; /* updated row should have xmin matching this xmax */ - priorXmax = update_xmax; + priorXmax = hufd.xmax; continue; } /* tuple was deleted, so give up */ diff --git a/src/backend/executor/nodeLockRows.c b/src/backend/executor/nodeLockRows.c index ec0825b460..6474393d7f 100644 --- a/src/backend/executor/nodeLockRows.c +++ b/src/backend/executor/nodeLockRows.c @@ -71,8 +71,7 @@ lnext: bool isNull; HeapTupleData tuple; Buffer buffer; - ItemPointerData update_ctid; - TransactionId update_xmax; + HeapUpdateFailureData hufd; LockTupleMode lockmode; HTSU_Result test; HeapTuple copyTuple; @@ -117,15 +116,26 @@ lnext: else lockmode = LockTupleShared; - test = heap_lock_tuple(erm->relation, &tuple, &buffer, - &update_ctid, &update_xmax, + test = heap_lock_tuple(erm->relation, &tuple, estate->es_output_cid, - lockmode, erm->noWait); + lockmode, erm->noWait, + &buffer, &hufd); ReleaseBuffer(buffer); switch (test) { case HeapTupleSelfUpdated: - /* treat it as deleted; do not process */ + /* + * The target tuple was already updated or deleted by the + * current command, or by a later command in the current + * transaction. We *must* ignore the tuple in the former + * case, so as to avoid the "Halloween problem" of repeated + * update attempts. In the latter case it might be sensible + * to fetch the updated tuple instead, but doing so would + * require changing heap_lock_tuple as well as heap_update and + * heap_delete to not complain about updating "invisible" + * tuples, which seems pretty scary. So for now, treat the + * tuple as deleted and do not process. + */ goto lnext; case HeapTupleMayBeUpdated: @@ -137,8 +147,7 @@ lnext: ereport(ERROR, (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE), errmsg("could not serialize access due to concurrent update"))); - if (ItemPointerEquals(&update_ctid, - &tuple.t_self)) + if (ItemPointerEquals(&hufd.ctid, &tuple.t_self)) { /* Tuple was deleted, so don't return it */ goto lnext; @@ -146,7 +155,7 @@ lnext: /* updated, so fetch and lock the updated version */ copyTuple = EvalPlanQualFetch(estate, erm->relation, lockmode, - &update_ctid, update_xmax); + &hufd.ctid, hufd.xmax); if (copyTuple == NULL) { diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index 26a59d0121..d31015c654 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -295,8 +295,7 @@ ExecDelete(ItemPointer tupleid, ResultRelInfo *resultRelInfo; Relation resultRelationDesc; HTSU_Result result; - ItemPointerData update_ctid; - TransactionId update_xmax; + HeapUpdateFailureData hufd; /* * get information on the (current) result relation @@ -348,14 +347,44 @@ ExecDelete(ItemPointer tupleid, */ ldelete:; result = heap_delete(resultRelationDesc, tupleid, - &update_ctid, &update_xmax, estate->es_output_cid, estate->es_crosscheck_snapshot, - true /* wait for commit */ ); + true /* wait for commit */, + &hufd); switch (result) { case HeapTupleSelfUpdated: - /* already deleted by self; nothing to do */ + /* + * The target tuple was already updated or deleted by the + * current command, or by a later command in the current + * transaction. The former case is possible in a join DELETE + * where multiple tuples join to the same target tuple. + * This is somewhat questionable, but Postgres has always + * allowed it: we just ignore additional deletion attempts. + * + * The latter case arises if the tuple is modified by a + * command in a BEFORE trigger, or perhaps by a command in a + * volatile function used in the query. In such situations we + * should not ignore the deletion, but it is equally unsafe to + * proceed. We don't want to discard the original DELETE + * while keeping the triggered actions based on its deletion; + * and it would be no better to allow the original DELETE + * while discarding updates that it triggered. The row update + * carries some information that might be important according + * to business rules; so throwing an error is the only safe + * course. + * + * If a trigger actually intends this type of interaction, + * it can re-execute the DELETE and then return NULL to + * cancel the outer delete. + */ + if (hufd.cmax != estate->es_output_cid) + ereport(ERROR, + (errcode(ERRCODE_TRIGGERED_DATA_CHANGE_VIOLATION), + errmsg("tuple to be updated was already modified by an operation triggered by the current command"), + errhint("Consider using an AFTER trigger instead of a BEFORE trigger to propagate changes to other rows."))); + + /* Else, already deleted by self; nothing to do */ return NULL; case HeapTupleMayBeUpdated: @@ -366,7 +395,7 @@ ldelete:; ereport(ERROR, (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE), errmsg("could not serialize access due to concurrent update"))); - if (!ItemPointerEquals(tupleid, &update_ctid)) + if (!ItemPointerEquals(tupleid, &hufd.ctid)) { TupleTableSlot *epqslot; @@ -374,11 +403,11 @@ ldelete:; epqstate, resultRelationDesc, resultRelInfo->ri_RangeTableIndex, - &update_ctid, - update_xmax); + &hufd.ctid, + hufd.xmax); if (!TupIsNull(epqslot)) { - *tupleid = update_ctid; + *tupleid = hufd.ctid; goto ldelete; } } @@ -482,8 +511,7 @@ ExecUpdate(ItemPointer tupleid, ResultRelInfo *resultRelInfo; Relation resultRelationDesc; HTSU_Result result; - ItemPointerData update_ctid; - TransactionId update_xmax; + HeapUpdateFailureData hufd; List *recheckIndexes = NIL; /* @@ -564,14 +592,43 @@ lreplace:; * mode transactions. */ result = heap_update(resultRelationDesc, tupleid, tuple, - &update_ctid, &update_xmax, estate->es_output_cid, estate->es_crosscheck_snapshot, - true /* wait for commit */ ); + true /* wait for commit */, + &hufd); switch (result) { case HeapTupleSelfUpdated: - /* already deleted by self; nothing to do */ + /* + * The target tuple was already updated or deleted by the + * current command, or by a later command in the current + * transaction. The former case is possible in a join UPDATE + * where multiple tuples join to the same target tuple. + * This is pretty questionable, but Postgres has always + * allowed it: we just execute the first update action and + * ignore additional update attempts. + * + * The latter case arises if the tuple is modified by a + * command in a BEFORE trigger, or perhaps by a command in a + * volatile function used in the query. In such situations we + * should not ignore the update, but it is equally unsafe to + * proceed. We don't want to discard the original UPDATE + * while keeping the triggered actions based on it; and we + * have no principled way to merge this update with the + * previous ones. So throwing an error is the only safe + * course. + * + * If a trigger actually intends this type of interaction, + * it can re-execute the UPDATE (assuming it can figure out + * how) and then return NULL to cancel the outer update. + */ + if (hufd.cmax != estate->es_output_cid) + ereport(ERROR, + (errcode(ERRCODE_TRIGGERED_DATA_CHANGE_VIOLATION), + errmsg("tuple to be updated was already modified by an operation triggered by the current command"), + errhint("Consider using an AFTER trigger instead of a BEFORE trigger to propagate changes to other rows."))); + + /* Else, already updated by self; nothing to do */ return NULL; case HeapTupleMayBeUpdated: @@ -582,7 +639,7 @@ lreplace:; ereport(ERROR, (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE), errmsg("could not serialize access due to concurrent update"))); - if (!ItemPointerEquals(tupleid, &update_ctid)) + if (!ItemPointerEquals(tupleid, &hufd.ctid)) { TupleTableSlot *epqslot; @@ -590,11 +647,11 @@ lreplace:; epqstate, resultRelationDesc, resultRelInfo->ri_RangeTableIndex, - &update_ctid, - update_xmax); + &hufd.ctid, + hufd.xmax); if (!TupIsNull(epqslot)) { - *tupleid = update_ctid; + *tupleid = hufd.ctid; slot = ExecFilterJunk(resultRelInfo->ri_junkFilter, epqslot); tuple = ExecMaterializeSlot(slot); goto lreplace; diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h index a4ab0627a0..3be6b3a39f 100644 --- a/src/include/access/heapam.h +++ b/src/include/access/heapam.h @@ -35,6 +35,29 @@ typedef enum LockTupleExclusive } LockTupleMode; +/* + * When heap_update, heap_delete, or heap_lock_tuple fail because the target + * tuple is already outdated, they fill in this struct to provide information + * to the caller about what happened. + * ctid is the target's ctid link: it is the same as the target's TID if the + * target was deleted, or the location of the replacement tuple if the target + * was updated. + * xmax is the outdating transaction's XID. If the caller wants to visit the + * replacement tuple, it must check that this matches before believing the + * replacement is really a match. + * cmax is the outdating command's CID, but only when the failure code is + * HeapTupleSelfUpdated (i.e., something in the current transaction outdated + * the tuple); otherwise cmax is zero. (We make this restriction because + * HeapTupleHeaderGetCmax doesn't work for tuples outdated in other + * transactions.) + */ +typedef struct HeapUpdateFailureData +{ + ItemPointerData ctid; + TransactionId xmax; + CommandId cmax; +} HeapUpdateFailureData; + /* ---------------- * function prototypes for heap access method @@ -100,16 +123,15 @@ extern Oid heap_insert(Relation relation, HeapTuple tup, CommandId cid, extern void heap_multi_insert(Relation relation, HeapTuple *tuples, int ntuples, CommandId cid, int options, BulkInsertState bistate); extern HTSU_Result heap_delete(Relation relation, ItemPointer tid, - ItemPointer ctid, TransactionId *update_xmax, - CommandId cid, Snapshot crosscheck, bool wait); + CommandId cid, Snapshot crosscheck, bool wait, + HeapUpdateFailureData *hufd); extern HTSU_Result heap_update(Relation relation, ItemPointer otid, HeapTuple newtup, - ItemPointer ctid, TransactionId *update_xmax, - CommandId cid, Snapshot crosscheck, bool wait); + CommandId cid, Snapshot crosscheck, bool wait, + HeapUpdateFailureData *hufd); extern HTSU_Result heap_lock_tuple(Relation relation, HeapTuple tuple, - Buffer *buffer, ItemPointer ctid, - TransactionId *update_xmax, CommandId cid, - LockTupleMode mode, bool nowait); + CommandId cid, LockTupleMode mode, bool nowait, + Buffer *buffer, HeapUpdateFailureData *hufd); extern void heap_inplace_update(Relation relation, HeapTuple tuple); extern bool heap_freeze_tuple(HeapTupleHeader tuple, TransactionId cutoff_xid); extern bool heap_tuple_needs_freeze(HeapTupleHeader tuple, TransactionId cutoff_xid, diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out index b5af066558..94ea61f80c 100644 --- a/src/test/regress/expected/triggers.out +++ b/src/test/regress/expected/triggers.out @@ -1550,3 +1550,197 @@ drop table depth_a, depth_b, depth_c; drop function depth_a_tf(); drop function depth_b_tf(); drop function depth_c_tf(); +-- +-- Test updates to rows during firing of BEFORE ROW triggers. +-- As of 9.2, such cases should be rejected (see bug #6123). +-- +create temp table parent ( + aid int not null primary key, + val1 text, + val2 text, + val3 text, + val4 text, + bcnt int not null default 0); +create temp table child ( + bid int not null primary key, + aid int not null, + val1 text); +create function parent_upd_func() + returns trigger language plpgsql as +$$ +begin + if old.val1 <> new.val1 then + new.val2 = new.val1; + delete from child where child.aid = new.aid and child.val1 = new.val1; + end if; + return new; +end; +$$; +create trigger parent_upd_trig before update on parent + for each row execute procedure parent_upd_func(); +create function parent_del_func() + returns trigger language plpgsql as +$$ +begin + delete from child where aid = old.aid; + return old; +end; +$$; +create trigger parent_del_trig before delete on parent + for each row execute procedure parent_del_func(); +create function child_ins_func() + returns trigger language plpgsql as +$$ +begin + update parent set bcnt = bcnt + 1 where aid = new.aid; + return new; +end; +$$; +create trigger child_ins_trig after insert on child + for each row execute procedure child_ins_func(); +create function child_del_func() + returns trigger language plpgsql as +$$ +begin + update parent set bcnt = bcnt - 1 where aid = old.aid; + return old; +end; +$$; +create trigger child_del_trig after delete on child + for each row execute procedure child_del_func(); +insert into parent values (1, 'a', 'a', 'a', 'a', 0); +insert into child values (10, 1, 'b'); +select * from parent; select * from child; + aid | val1 | val2 | val3 | val4 | bcnt +-----+------+------+------+------+------ + 1 | a | a | a | a | 1 +(1 row) + + bid | aid | val1 +-----+-----+------ + 10 | 1 | b +(1 row) + +update parent set val1 = 'b' where aid = 1; -- should fail +ERROR: tuple to be updated was already modified by an operation triggered by the current command +HINT: Consider using an AFTER trigger instead of a BEFORE trigger to propagate changes to other rows. +select * from parent; select * from child; + aid | val1 | val2 | val3 | val4 | bcnt +-----+------+------+------+------+------ + 1 | a | a | a | a | 1 +(1 row) + + bid | aid | val1 +-----+-----+------ + 10 | 1 | b +(1 row) + +delete from parent where aid = 1; -- should fail +ERROR: tuple to be updated was already modified by an operation triggered by the current command +HINT: Consider using an AFTER trigger instead of a BEFORE trigger to propagate changes to other rows. +select * from parent; select * from child; + aid | val1 | val2 | val3 | val4 | bcnt +-----+------+------+------+------+------ + 1 | a | a | a | a | 1 +(1 row) + + bid | aid | val1 +-----+-----+------ + 10 | 1 | b +(1 row) + +-- replace the trigger function with one that restarts the deletion after +-- having modified a child +create or replace function parent_del_func() + returns trigger language plpgsql as +$$ +begin + delete from child where aid = old.aid; + if found then + delete from parent where aid = old.aid; + return null; -- cancel outer deletion + end if; + return old; +end; +$$; +delete from parent where aid = 1; +select * from parent; select * from child; + aid | val1 | val2 | val3 | val4 | bcnt +-----+------+------+------+------+------ +(0 rows) + + bid | aid | val1 +-----+-----+------ +(0 rows) + +drop table parent, child; +drop function parent_upd_func(); +drop function parent_del_func(); +drop function child_ins_func(); +drop function child_del_func(); +-- similar case, but with a self-referencing FK so that parent and child +-- rows can be affected by a single operation +create temp table self_ref_trigger ( + id int primary key, + parent int references self_ref_trigger, + data text, + nchildren int not null default 0 +); +create function self_ref_trigger_ins_func() + returns trigger language plpgsql as +$$ +begin + if new.parent is not null then + update self_ref_trigger set nchildren = nchildren + 1 + where id = new.parent; + end if; + return new; +end; +$$; +create trigger self_ref_trigger_ins_trig before insert on self_ref_trigger + for each row execute procedure self_ref_trigger_ins_func(); +create function self_ref_trigger_del_func() + returns trigger language plpgsql as +$$ +begin + if old.parent is not null then + update self_ref_trigger set nchildren = nchildren - 1 + where id = old.parent; + end if; + return old; +end; +$$; +create trigger self_ref_trigger_del_trig before delete on self_ref_trigger + for each row execute procedure self_ref_trigger_del_func(); +insert into self_ref_trigger values (1, null, 'root'); +insert into self_ref_trigger values (2, 1, 'root child A'); +insert into self_ref_trigger values (3, 1, 'root child B'); +insert into self_ref_trigger values (4, 2, 'grandchild 1'); +insert into self_ref_trigger values (5, 3, 'grandchild 2'); +update self_ref_trigger set data = 'root!' where id = 1; +select * from self_ref_trigger; + id | parent | data | nchildren +----+--------+--------------+----------- + 2 | 1 | root child A | 1 + 4 | 2 | grandchild 1 | 0 + 3 | 1 | root child B | 1 + 5 | 3 | grandchild 2 | 0 + 1 | | root! | 2 +(5 rows) + +delete from self_ref_trigger; +ERROR: tuple to be updated was already modified by an operation triggered by the current command +HINT: Consider using an AFTER trigger instead of a BEFORE trigger to propagate changes to other rows. +select * from self_ref_trigger; + id | parent | data | nchildren +----+--------+--------------+----------- + 2 | 1 | root child A | 1 + 4 | 2 | grandchild 1 | 0 + 3 | 1 | root child B | 1 + 5 | 3 | grandchild 2 | 0 + 1 | | root! | 2 +(5 rows) + +drop table self_ref_trigger; +drop function self_ref_trigger_ins_func(); +drop function self_ref_trigger_del_func(); diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql index f88cb81940..78c5407560 100644 --- a/src/test/regress/sql/triggers.sql +++ b/src/test/regress/sql/triggers.sql @@ -1025,3 +1025,158 @@ drop table depth_a, depth_b, depth_c; drop function depth_a_tf(); drop function depth_b_tf(); drop function depth_c_tf(); + +-- +-- Test updates to rows during firing of BEFORE ROW triggers. +-- As of 9.2, such cases should be rejected (see bug #6123). +-- + +create temp table parent ( + aid int not null primary key, + val1 text, + val2 text, + val3 text, + val4 text, + bcnt int not null default 0); +create temp table child ( + bid int not null primary key, + aid int not null, + val1 text); + +create function parent_upd_func() + returns trigger language plpgsql as +$$ +begin + if old.val1 <> new.val1 then + new.val2 = new.val1; + delete from child where child.aid = new.aid and child.val1 = new.val1; + end if; + return new; +end; +$$; +create trigger parent_upd_trig before update on parent + for each row execute procedure parent_upd_func(); + +create function parent_del_func() + returns trigger language plpgsql as +$$ +begin + delete from child where aid = old.aid; + return old; +end; +$$; +create trigger parent_del_trig before delete on parent + for each row execute procedure parent_del_func(); + +create function child_ins_func() + returns trigger language plpgsql as +$$ +begin + update parent set bcnt = bcnt + 1 where aid = new.aid; + return new; +end; +$$; +create trigger child_ins_trig after insert on child + for each row execute procedure child_ins_func(); + +create function child_del_func() + returns trigger language plpgsql as +$$ +begin + update parent set bcnt = bcnt - 1 where aid = old.aid; + return old; +end; +$$; +create trigger child_del_trig after delete on child + for each row execute procedure child_del_func(); + +insert into parent values (1, 'a', 'a', 'a', 'a', 0); +insert into child values (10, 1, 'b'); +select * from parent; select * from child; + +update parent set val1 = 'b' where aid = 1; -- should fail +select * from parent; select * from child; + +delete from parent where aid = 1; -- should fail +select * from parent; select * from child; + +-- replace the trigger function with one that restarts the deletion after +-- having modified a child +create or replace function parent_del_func() + returns trigger language plpgsql as +$$ +begin + delete from child where aid = old.aid; + if found then + delete from parent where aid = old.aid; + return null; -- cancel outer deletion + end if; + return old; +end; +$$; + +delete from parent where aid = 1; +select * from parent; select * from child; + +drop table parent, child; + +drop function parent_upd_func(); +drop function parent_del_func(); +drop function child_ins_func(); +drop function child_del_func(); + +-- similar case, but with a self-referencing FK so that parent and child +-- rows can be affected by a single operation + +create temp table self_ref_trigger ( + id int primary key, + parent int references self_ref_trigger, + data text, + nchildren int not null default 0 +); + +create function self_ref_trigger_ins_func() + returns trigger language plpgsql as +$$ +begin + if new.parent is not null then + update self_ref_trigger set nchildren = nchildren + 1 + where id = new.parent; + end if; + return new; +end; +$$; +create trigger self_ref_trigger_ins_trig before insert on self_ref_trigger + for each row execute procedure self_ref_trigger_ins_func(); + +create function self_ref_trigger_del_func() + returns trigger language plpgsql as +$$ +begin + if old.parent is not null then + update self_ref_trigger set nchildren = nchildren - 1 + where id = old.parent; + end if; + return old; +end; +$$; +create trigger self_ref_trigger_del_trig before delete on self_ref_trigger + for each row execute procedure self_ref_trigger_del_func(); + +insert into self_ref_trigger values (1, null, 'root'); +insert into self_ref_trigger values (2, 1, 'root child A'); +insert into self_ref_trigger values (3, 1, 'root child B'); +insert into self_ref_trigger values (4, 2, 'grandchild 1'); +insert into self_ref_trigger values (5, 3, 'grandchild 2'); + +update self_ref_trigger set data = 'root!' where id = 1; + +select * from self_ref_trigger; + +delete from self_ref_trigger; + +select * from self_ref_trigger; + +drop table self_ref_trigger; +drop function self_ref_trigger_ins_func(); +drop function self_ref_trigger_del_func();