Tom Lane <[email protected]> wrote:
> I suggest that the current behavior was designed for the case of
> independent concurrent updates, and you have not made a good
> argument for changing that. What does make sense to me, in light
> of these examples, is to complain if a BEFORE trigger modifies the
> row "itself" (including indirect updates). IOW, at conclusion of
> trigger firing (I see no need to do it for each trigger), check to
> see if the target row has been outdated *by the current
> transaction*, and throw error if not.
>
> And, if you accept the statement of the fix like that, then
> actually there is no performance hit because there is no need to
> introduce new tests. All we have to do is start treating
> HeapTupleSelfUpdated result from heap_update or heap_delete as an
> error case instead of an okay-do-nothing case. There doesn't even
> need to be an explicit check that this was caused by a trigger,
> because AFAICS there isn't any other possibility.
I think that's pretty much what my previously posted patches did.
Here's a slightly modified one, based on Florian's feedback. Is
this what you had in mind, or am I misunderstanding?
-Kevin
*** a/src/backend/executor/nodeModifyTable.c
--- b/src/backend/executor/nodeModifyTable.c
***************
*** 354,359 **** ldelete:;
--- 354,379 ----
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.
+ */
+ if (!ItemPointerEquals(tupleid, &update_ctid)
+ && GetCurrentCommandId(false) !=
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;
***************
*** 570,575 **** lreplace:;
--- 590,613 ----
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
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;
*** 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
