Tom Lane <[email protected]> wrote:
> You forgot to attach the patch, but the approach seems totally
> Rube Goldberg to me anyway. Why not just fix heap_update/
> heap_delete to return the additional information? It's not like
> we don't whack their parameter lists around regularly.
>
> Rather than having three output parameters to support the case,
> I'm a bit inclined to merge them into a single-purpose struct
> type. But that's mostly cosmetic.
OK, I got rid of the parrots and candles and added a structure to
hold the data returned only on failure.
Am I getting closer?
-Kevin
*** a/src/backend/access/heap/heapam.c
--- b/src/backend/access/heap/heapam.c
***************
*** 2336,2343 **** simple_heap_insert(Relation relation, HeapTuple tup)
*/
HTSU_Result
heap_delete(Relation relation, ItemPointer tid,
! ItemPointer ctid, TransactionId *update_xmax,
! CommandId cid, Snapshot crosscheck, bool wait)
{
HTSU_Result result;
TransactionId xid = GetCurrentTransactionId();
--- 2336,2343 ----
*/
HTSU_Result
heap_delete(Relation relation, ItemPointer tid,
! HeapUpdateFailureData *hufd, CommandId cid,
! Snapshot crosscheck, bool wait)
{
HTSU_Result result;
TransactionId xid = GetCurrentTransactionId();
***************
*** 2498,2505 **** 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);
UnlockReleaseBuffer(buffer);
if (have_tuple_lock)
UnlockTuple(relation, &(tp.t_self), ExclusiveLock);
--- 2498,2506 ----
result == HeapTupleUpdated ||
result == HeapTupleBeingUpdated);
Assert(!(tp.t_data->t_infomask & HEAP_XMAX_INVALID));
! hufd->update_ctid = tp.t_data->t_ctid;
! hufd->update_xmax = HeapTupleHeaderGetXmax(tp.t_data);
! hufd->update_cmax = HeapTupleHeaderGetCmax(tp.t_data);
UnlockReleaseBuffer(buffer);
if (have_tuple_lock)
UnlockTuple(relation, &(tp.t_self), ExclusiveLock);
***************
*** 2631,2643 **** void
simple_heap_delete(Relation relation, ItemPointer tid)
{
HTSU_Result result;
! ItemPointerData update_ctid;
! TransactionId update_xmax;
result = heap_delete(relation, tid,
! &update_ctid, &update_xmax,
! GetCurrentCommandId(true),
InvalidSnapshot,
! true /* wait for commit */ );
switch (result)
{
case HeapTupleSelfUpdated:
--- 2632,2642 ----
simple_heap_delete(Relation relation, ItemPointer tid)
{
HTSU_Result result;
! HeapUpdateFailureData hufd;
result = heap_delete(relation, tid,
! &hufd,
GetCurrentCommandId(true),
! InvalidSnapshot, true /* wait
for commit */ );
switch (result)
{
case HeapTupleSelfUpdated:
***************
*** 2693,2700 **** simple_heap_delete(Relation relation, ItemPointer tid)
*/
HTSU_Result
heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
! ItemPointer ctid, TransactionId *update_xmax,
! CommandId cid, Snapshot crosscheck, bool wait)
{
HTSU_Result result;
TransactionId xid = GetCurrentTransactionId();
--- 2692,2699 ----
*/
HTSU_Result
heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
! HeapUpdateFailureData *hufd, CommandId cid,
! Snapshot crosscheck, bool wait)
{
HTSU_Result result;
TransactionId xid = GetCurrentTransactionId();
***************
*** 2873,2880 **** 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);
UnlockReleaseBuffer(buffer);
if (have_tuple_lock)
UnlockTuple(relation, &(oldtup.t_self), ExclusiveLock);
--- 2872,2880 ----
result == HeapTupleUpdated ||
result == HeapTupleBeingUpdated);
Assert(!(oldtup.t_data->t_infomask & HEAP_XMAX_INVALID));
! hufd->update_ctid = oldtup.t_data->t_ctid;
! hufd->update_xmax = HeapTupleHeaderGetXmax(oldtup.t_data);
! hufd->update_cmax = HeapTupleHeaderGetCmax(oldtup.t_data);
UnlockReleaseBuffer(buffer);
if (have_tuple_lock)
UnlockTuple(relation, &(oldtup.t_self), ExclusiveLock);
***************
*** 3344,3356 **** void
simple_heap_update(Relation relation, ItemPointer otid, HeapTuple tup)
{
HTSU_Result result;
! ItemPointerData update_ctid;
! TransactionId update_xmax;
result = heap_update(relation, otid, tup,
! &update_ctid, &update_xmax,
! GetCurrentCommandId(true),
InvalidSnapshot,
! true /* wait for commit */ );
switch (result)
{
case HeapTupleSelfUpdated:
--- 3344,3354 ----
simple_heap_update(Relation relation, ItemPointer otid, HeapTuple tup)
{
HTSU_Result result;
! HeapUpdateFailureData hufd;
result = heap_update(relation, otid, tup,
! &hufd,
GetCurrentCommandId(true),
! InvalidSnapshot, true /* wait
for commit */ );
switch (result)
{
case HeapTupleSelfUpdated:
*** a/src/backend/executor/execMain.c
--- b/src/backend/executor/execMain.c
***************
*** 1921,1928 **** EvalPlanQualFetch(EState *estate, Relation relation, int
lockmode,
switch (test)
{
case HeapTupleSelfUpdated:
- /* treat it as deleted; do not process
*/
ReleaseBuffer(buffer);
return NULL;
case HeapTupleMayBeUpdated:
--- 1921,1936 ----
switch (test)
{
case HeapTupleSelfUpdated:
ReleaseBuffer(buffer);
+ if (!ItemPointerEquals(&update_ctid,
&tuple.t_self))
+ {
+ /* it was updated, so look at
the updated version */
+ tuple.t_self = update_ctid;
+ /* updated row should have xmin
matching this xmax */
+ priorXmax = update_xmax;
+ continue;
+ }
+ /* treat it as deleted; do not process
*/
return NULL;
case HeapTupleMayBeUpdated:
*** a/src/backend/executor/nodeModifyTable.c
--- b/src/backend/executor/nodeModifyTable.c
***************
*** 294,301 **** ExecDelete(ItemPointer tupleid,
ResultRelInfo *resultRelInfo;
Relation resultRelationDesc;
HTSU_Result result;
! ItemPointerData update_ctid;
! TransactionId update_xmax;
/*
* get information on the (current) result relation
--- 294,300 ----
ResultRelInfo *resultRelInfo;
Relation resultRelationDesc;
HTSU_Result result;
! HeapUpdateFailureData hufd;
/*
* get information on the (current) result relation
***************
*** 347,359 **** 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 */ );
switch (result)
{
case HeapTupleSelfUpdated:
/* already deleted by self; nothing to do */
return NULL;
--- 346,381 ----
*/
ldelete:;
result = heap_delete(resultRelationDesc, tupleid,
! &hufd,
estate->es_output_cid,
estate->es_crosscheck_snapshot,
true /* wait for
commit */ );
switch (result)
{
case HeapTupleSelfUpdated:
+ /*
+ * There is no sensible action to take if the
BEFORE DELETE
+ * trigger for a row issues an UPDATE for the
same row, either
+ * directly or by performing DML which fires
other triggers
+ * which do the update. 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 some of its
triggered actions.
+ * Updating the row which is being deleted
risks losing some
+ * information which might be important
according to business
+ * rules; so throwing an error is the only safe
course.
+ *
+ * We want to be careful not to trigger this
for join/updates
+ * which join to the same row more than once,
so we check
+ * whether the tuple was expired by a command
other than the
+ * one which initially fired the trigger.
+ */
+ if (hufd.update_cmax != estate->es_output_cid)
+ {
+ ereport(ERROR,
+
(errcode(ERRCODE_TRIGGERED_DATA_CHANGE_VIOLATION),
+ errmsg("cannot update
or delete a row from its BEFORE DELETE trigger"),
+ errhint("Consider
moving code to an AFTER DELETE trigger.")));
+ }
/* already deleted by self; nothing to do */
return NULL;
***************
*** 365,371 **** ldelete:;
ereport(ERROR,
(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
errmsg("could not
serialize access due to concurrent update")));
! if (!ItemPointerEquals(tupleid, &update_ctid))
{
TupleTableSlot *epqslot;
--- 387,393 ----
ereport(ERROR,
(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
errmsg("could not
serialize access due to concurrent update")));
! if (!ItemPointerEquals(tupleid,
&(hufd.update_ctid)))
{
TupleTableSlot *epqslot;
***************
*** 373,383 **** ldelete:;
epqstate,
resultRelationDesc,
resultRelInfo->ri_RangeTableIndex,
!
&update_ctid,
!
update_xmax);
if (!TupIsNull(epqslot))
{
! *tupleid = update_ctid;
goto ldelete;
}
}
--- 395,405 ----
epqstate,
resultRelationDesc,
resultRelInfo->ri_RangeTableIndex,
!
&(hufd.update_ctid),
!
hufd.update_xmax);
if (!TupIsNull(epqslot))
{
! *tupleid = hufd.update_ctid;
goto ldelete;
}
}
***************
*** 481,488 **** ExecUpdate(ItemPointer tupleid,
ResultRelInfo *resultRelInfo;
Relation resultRelationDesc;
HTSU_Result result;
! ItemPointerData update_ctid;
! TransactionId update_xmax;
List *recheckIndexes = NIL;
/*
--- 503,509 ----
ResultRelInfo *resultRelInfo;
Relation resultRelationDesc;
HTSU_Result result;
! HeapUpdateFailureData hufd;
List *recheckIndexes = NIL;
/*
***************
*** 563,575 **** 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 */ );
switch (result)
{
case HeapTupleSelfUpdated:
/* already deleted by self; nothing to do */
return NULL;
--- 584,617 ----
* mode transactions.
*/
result = heap_update(resultRelationDesc, tupleid, tuple,
! &hufd,
estate->es_output_cid,
estate->es_crosscheck_snapshot,
true /* wait for
commit */ );
switch (result)
{
case HeapTupleSelfUpdated:
+ /*
+ * There is no sensible action to take if the
BEFORE UPDATE
+ * trigger for a row issues another UPDATE for
the same row,
+ * either directly or by performing DML which
fires other
+ * triggers which do the update. We don't want
to discard the
+ * original UPDATE while keeping the triggered
actions based
+ * on its update; and it would be no better to
allow the
+ * original UPDATE while discarding some of its
triggered
+ * actions.
+ *
+ * We want to be careful not to trigger this
for join/updates
+ * which join to the same row more than once,
so we check
+ * whether the tuple was expired by a command
other than the
+ * one which initially fired the trigger.
+ */
+ if (hufd.update_cmax != estate->es_output_cid)
+ {
+ ereport(ERROR,
+
(errcode(ERRCODE_TRIGGERED_DATA_CHANGE_VIOLATION),
+ errmsg("cannot update
or delete a row from its BEFORE UPDATE trigger"),
+ errhint("Consider
moving code to an AFTER UPDATE trigger.")));
+ }
/* already deleted by self; nothing to do */
return NULL;
***************
*** 581,587 **** lreplace:;
ereport(ERROR,
(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
errmsg("could not
serialize access due to concurrent update")));
! if (!ItemPointerEquals(tupleid, &update_ctid))
{
TupleTableSlot *epqslot;
--- 623,629 ----
ereport(ERROR,
(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
errmsg("could not
serialize access due to concurrent update")));
! if (!ItemPointerEquals(tupleid,
&(hufd.update_ctid)))
{
TupleTableSlot *epqslot;
***************
*** 589,599 **** lreplace:;
epqstate,
resultRelationDesc,
resultRelInfo->ri_RangeTableIndex,
!
&update_ctid,
!
update_xmax);
if (!TupIsNull(epqslot))
{
! *tupleid = update_ctid;
slot =
ExecFilterJunk(resultRelInfo->ri_junkFilter, epqslot);
tuple =
ExecMaterializeSlot(slot);
goto lreplace;
--- 631,641 ----
epqstate,
resultRelationDesc,
resultRelInfo->ri_RangeTableIndex,
!
&(hufd.update_ctid),
!
hufd.update_xmax);
if (!TupIsNull(epqslot))
{
! *tupleid = hufd.update_ctid;
slot =
ExecFilterJunk(resultRelInfo->ri_junkFilter, epqslot);
tuple =
ExecMaterializeSlot(slot);
goto lreplace;
*** a/src/include/access/heapam.h
--- b/src/include/access/heapam.h
***************
*** 36,41 **** typedef enum
--- 36,50 ----
} LockTupleMode;
+ /* return values for heap_update and heap_delete */
+ typedef struct HeapUpdateFailureData
+ {
+ ItemPointerData update_ctid;
+ TransactionId update_xmax;
+ CommandId update_cmax;
+ } HeapUpdateFailureData;
+
+
/* ----------------
* function prototypes for heap access method
*
***************
*** 100,111 **** 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);
extern HTSU_Result heap_update(Relation relation, ItemPointer otid,
HeapTuple newtup,
! ItemPointer ctid, TransactionId *update_xmax,
! CommandId cid, Snapshot crosscheck, bool wait);
extern HTSU_Result heap_lock_tuple(Relation relation, HeapTuple tuple,
Buffer *buffer, ItemPointer ctid,
TransactionId *update_xmax, CommandId cid,
--- 109,120 ----
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,
! HeapUpdateFailureData *hufd, CommandId cid,
! Snapshot crosscheck, bool wait);
extern HTSU_Result heap_update(Relation relation, ItemPointer otid,
HeapTuple newtup,
! HeapUpdateFailureData *hufd, CommandId cid,
! Snapshot crosscheck, bool wait);
extern HTSU_Result heap_lock_tuple(Relation relation, HeapTuple tuple,
Buffer *buffer, ItemPointer ctid,
TransactionId *update_xmax, CommandId cid,
*** a/src/test/regress/expected/triggers.out
--- b/src/test/regress/expected/triggers.out
***************
*** 1443,1445 **** NOTICE: drop cascades to 2 other objects
--- 1443,1566 ----
DETAIL: drop cascades to view city_view
drop cascades to view european_city_view
DROP TABLE country_table;
+ --
+ -- Test updates to rows during firing of BEFORE ROW triggers.
+ --
+ create table parent (aid int not null primary key,
+ val1 text,
+ val2 text,
+ val3 text,
+ val4 text,
+ bcnt int not null default 0);
+ NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "parent_pkey"
for table "parent"
+ create table child (bid int not null primary key,
+ aid int not null,
+ val1 text);
+ NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "child_pkey"
for table "child"
+ 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;
+ ERROR: cannot update or delete a row from its BEFORE UPDATE trigger
+ HINT: Consider moving code to an AFTER UPDATE trigger.
+ 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;
+ ERROR: cannot update or delete a row from its BEFORE DELETE trigger
+ HINT: Consider moving code to an AFTER DELETE trigger.
+ 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)
+
+ 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;
+ 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;
*** a/src/test/regress/sql/triggers.sql
--- b/src/test/regress/sql/triggers.sql
***************
*** 961,963 **** SELECT * FROM city_view;
--- 961,1050 ----
DROP TABLE city_table CASCADE;
DROP TABLE country_table;
+
+ --
+ -- Test updates to rows during firing of BEFORE ROW triggers.
+ --
+
+ create table parent (aid int not null primary key,
+ val1 text,
+ val2 text,
+ val3 text,
+ val4 text,
+ bcnt int not null default 0);
+ create 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;
+ select * from parent; select * from child;
+ delete from parent where aid = 1;
+ select * from parent; select * from 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;
+ end if;
+ return old;
+ end;
+ $$;
+
+ delete from parent where aid = 1;
+ select * from parent; select * from child;
+
+ drop table parent, child;
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
