"Kevin Grittner" <kevin.gritt...@wicourts.gov> wrote:
> By the way, my current patch does break two existing UPDATE
> statements in the regression test misc.sql file:
Fixed in the attached version of the patch.
I consider the trigger behavior addressed by this patch to be a bug,
and serious enough to merit inclusion of a fix in the 9.1 release,
even at this late stage. I don't think it should be back-patched,
because it changes behavior that there is some remote chance that
somebody somewhere understands and is intentionally using. While I
agree that Robert's suggested approach (or something functionally
equivalent) would be the ideal long-term solution, I think that it
would be too large of a change to consider for 9.1. I am suggesting
a minimal "defensive" change for 9.1, with possible development of a
fancier approach in a later release.
To recap: a trigger for UPDATE BEFORE EACH ROW or DELETE BEFORE EACH
ROW, which directly or indirectly causes update of the OLD row in
the trigger, can cause the triggering operation to be silently
ignored even though the trigger returns a non-NULL record, and even
though all triggered modifications are persisted. The only clue
that an incomplete and incongruous set of operations has been
performed is that the UPDATE or DELETE count from the statement is
reduced by the number of rows on which this occurred: if an UPDATE
would have affected 5 rows, but one of them just fired triggers *as
though* it had been updated but was actually left untouched by the
original UPDATE statement, you would get "UPDATE 4" rather than
"UPDATE 5".
This patch causes DELETE to behave as most people would expect, and
throws and ERROR on the conflicting UPDATE case.
So far, beyond my confirmation that it passes all PostgreSQL
regression tests and works as expected in a few ad hoc tests I've
done, there have been six full-time days of business analysts here
testing our applications with a version of 9.0.4 patched this way,
confirming that the application works as expected. They have a
standard set of testing scripts they use for every release, and
programmers have added tests specifically targeted at this area.
Plus the analysts are trying to exercise as many paths to data
modification as possible, to stress the triggers, including
interfaces with other agencies. They are scheduled to do a minimum
of 20 more full-time days of testing in the next two weeks, so if
someone wants to suggest changes or alternatives to this particular
patch, there would still be time to get over 100 hours of
professional testing against it -- if it comes through soon enough.
-Kevin
*** a/src/backend/executor/execMain.c
--- b/src/backend/executor/execMain.c
***************
*** 1847,1854 **** EvalPlanQualFetch(EState *estate, Relation relation, int
lockmode,
switch (test)
{
case HeapTupleSelfUpdated:
- /* treat it as deleted; do not process
*/
ReleaseBuffer(buffer);
return NULL;
case HeapTupleMayBeUpdated:
--- 1847,1862 ----
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
***************
*** 353,359 **** ldelete:;
--- 353,382 ----
true /* wait for
commit */ );
switch (result)
{
+ /*
+ * Don't allow updates to a row during its BEFORE
DELETE trigger
+ * to prevent the deletion. One example of where this
might
+ * happen is that the BEFORE DELETE trigger could
delete a child
+ * row, and a trigger on that child row might update
some count or
+ * status column in the row originally being deleted.
+ */
case HeapTupleSelfUpdated:
+ if (!ItemPointerEquals(tupleid, &update_ctid))
+ {
+ HeapTuple copyTuple;
+
+ estate->es_output_cid =
GetCurrentCommandId(false);
+ copyTuple = EvalPlanQualFetch(estate,
+
resultRelationDesc,
+
LockTupleExclusive,
+
&update_ctid,
+
update_xmax);
+ if (copyTuple != NULL)
+ {
+ *tupleid = update_ctid =
copyTuple->t_self;
+ goto ldelete;
+ }
+ }
/* already deleted by self; nothing to do */
return NULL;
***************
*** 570,576 **** lreplace:;
switch (result)
{
case HeapTupleSelfUpdated:
! /* already deleted by self; nothing to do */
return NULL;
case HeapTupleMayBeUpdated:
--- 593,617 ----
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.
! */
! if (!ItemPointerEquals(tupleid, &update_ctid)
! && GetCurrentCommandId(false) !=
estate->es_output_cid)
! {
! ereport(ERROR,
!
(errcode(ERRCODE_TRIGGERED_DATA_CHANGE_VIOLATION),
! errmsg("cannot update
a row from its BEFORE UPDATE trigger"),
! errhint("Consider
moving updates to the AFTER UPDATE trigger.")));
! }
! /* already deleted or updated by self; nothing
to do */
return NULL;
case HeapTupleMayBeUpdated:
*** a/src/test/regress/expected/triggers.out
--- b/src/test/regress/expected/triggers.out
***************
*** 1414,1416 **** NOTICE: drop cascades to 2 other objects
--- 1414,1511 ----
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 a row from its BEFORE UPDATE trigger
+ HINT: Consider moving updates to the 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;
+ 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
***************
*** 935,937 **** SELECT * FROM city_view;
--- 935,1008 ----
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;
+
+ drop table parent, child;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
