Remembering bug #6123

Started by Kevin Grittnerabout 14 years ago39 messages
#1Kevin Grittner
Kevin.Grittner@wicourts.gov

Going back through the patches we had to make to 9.0 to move to
PostgreSQL triggers, I noticed that I let the issues raised as bug
#6123 lie untouched during the 9.2 development cycle. In my view,
the best suggestion for a solution was proposed by Florian here:

http://archives.postgresql.org/pgsql-hackers/2011-08/msg00388.php

As pointed out in a brief exchange after that post, there would be
ways to do the more exotic things that might be desired, while
preventing apparently straightforward code from doing surprising
things. I'm not sure whether that discussion fully satisfies the
concerns raised by Robert, though.

Because I let this lapse, it only seems feasible to go forward with
a patch for 9.2 if there is consensus around Florian's proposal. If
there is any dissent, I guess the thing to do is for me to gather
the issues and see about getting something into 9.3, once 9.2 work
has died down -- in five months or so. Wisconsin Courts can
continue to deal with the issues using my more simple-minded patch,
but others still are getting surprised by it -- bug #6226 is
apparently another manifestation.

Comments?

-Kevin

#2Tom Lane
tgl@sss.pgh.pa.us
In reply to: Kevin Grittner (#1)
Re: Remembering bug #6123

"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:

Going back through the patches we had to make to 9.0 to move to
PostgreSQL triggers, I noticed that I let the issues raised as bug
#6123 lie untouched during the 9.2 development cycle. In my view,
the best suggestion for a solution was proposed by Florian here:

http://archives.postgresql.org/pgsql-hackers/2011-08/msg00388.php

Do you mean this:

After every BEFORE trigger invocation, if the trigger returned
non-NULL, check if latest row version is still the same as when
the trigger started. If not, complain.

While that sounds relatively safe, if possibly performance-impacting,
it's not apparent to me how it fixes the problem you complained of.
The triggers you were using were modifying rows other than the one
being targeted by the triggering action, so a test like the above would
not notice that they'd done anything.

regards, tom lane

#3Kevin Grittner
Kevin.Grittner@wicourts.gov
In reply to: Tom Lane (#2)
Re: Remembering bug #6123

Tom Lane wrote:

"Kevin Grittner" writes:

Going back through the patches we had to make to 9.0 to move to
PostgreSQL triggers, I noticed that I let the issues raised as bug
#6123 lie untouched during the 9.2 development cycle. In my view,
the best suggestion for a solution was proposed by Florian here:

http://archives.postgresql.org/pgsql-hackers/2011-08/msg00388.php

Do you mean this:

After every BEFORE trigger invocation, if the trigger returned
non-NULL, check if latest row version is still the same as when
the trigger started. If not, complain.

That is the consice statement of it, yes.

While that sounds relatively safe, if possibly performance-
impacting, it's not apparent to me how it fixes the problem you
complained of. The triggers you were using were modifying rows
other than the one being targeted by the triggering action, so a
test like the above would not notice that they'd done anything.

My initial use-case was that a BEFORE DELETE trigger was deleting
related "child" rows, and the BEFORE DELETE trigger at the child
level was updating counts on the original (parent) row. The proposed
change would cause an error to be thrown when the parent level
returned a non-NULL value from its BEFORE DELETE trigger. That would
prevent the silent corruption of the data, so it's a big step forward
in my view; but it's not the behavior we most want in our shop for
this particular case. In the messages later in the thread, Florian
pointed out that this pattern would allow us to get the desired
behavior:

| BEFORE DELETE ON :
| DELETE FROM WHERE parent_id = OLD.id;
| IF FOUND THEN
| -- Removing children might have modified our row,
| -- so returning non-NULL is not an option
| DELETE FROM WHERE id = OLD.id;
| RETURN NULL;
| ELSE
| -- No children removed, so our row should be unmodified
| RETURN OLD;
| END IF;

The advantage of Florian's approach is that it changes the default
behavior to something very safe, while allowing arbitrarily complex
behavior through correspondingly more complex code.

-Kevin

#4Tom Lane
tgl@sss.pgh.pa.us
In reply to: Kevin Grittner (#3)
Re: Remembering bug #6123

"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:

Tom Lane wrote:

While that sounds relatively safe, if possibly performance-
impacting, it's not apparent to me how it fixes the problem you
complained of. The triggers you were using were modifying rows
other than the one being targeted by the triggering action, so a
test like the above would not notice that they'd done anything.

My initial use-case was that a BEFORE DELETE trigger was deleting
related "child" rows, and the BEFORE DELETE trigger at the child
level was updating counts on the original (parent) row. The proposed
change would cause an error to be thrown when the parent level
returned a non-NULL value from its BEFORE DELETE trigger.

Ah, I see.

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.

regards, tom lane

#5Kevin Grittner
Kevin.Grittner@wicourts.gov
In reply to: Tom Lane (#4)
1 attachment(s)
Re: Remembering bug #6123

Tom Lane <tgl@sss.pgh.pa.us> 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

Attachments:

bug6123-v3.patchtext/plain; name=bug6123-v3.patchDownload
*** 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;
#6Florian Pflug
fgp@phlo.org
In reply to: Kevin Grittner (#1)
Re: Remembering bug #6123

On Jan12, 2012, at 00:32 , Kevin Grittner wrote:

Going back through the patches we had to make to 9.0 to move to
PostgreSQL triggers, I noticed that I let the issues raised as bug
#6123 lie untouched during the 9.2 development cycle. In my view,
the best suggestion for a solution was proposed by Florian here:

http://archives.postgresql.org/pgsql-hackers/2011-08/msg00388.php

I've recently encountered a related issue, this time not related to
triggers but to FOR UPDATE.

My code declared a cursor which iterates over pairs of parent and
child rows, and updated the parent which values from the child. I
declared the cursor "FOR UPDATE OF parent", and used "WHERE CURRENT OF"
to update the parent row while iterating over the cursor. The code
looked something like this

DECLARE
c_parent_child CURSOR FOR
SELECT ... FROM parent JOIN child ON ...
FOR UPDATE OF parent;

BEGIN
FOR v_row IN c_parent_child LOOP
...
UPDATE parent SET ... WHERE CURRENT OF c_parent_child
END LOOP

I figured that was all safe and sound, since with the cursor's results
should be unaffected by the later UPDATES - after all, it's cmax is
smaller than any of the UPDATEs command ids. What I didn't take into
account, however, is that "FOR UPDATE OF" will silently skip rows which
have been updated (by the same transaction) after the cursor's snapshot
was established.

Thus, what happened was that things worked fine for parents with only
one child, but for parents with multiple children only the first child
got be processed. Once I realized that source of that problem, the fix was
easy - simply using the parent table's primary key to do the update, and
dropping the "FOR UPDATE OF" clause fixed the problem.

So, I guess my question is, if we add safeguards against these sorts of
bugs for triggers, should we also add them to FOR UPDATE? Historically,
we seem to have taken the stand that modifications of self-updated tuples
should be ignored. If we're going to reverse that decision (which I think
Kevin has argued for quite convincingly), it seems consistent to complain
about all modifications to self-updated tuples, not only to those involving
triggers.

OTOH, the more cases we complain, the larger the chance that we cause serious
issues for people who upgrade to 9.2. (or 9.3, whatever).

best regards,
Florian Pflug

#7Tom Lane
tgl@sss.pgh.pa.us
In reply to: Kevin Grittner (#5)
Re: Remembering bug #6123

"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:

Tom Lane <tgl@sss.pgh.pa.us> wrote:

... 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?

Actually, I was just about to follow up with a comment that that was too
simplistic: it would break the current behavior of join updates/deletes
that join to the same target row more than once. (There might be an
argument for restricting those, but this discussion isn't about that.)
So what we need to do is check whether the outdate was done by a later
CommandId than current. I see that your patch is attempting to deal
with these issues by testing GetCurrentCommandId(false) !=
estate->es_output_cid, but that seems completely wrong to me, as what it
does is complain if *any* additional command has been executed in the
transaction, regardless of what changed the target tuple. It ought to
be comparing the tuple's xmax to es_output_cid. And the comment needs
to cover why it's worrying about that.

Also, what's the point of testing update_ctid? I don't see that it
matters whether the outdate was a delete or an update.

regards, tom lane

#8Kevin Grittner
Kevin.Grittner@wicourts.gov
In reply to: Tom Lane (#7)
Re: Remembering bug #6123

Tom Lane <tgl@sss.pgh.pa.us> wrote:

So what we need to do is check whether the outdate was done by a
later CommandId than current. I see that your patch is attempting
to deal with these issues by testing GetCurrentCommandId(false) !=
estate->es_output_cid, but that seems completely wrong to me, as
what it does is complain if *any* additional command has been
executed in the transaction, regardless of what changed the target
tuple. It ought to be comparing the tuple's xmax to
es_output_cid. And the comment needs to cover why it's worrying
about that.

OK. I'll rework based on your comments.

Also, what's the point of testing update_ctid? I don't see that
it matters whether the outdate was a delete or an update.

The update_ctid code was a carry-over from my old, slightly
different approach, which I failed to change as I should have. I'll
fix that along with the other.

Thanks,

-Kevin

#9Tom Lane
tgl@sss.pgh.pa.us
In reply to: Florian Pflug (#6)
Re: Remembering bug #6123

Florian Pflug <fgp@phlo.org> writes:

So, I guess my question is, if we add safeguards against these sorts of
bugs for triggers, should we also add them to FOR UPDATE? Historically,
we seem to have taken the stand that modifications of self-updated tuples
should be ignored. If we're going to reverse that decision (which I think
Kevin has argued for quite convincingly), it seems consistent to complain
about all modifications to self-updated tuples, not only to those involving
triggers.

I'm not very convinced, except for the specific case of updates caused
by cascaded triggers.

regards, tom lane

#10Tom Lane
tgl@sss.pgh.pa.us
In reply to: Kevin Grittner (#8)
Re: Remembering bug #6123

"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:

Tom Lane <tgl@sss.pgh.pa.us> wrote:

Also, what's the point of testing update_ctid? I don't see that
it matters whether the outdate was a delete or an update.

The update_ctid code was a carry-over from my old, slightly
different approach, which I failed to change as I should have. I'll
fix that along with the other.

Actually, on reflection there might be a reason for checking
update_ctid, with a view to allowing "harmless" cases. I see
these cases:

* UPDATE finds a trigger already updated the row: must throw error
since we can't apply the update.

* UPDATE finds a trigger already deleted the row: arguably, we could
let the deletion stand and ignore the update action.

* DELETE finds a trigger already updated the row: must throw error
since we can't apply the delete.

* DELETE finds a trigger already deleted the row: arguably, there's
no reason to complain.

Don't know if that was your reasoning as well. But if it is, then again
the comment needs to cover that.

regards, tom lane

#11Tom Lane
tgl@sss.pgh.pa.us
In reply to: Kevin Grittner (#8)
Re: Remembering bug #6123

I wrote:

... It ought to be comparing the tuple's xmax to
es_output_cid.

Sigh, need to go find some caffeine. Obviously, it needs to check the
tuple's cmax, not xmax. And that means the patch will be a bit more
invasive than this, because heap_update and heap_delete don't return
that information at present.

regards, tom lane

#12Florian Pflug
fgp@phlo.org
In reply to: Tom Lane (#10)
Re: Remembering bug #6123

On Jan12, 2012, at 17:30 , Tom Lane wrote:

Actually, on reflection there might be a reason for checking
update_ctid, with a view to allowing "harmless" cases. I see
these cases:

* UPDATE finds a trigger already updated the row: must throw error
since we can't apply the update.

* UPDATE finds a trigger already deleted the row: arguably, we could
let the deletion stand and ignore the update action.

I've argued against that in the past, and I still think it's a bad idea.
The BEFORE UPDATE trigger might have done some actions which aren't valid
now that the row has been deleted. If it actually *is* safe to let a
self-delete take precent, the BEFORE UPDATE trigger ought to check whether
the row still exists, and return NULL if it doesn't.

* DELETE finds a trigger already updated the row: must throw error
since we can't apply the delete.

* DELETE finds a trigger already deleted the row: arguably, there's
no reason to complain.

I'm not convinced that is a a good idea, either. If we do that, there will
essentially be two BEFORE DELETE trigger invocations for a single deleted
row, which seems wrong. And again - if a BEFORE DELETE trigger *does* deal
with this case nicely, it simply has to check whether the row still exists
before returning non-NULL.

Also, without these exceptions, the behaviour (post-patch) is simply
explain by

Either don't cause recursive same-row updates from BEFORE trigger,
or have your trigger return NULL.

and except for the case of multiple BEFORE triggers on the same table
you can always assume that

A BEFORE trigger's view of a tuple modifications always reflects the
actual modification that will eventually happen (or an error is
thrown)

Adding a lists of "buts" and "ifs" to these rules has a higher chance of
adding confusion and causing bugs than to actually help people, IMHO.

Especially since (judged from the number of years the current behaviour
as stood undisputed) these cases seems to arise quite infrequently in
practice.

best regards,
Florian Pflug

#13Tom Lane
tgl@sss.pgh.pa.us
In reply to: Florian Pflug (#12)
Re: Remembering bug #6123

Florian Pflug <fgp@phlo.org> writes:

On Jan12, 2012, at 17:30 , Tom Lane wrote:

Actually, on reflection there might be a reason for checking
update_ctid, with a view to allowing "harmless" cases.

I've argued against that in the past, and I still think it's a bad idea.

Well, the main argument for it in my mind is that we could avoid
breaking existing behavior in cases where it's not clearly essential
to do so. Perhaps that's less compelling than keeping the behavior
simple, since it's true that the specific cases we could preserve the old
behavior in are pretty infrequent. I'm not wedded to the idea, but
would like to hear a few other peoples' opinions.

regards, tom lane

#14Kevin Grittner
Kevin.Grittner@wicourts.gov
In reply to: Tom Lane (#13)
Re: Remembering bug #6123

Tom Lane <tgl@sss.pgh.pa.us> wrote:

Florian Pflug <fgp@phlo.org> writes:

On Jan12, 2012, at 17:30 , Tom Lane wrote:

Actually, on reflection there might be a reason for checking
update_ctid, with a view to allowing "harmless" cases.

I've argued against that in the past, and I still think it's a
bad idea.

Well, the main argument for it in my mind is that we could avoid
breaking existing behavior in cases where it's not clearly
essential to do so. Perhaps that's less compelling than keeping
the behavior simple, since it's true that the specific cases we
could preserve the old behavior in are pretty infrequent. I'm not
wedded to the idea, but would like to hear a few other peoples'
opinions.

FWIW, I started from the position that the "harmless" cases should
be quietly handled. But Florian made what, to me, were persuasive
arguments against that. A DELETE trigger might fire twice for one
delete, mucking up data integrity, for example. His proposal seemed
to make my use case hard to handle, but then he pointed out how
triggers could be coded to allow that -- it's just that you would
need to go out of your way to do so, reducing the chance of falling
into bad behavior accidentally.

So count me as a convert to Florian's POV, even if I didn't succeed
in ripping out all the code from my former POV from the v3 patch.

-Kevin

#15Kevin Grittner
Kevin.Grittner@wicourts.gov
In reply to: Tom Lane (#11)
Re: Remembering bug #6123

Tom Lane <tgl@sss.pgh.pa.us> wrote:

it needs to check the tuple's cmax [...] And that means the patch
will be a bit more invasive than this, because heap_update and
heap_delete don't return that information at present.

I'm thinking that I could keep the test for:

GetCurrentCommandId(false) != estate->es_output_cid

as a "first pass". If that's true, I could use EvalPlanQualFetch()
to find the last version of the tuple, and generate the error if the
tuple's cmax != estate->es_output_cid. I think, although I'm not
entirely sure, that EvalPlanQualFetch needs a little work to support
this usage.

Attached is a patch based on these thoughts. Is it on the right
track? I suspect I haven't got everything covered, but thought a
reality check was in order at this point.

It does pass regression tests, including the new ones I added.

-Kevin

#16Kevin Grittner
Kevin.Grittner@wicourts.gov
In reply to: Kevin Grittner (#15)
1 attachment(s)
Re: Remembering bug #6123

"Kevin Grittner" <Kevin.Grittner@wicourts.gov> wrote:

Attached is a patch based on these thoughts.

OK, really attached this time.

-Kevin

Attachments:

bug6123-v4.patchtext/plain; name=bug6123-v4.patchDownload
*** 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
***************
*** 354,359 **** ldelete:;
--- 354,398 ----
  		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 it is, the
+ 				 * current command ID must have changed, so do that check
+ 				 * first, because it is fast.  If it has changed, chase down
+ 				 * to the last version of the row to see if it was changed by
+ 				 * a subsequent command.
+ 				 */
+ 				if (GetCurrentCommandId(false) != estate->es_output_cid)
+ 				{
+ 					HeapTuple	copyTuple;
+ 
+ 					copyTuple = EvalPlanQualFetch(estate,
+ 												  resultRelationDesc,
+ 												  LockTupleExclusive,
+ 												  &update_ctid,
+ 												  update_xmax);
+ 					if (copyTuple == NULL
+ 						|| HeapTupleHeaderGetCmax(copyTuple->t_data) != 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:;
--- 609,651 ----
  		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 it is, the
+ 				 * current command ID must have changed, so do that check
+ 				 * first, because it is fast.  If it has changed, chase down
+ 				 * to the last version of the row to see if it was changed by
+ 				 * a subsequent command.
+ 				 */
+ 				if (GetCurrentCommandId(false) != estate->es_output_cid)
+ 				{
+ 					HeapTuple	copyTuple;
+ 
+ 					copyTuple = EvalPlanQualFetch(estate,
+ 												  resultRelationDesc,
+ 												  LockTupleExclusive,
+ 												  &update_ctid,
+ 												  update_xmax);
+ 					if (copyTuple == NULL
+ 						|| HeapTupleHeaderGetCmax(copyTuple->t_data) != 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;
#17Tom Lane
tgl@sss.pgh.pa.us
In reply to: Kevin Grittner (#15)
Re: Remembering bug #6123

"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:

Tom Lane <tgl@sss.pgh.pa.us> wrote:

it needs to check the tuple's cmax [...] And that means the patch
will be a bit more invasive than this, because heap_update and
heap_delete don't return that information at present.

I'm thinking that I could keep the test for:

GetCurrentCommandId(false) != estate->es_output_cid

as a "first pass". If that's true, I could use EvalPlanQualFetch()
to find the last version of the tuple, and generate the error if the
tuple's cmax != estate->es_output_cid. I think, although I'm not
entirely sure, that EvalPlanQualFetch needs a little work to support
this usage.

Attached is a patch based on these thoughts. Is it on the right
track? I suspect I haven't got everything covered, but thought a
reality check was in order at this point.

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.

regards, tom lane

#18Kevin Grittner
Kevin.Grittner@wicourts.gov
In reply to: Tom Lane (#17)
Re: Remembering bug #6123

Tom Lane <tgl@sss.pgh.pa.us> 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.

OK.

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.

Working on v5.

-Kevin

#19Kevin Grittner
Kevin.Grittner@wicourts.gov
In reply to: Tom Lane (#17)
1 attachment(s)
Re: Remembering bug #6123

Tom Lane <tgl@sss.pgh.pa.us> 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

Attachments:

bug6123-v5.patchtext/plain; name=bug6123-v5.patchDownload
*** 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;
#20Tom Lane
tgl@sss.pgh.pa.us
In reply to: Kevin Grittner (#19)
Re: Remembering bug #6123

"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:

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?

Hmm, I would think you'd get assertion failures from calling
HeapTupleHeaderGetCmax when xmax isn't the current transaction.
(But I'm not sure that the regression tests really exercise such
cases ... did you try the isolation tests with this?) I was thinking
we should probably define the cmax as being returned only in SelfUpdated
cases.

You failed to update assorted relevant comments, too. But I can take
it from here.

regards, tom lane

#21Kevin Grittner
Kevin.Grittner@wicourts.gov
In reply to: Tom Lane (#20)
Re: Remembering bug #6123

Tom Lane <tgl@sss.pgh.pa.us> wrote:

"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:

Am I getting closer?

Hmm, I would think you'd get assertion failures from calling
HeapTupleHeaderGetCmax when xmax isn't the current transaction.
(But I'm not sure that the regression tests really exercise such
cases ... did you try the isolation tests with this?)

I didn't go farther than a `make check`, but I'm doing a more
thorough set of tests now.

I was thinking we should probably define the cmax as being
returned only in SelfUpdated cases.

I thought about that, but didn't see how it could be other than
self-updated. If you do, I guess I missed something.

You failed to update assorted relevant comments, too. But I can
take it from here.

I can take another pass to polish it if you'd like. I didn't expect
this to be the last version I posted; I was afraid I might still
have some restructuring to do.

-Kevin

#22Kevin Grittner
Kevin.Grittner@wicourts.gov
In reply to: Kevin Grittner (#21)
Re: Remembering bug #6123

"Kevin Grittner" <Kevin.Grittner@wicourts.gov> wrote:

I was thinking we should probably define the cmax as being
returned only in SelfUpdated cases.

I thought about that, but didn't see how it could be other than
self-updated. If you do, I guess I missed something.

Oh, I see it now. Oops.

I can fix that and the comments if you like.

-Kevin

#23Kevin Grittner
Kevin.Grittner@wicourts.gov
In reply to: Tom Lane (#20)
1 attachment(s)
Re: Remembering bug #6123

Tom Lane <tgl@sss.pgh.pa.us> wrote:

Hmm, I would think you'd get assertion failures from calling
HeapTupleHeaderGetCmax when xmax isn't the current transaction.
(But I'm not sure that the regression tests really exercise such
cases ... did you try the isolation tests with this?) I was
thinking we should probably define the cmax as being returned only
in SelfUpdated cases.

You were right. One of the isolation tests failed an assertion;
things pass with the attached change, setting the cmax
conditionally. Some comments updated. I hope this is helpful. I
can't take more time right now, because we're getting major snowfall
and I've got to leave now to get home safely.

-Kevin

Attachments:

bug6123-v6.patchtext/plain; name=bug6123-v6.patchDownload
*** a/src/backend/access/heap/heapam.c
--- b/src/backend/access/heap/heapam.c
***************
*** 2317,2324 **** 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
--- 2317,2323 ----
   *
   *	relation - table to be modified (caller must hold suitable lock)
   *	tid - TID of tuple to be deleted
!  *	hufd - output structure, 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
***************
*** 2330,2343 **** simple_heap_insert(Relation relation, HeapTuple tup)
   * (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.)
   */
  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();
--- 2329,2343 ----
   * (the last only possible if wait == false).
   *
   * In the failure cases, the routine returns the tuple's t_ctid and t_xmax.
+  * If HeapTupleSelfUpdated, it also returns the cmax from the tuple.
   * 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.)
   */
  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,2507 ----
  			   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);
! 		if (result == HeapTupleSelfUpdated)
! 			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:
--- 2633,2643 ----
  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:
***************
*** 2668,2675 **** 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
--- 2668,2674 ----
   *	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
!  *	hufd - output structure, 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
***************
*** 2687,2700 **** simple_heap_delete(Relation relation, ItemPointer tid)
   * 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.)
   */
  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();
--- 2686,2700 ----
   * data are not reflected into *newtup.
   *
   * In the failure cases, the routine returns the tuple's t_ctid and t_xmax.
+  * If HeapTupleSelfUpdated, it also returns the cmax from the tuple.
   * 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.)
   */
  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);
--- 2873,2882 ----
  			   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);
! 		if (result == HeapTupleSelfUpdated)
! 			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:
--- 3346,3356 ----
  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,53 ----
  } LockTupleMode;
  
  
+ /*
+  * return values for heap_update and heap_delete, filled only on failure;
+  * otherwise the contents are undefined.
+  */
+ typedef struct HeapUpdateFailureData
+ {
+ 	ItemPointerData		update_ctid;
+ 	TransactionId		update_xmax;
+ 	CommandId			update_cmax;	/* filled only for SelfUpdated */
+ }	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,
--- 112,123 ----
  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;
#24Tom Lane
tgl@sss.pgh.pa.us
In reply to: Kevin Grittner (#23)
Re: Remembering bug #6123

"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:

You were right. One of the isolation tests failed an assertion;
things pass with the attached change, setting the cmax
conditionally. Some comments updated. I hope this is helpful.

I worked over this patch a bit, mostly cosmetically; updated version
attached. However, we're not there yet. I have a couple of remaining
concerns:

1. I think the error message needs more thought. I believe it's
possible to trigger this error not only with BEFORE triggers, but also
with a volatile function used in the query that reaches around and
updates row(s) of the target table. Now, I don't have the slightest
qualms about breaking any apps that do anything so dirty, but should
we try to generalize the message text to cope with that?

2. The HINT needs work too, as it seems pretty useless as it stands.
I'd have expected some short reference to the technique of re-executing
the update in the trigger and then returning NULL. (BTW, does this
patch require any documentation changes, and if so where?)

3. I modified heap_lock_tuple to also return a HeapUpdateFailureData,
principally on the grounds that its API should be largely parallel to
heap_update, but having done that I can't help wondering whether its
callers are okay with skipping self-updated tuples. I see that you
changed EvalPlanQualFetch, but I'm not entirely sure that that is right;
shouldn't it continue to ignore rows modified by the current command
itself? And you did not touch the other two callers, which definitely
have got issues. Here is an example, which is basically the reference
count case refactored into a single self-referencing table, so that we
can hit both parent and child rows in one operation:

create table test (
id int primary key,
parent int references test,
data text,
nchildren int not null default 0
);

create function test_ins_func()
returns trigger language plpgsql as
$$
begin
if new.parent is not null then
update test set nchildren = nchildren + 1 where id = new.parent;
end if;
return new;
end;
$$;
create trigger test_ins_trig before insert on test
for each row execute procedure test_ins_func();

create function test_del_func()
returns trigger language plpgsql as
$$
begin
if old.parent is not null then
update test set nchildren = nchildren - 1 where id = old.parent;
end if;
return old;
end;
$$;
create trigger test_del_trig before delete on test
for each row execute procedure test_del_func();

insert into test values (1, null, 'root');
insert into test values (2, 1, 'root child A');
insert into test values (3, 1, 'root child B');
insert into test values (4, 2, 'grandchild 1');
insert into test values (5, 3, 'grandchild 2');

update test set data = 'root!' where id = 1;

select * from test;

delete from test;

select * from test;

drop table test;
drop function test_ins_func();
drop function test_del_func();

When you run this, with or without the current patch, you get:

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 4
id | parent | data | nchildren
----+--------+-------+-----------
1 | | root! | 0
(1 row)

the reason being that when the outer delete arrives at the last (root!)
row, GetTupleForTrigger fails because the tuple is already self-updated,
and we treat that as canceling the outer delete action.

I'm not sure what to do about this. If we throw an error there, there
will be no way that the trigger can override the error because it will
never get to run. Possibly we could plow ahead with the expectation
of throwing an error later if the trigger doesn't cancel the
update/delete, but is it safe to do so if we don't hold lock on the
tuple? In any case that idea doesn't help with the remaining caller,
ExecLockRows.

regards, tom lane

#25Kevin Grittner
Kevin.Grittner@wicourts.gov
In reply to: Tom Lane (#24)
Re: Remembering bug #6123

Tom Lane <tgl@sss.pgh.pa.us> wrote:

I worked over this patch a bit, mostly cosmetically; updated
version attached.

Thanks!

However, we're not there yet. I have a couple of remaining
concerns:

1. I think the error message needs more thought. I believe it's
possible to trigger this error not only with BEFORE triggers, but
also with a volatile function used in the query that reaches
around and updates row(s) of the target table. Now, I don't have
the slightest qualms about breaking any apps that do anything so
dirty, but should we try to generalize the message text to cope
with that?

I hadn't thought of that, but it does seem possible. Maybe after
dealing with the other points, I'll work on a test case to show that
behavior.

I'm also fine with generating an error for such dirty tricks, and I
agree that if that's indeed possible we should make the message
general enough to cover that case. Nothing comes to mind at the
moment, but I'll think on it.

2. The HINT needs work too, as it seems pretty useless as it
stands. I'd have expected some short reference to the technique
of re-executing the update in the trigger and then returning NULL.

In the previous (rather long) thread on the topic, it seemed that
most cases where people have hit this, the problem was easily solved
by moving the offending code to the AFTER trigger. The technique of
re-issuing the command didn't turn up until much later. I would bet
it will be the right thing to do 20% of the time when people get
such an error. I don't want to leave the 80% solution out of the
hint, but if you don't think it's too verbose, I guess it would be a
good thing to mention the 20% solution, too.

(BTW, does this patch require any documentation changes, and if so
where?)

I've been wondering that. Perhaps a paragraph or two with an
example on this page?:

http://www.postgresql.org/docs/devel/static/trigger-definition.html

3. I modified heap_lock_tuple to also return a
HeapUpdateFailureData, principally on the grounds that its API
should be largely parallel to heap_update, but having done that I
can't help wondering whether its callers are okay with skipping
self-updated tuples. I see that you changed EvalPlanQualFetch,
but I'm not entirely sure that that is right; shouldn't it
continue to ignore rows modified by the current command itself?

I made that change when working on the approach where "safe"
conflicts (like a DELETE from within the BEFORE DELETE trigger) were
quietly ignored. Without that change, it didn't see the end of the
ctid chain, and didn't behave correctly. I've been wondering if it
is still needed. I had been planning to create a test case to try
to show that it was. Maybe an UPDATE with a join to force multiple
row updates from the "primary" statement, followed by a triggered
UPDATE to the row. If that doesn't need the EvalPlanQualFetch
change, I don't know what would.

And you did not touch the other two callers, which definitely
have got issues. Here is an example

[example]

I'm not sure what to do about this. If we throw an error there,
there will be no way that the trigger can override the error
because it will never get to run. Possibly we could plow ahead
with the expectation of throwing an error later if the trigger
doesn't cancel the update/delete, but is it safe to do so if we
don't hold lock on the tuple? In any case that idea doesn't help
with the remaining caller, ExecLockRows.

It will take me some time to work though the example and review the
relevant code.

-Kevin

#26Tom Lane
tgl@sss.pgh.pa.us
In reply to: Kevin Grittner (#25)
Re: Remembering bug #6123

"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:

Tom Lane <tgl@sss.pgh.pa.us> wrote:

3. I modified heap_lock_tuple to also return a
HeapUpdateFailureData, principally on the grounds that its API
should be largely parallel to heap_update, but having done that I
can't help wondering whether its callers are okay with skipping
self-updated tuples. I see that you changed EvalPlanQualFetch,
but I'm not entirely sure that that is right; shouldn't it
continue to ignore rows modified by the current command itself?

I made that change when working on the approach where "safe"
conflicts (like a DELETE from within the BEFORE DELETE trigger) were
quietly ignored. Without that change, it didn't see the end of the
ctid chain, and didn't behave correctly. I've been wondering if it
is still needed. I had been planning to create a test case to try
to show that it was. Maybe an UPDATE with a join to force multiple
row updates from the "primary" statement, followed by a triggered
UPDATE to the row. If that doesn't need the EvalPlanQualFetch
change, I don't know what would.

The EvalPlanQual code isn't really exercised by any existing regression
tests, because it is only triggered when two sessions concurrently
update the same row, something that we avoid for reproducibility's sake
in the regression tests. I think we could now test it using the
isolation test scaffolding, and maybe it would be a good idea to add
some tests there. I did verify that whether that part of your patch
is included or not makes no difference to any existing test.

regards, tom lane

#27Kevin Grittner
Kevin.Grittner@wicourts.gov
In reply to: Tom Lane (#24)
Re: Remembering bug #6123

Tom Lane <tgl@sss.pgh.pa.us> wrote:

[rearranging so results directly follow statements]

select * from test;
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 test;
DELETE 4

select * from test;
id | parent | data | nchildren
----+--------+-------+-----------
1 | | root! | 0
(1 row)

And other minor updates to the data column can result in totally
different sets of rows left over after a DELETE from the table with
no WHERE clause. It makes me pretty queasy whenever the semantics
of a statement can depend on the order of tuples in the heap. It's
pretty hard to view this particular case as anything other than a
bug.

the reason being that when the outer delete arrives at the last
(root!) row, GetTupleForTrigger fails because the tuple is already
self-updated, and we treat that as canceling the outer delete
action.

I'm not sure what to do about this. If we throw an error there,
there will be no way that the trigger can override the error
because it will never get to run. Possibly we could plow ahead
with the expectation of throwing an error later if the trigger
doesn't cancel the update/delete, but is it safe to do so if we
don't hold lock on the tuple? In any case that idea doesn't help
with the remaining caller, ExecLockRows.

I'm still trying to sort through what could be done at the source
code level, but from a user level I would much rather see an error
than such surprising and unpredictable behavior.

-Kevin

#28Tom Lane
tgl@sss.pgh.pa.us
In reply to: Kevin Grittner (#27)
Re: Remembering bug #6123

"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:

Tom Lane <tgl@sss.pgh.pa.us> wrote:

I'm not sure what to do about this. If we throw an error there,
there will be no way that the trigger can override the error
because it will never get to run. Possibly we could plow ahead
with the expectation of throwing an error later if the trigger
doesn't cancel the update/delete, but is it safe to do so if we
don't hold lock on the tuple? In any case that idea doesn't help
with the remaining caller, ExecLockRows.

I'm still trying to sort through what could be done at the source
code level, but from a user level I would much rather see an error
than such surprising and unpredictable behavior.

I don't object to throwing an error by default. What I'm wondering is
what would have to be done to make such updates work safely. AFAICT,
throwing an error in GetTupleForTrigger would preclude any chance of
coding a trigger to make this work, which IMO greatly weakens the
argument that this whole approach is acceptable.

In this particular example, I think it would work just as well to do the
reference-count updates in AFTER triggers, and maybe the short answer
is to tell people they have to do it like that instead of in BEFORE
triggers. However, I wonder what use-case led you to file bug #6123 to
begin with.

regards, tom lane

#29Tom Lane
tgl@sss.pgh.pa.us
In reply to: Kevin Grittner (#25)
Re: Remembering bug #6123

"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:

I'm also fine with generating an error for such dirty tricks, and I
agree that if that's indeed possible we should make the message
general enough to cover that case. Nothing comes to mind at the
moment, but I'll think on it.

What do you think of

ERROR: tuple to be updated was already modified by an operation triggered by the UPDATE command
HINT: Consider using an AFTER trigger instead of a BEFORE trigger to propagate changes to other rows.

(s/update/delete/ for the DELETE case of course)

The phrase "triggered by" seems slippery enough to cover cases such as a
volatile function executed by the UPDATE. The HINT doesn't cover that
case of course, but we have a ground rule that HINTs can be wrong.

regards, tom lane

#30Kevin Grittner
Kevin.Grittner@wicourts.gov
In reply to: Tom Lane (#29)
Re: Remembering bug #6123

Tom Lane <tgl@sss.pgh.pa.us> wrote:

What do you think of

ERROR: tuple to be updated was already modified by an operation
triggered by the UPDATE command
HINT: Consider using an AFTER trigger instead of a BEFORE trigger
to propagate changes to other rows.

(s/update/delete/ for the DELETE case of course)

The phrase "triggered by" seems slippery enough to cover cases
such as a volatile function executed by the UPDATE. The HINT
doesn't cover that case of course, but we have a ground rule that
HINTs can be wrong.

Looks good to me.

-Kevin

#31Kevin Grittner
Kevin.Grittner@wicourts.gov
In reply to: Tom Lane (#28)
Re: Remembering bug #6123

Tom Lane <tgl@sss.pgh.pa.us> wrote:

In this particular example, I think it would work just as well to
do the reference-count updates in AFTER triggers, and maybe the
short answer is to tell people they have to do it like that
instead of in BEFORE triggers.

I think that is quite often the right answer.

However, I wonder what use-case led you to file bug #6123 to begin
with.

In our Circuit Court software, we have about 1600 trigger functions
on about 400 tables, and this summer we converted them from our Java
middle tier framework to native PostgreSQL triggers. Since we had
been writing them in our interpretation of ANSI SQL trigger code,
parsing that, and using the parse tree to build a Java class for
each trigger, we were able to generate the PostgreSQL trigger
functions and CREATE TRIGGER statement mechanically (from the parse
tree), with pretty good success. In testing, though, our business
analysts noticed a number of situations where an attempt to delete a
row actually deleted related rows which should have gone away with
the row they were directly trying to delete, but the target row was
still there. A few days of investigation, including stepping
through query execution in gdb, turned up this issue.

Having identified (at least one flavor of) the problem, we grepped
the source code for the BEFORE triggers for UPDATE and DELETE
statements, and were able to fix a number of them by moving code to
AFTER triggers or setting values into NEW fields rather than running
an UPDATE. So far, so good.

But there were a number of situations where the DELETE of a row
needed to cause related rows in other tables to be deleted, and for
one reason or another a foreign key with ON DELETE CASCADE was not
an option. At the same time, triggers on some of those related
tables needed to update summary or redundant data in other tables
for performance reasons. Because a number of tables could be
involved, and some of the triggers (at the "lower" levels) could be
AFTER triggers and still contribute to the problem, (1) we had no
reliable way to ensure we would find all of the cases of this on all
code paths, and (2) due to referential integrity and other trigger-
based validations, it would be hard to restructure such that the
DELETE of the "child" rows was not done on the BEFORE DELETE trigger
of the "parent".

The patch we've been using in production throws errors if the row
for a BEFORE UPDATE trigger is updated by another statement. (Well,
OK, you showed me that it really is throwing an error if the row is
updated and there has been another statement executed, but as long
as it is *more* strict than we actually need, we won't corrupt data
-- and the current rule hasn't been hard for us to live with.) It
allows the DELETE to proceed if the tuple is updated from within the
BEFORE DELETE trigger. We would need to tweak some triggers to move
to the approach embodied in the recent patch drafts, but the IF
FOUND logic suggested by Florian looks like it will cover all of our
use cases, and they should be fairly easy to find with grep.

Hopefully this answers your question. I went looking for details on
particular failures here, but didn't have luck with so far. I can
try again if more detail like that would help.

-Kevin

#32Tom Lane
tgl@sss.pgh.pa.us
In reply to: Kevin Grittner (#31)
Re: Remembering bug #6123

"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:

Tom Lane <tgl@sss.pgh.pa.us> wrote:

However, I wonder what use-case led you to file bug #6123 to begin
with.

[ details ]

Hopefully this answers your question. I went looking for details on
particular failures here, but didn't have luck with so far. I can
try again if more detail like that would help.

Well, the bottom line that's concerning me here is whether throwing
errors is going to push anyone's application into an unfixable corner.
I'm somewhat encouraged that your Circuit Courts software can adapt
to it, since that's certainly one of the larger and more complex
applications out there. Or at least I would be if you had actually
verified that the CC code was okay with the recently-proposed patch
versions. Do you have any thorough tests you can run against whatever
we end up with?

regards, tom lane

#33Kevin Grittner
Kevin.Grittner@wicourts.gov
In reply to: Tom Lane (#32)
Re: Remembering bug #6123

Tom Lane wrote:

Well, the bottom line that's concerning me here is whether throwing
errors is going to push anyone's application into an unfixable
corner. I'm somewhat encouraged that your Circuit Courts software
can adapt to it, since that's certainly one of the larger and more
complex applications out there. Or at least I would be if you had
actually verified that the CC code was okay with the recently-
proposed patch versions. Do you have any thorough tests you can run
against whatever we end up with?

In spite of several attempts over the years to come up with automated
tests of our applications at a level that would show these issues,
we're still dependent on business analysts to run through a standard
list of tests for each release, plus tests designed to exercise code
modified for the release under test. For the release where we went
to PostgreSQL triggers, that included running lists against the
statistics tables to see which trigger functions had not yet been
exercised in testing, until we had everything covered.

To test the new version of this patch, we would need to pick an
application release, and use the patch through the development,
testing, and staging cycles, We would need to look for all triggers
needing adjustment, and make the necessary changes. We would need to
figure out which triggers were important to cover, and ensure that
testing covered all of them.

Given the discussions with my new manager this past week, I'm pretty
sure we can work this into a release that would complete testing and
hit pilot deployment in something like three months, give or take a
little. I can't actually make any promises on that until I talk to
her next week.

From working through all the BEFORE triggers with UPDATE or DELETE

statements this summer, I'm pretty confident that the ones which
remain can be handled by reissuing the DELETE (we didn't keep any of
the UPDATEs with this pattern) and returning NULL. The most
complicated and troublesome trigger code has to do with purging old
data. I suspect that if we include testing of all purge processes in
that release cycle, we'll shake out just about any issues we have.

-Kevin

#34Kevin Grittner
Kevin.Grittner@wicourts.gov
In reply to: Kevin Grittner (#33)
Re: Remembering bug #6123

"Kevin Grittner" <Kevin.Grittner@wicourts.gov> wrote:

Tom Lane wrote:

Well, the bottom line that's concerning me here is whether
throwing errors is going to push anyone's application into an
unfixable corner. I'm somewhat encouraged that your Circuit
Courts software can adapt to it, since that's certainly one of
the larger and more complex applications out there. Or at least I
would be if you had actually verified that the CC code was okay
with the recently-proposed patch versions. Do you have any
thorough tests you can run against whatever we end up with?

To test the new version of this patch, we would need to pick an
application release, and use the patch through the development,
testing, and staging cycles, We would need to look for all
triggers needing adjustment, and make the necessary changes. We
would need to figure out which triggers were important to cover,
and ensure that testing covered all of them.

Given the discussions with my new manager this past week, I'm
pretty sure we can work this into a release that would complete
testing and hit pilot deployment in something like three months,
give or take a little. I can't actually make any promises on that
until I talk to her next week.

After a couple meetings, I have approval to get this into an
application release currently in development. Assuming that your
patch from the 13th is good for doing the testing, I think I can
post test results in about three weeks. I'll also work on a
follow-on patch to add couple paragraphs and an example of the issue
to the docs by then.

-Kevin

#35Tom Lane
tgl@sss.pgh.pa.us
In reply to: Kevin Grittner (#34)
Re: Remembering bug #6123

"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:

After a couple meetings, I have approval to get this into an
application release currently in development. Assuming that your
patch from the 13th is good for doing the testing, I think I can
post test results in about three weeks. I'll also work on a
follow-on patch to add couple paragraphs and an example of the issue
to the docs by then.

Well, the issues about wording of the error message are obviously just
cosmetic, but I think we'd better do whatever we intend to do to the
callers of heap_lock_tuple before putting the patch through testing.
I'm inclined to go ahead and make them throw errors, just to see if
that has any effects we don't like.

I'm up to my elbows in planner guts at the moment, but will try to
fix up the patch this weekend if you want.

regards, tom lane

#36Kevin Grittner
Kevin.Grittner@wicourts.gov
In reply to: Tom Lane (#35)
Re: Remembering bug #6123

Tom Lane <tgl@sss.pgh.pa.us> wrote:

I'm up to my elbows in planner guts at the moment, but will try to
fix up the patch this weekend if you want.

They have scheduled testers to check on this issue next week, so it
would be great to get as close as we can on the stuff that matters.

-Kevin

#37Tom Lane
tgl@sss.pgh.pa.us
In reply to: Kevin Grittner (#36)
Re: Remembering bug #6123

OK, here's an updated version of the patch. I changed the error message
texts as per discussion (except I decided to use one message string for
all the cases rather than saddle translators with several variants).
Also, I put in an error in GetTupleForTrigger, which fixes the
self-reference case I illustrated before (now added to the regression
test). However, I found out that changing the other two callers of
heap_lock_tuple would open an entirely new can of worms, so for now
they still have the historical behavior of ignoring self-updated tuples.

The problem with changing ExecLockRows or EvalPlanQualFetch can be
illustrated by the regression test case it breaks, which basically is

BEGIN;
DECLARE c1 CURSOR FOR SELECT * FROM table FOR UPDATE;
UPDATE table SET ...;
FETCH ALL FROM c1;
COMMIT;

When the FETCH comes to a row that's been updated by the UPDATE, it sees
that row as HeapTupleSelfUpdated with a cmax greater than es_output_cid
(which is the CID assigned to the DECLARE). So if we make these callers
throw an error for the case, coding like the above will fail, which
seems to me to be pretty darn hard to justify. It is not a corner case
that could be caused only by questionable use of trigger side effects.
So that seems to leave us with two choices: (1) ignore the row, or
(2) attempt to lock the latest version instead of the visible version.
(1) is our historical behavior but seems arguably wrong. I tried to
make the patch do (2) but it crashed and burned because heap_lock_tuple
spits up if asked to lock an "invisible" row. We could possibly finesse
that by having EvalPlanQualFetch sometimes pass a CID later than
es_output_cid to heap_lock_tuple, but it seems ticklish. More, I think
it would also take some adjustments to the behavior of
HeapTupleSatisfiesDirty, else we'll not see such tuples in the first
place. So this looks messy, and also rather orthogonal to the current
goals of the patch.

Also, I'm not sure that your testing would exercise such cases at all,
as you have to be using SELECT FOR UPDATE and/or READ COMMITTED mode to
get to any of the relevant code. I gather your software mostly relies
on SERIALIZABLE mode to avoid such issues. So I stopped with this.

regards, tom lane

#38Simon Riggs
simon@2ndQuadrant.com
In reply to: Tom Lane (#10)
Re: Remembering bug #6123

On Thu, Jan 12, 2012 at 4:30 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:

Tom Lane <tgl@sss.pgh.pa.us> wrote:

Also, what's the point of testing update_ctid?  I don't see that
it matters whether the outdate was a delete or an update.

The update_ctid code was a carry-over from my old, slightly
different approach, which I failed to change as I should have.  I'll
fix that along with the other.

Actually, on reflection there might be a reason for checking
update_ctid, with a view to allowing "harmless" cases.  I see
these cases:

* UPDATE finds a trigger already updated the row: must throw error
since we can't apply the update.

* UPDATE finds a trigger already deleted the row: arguably, we could
let the deletion stand and ignore the update action.

* DELETE finds a trigger already updated the row: must throw error
since we can't apply the delete.

* DELETE finds a trigger already deleted the row: arguably, there's
no reason to complain.

Don't know if that was your reasoning as well.  But if it is, then again
the comment needs to cover that.

Just been reading this thread a little.

ISTM that seeing a SelfUpdated row on a cursor when we didn't use
WHERE CURRENT OF really ought to raise some kind of exception
condition like a WARNING or a NOTICE. That situation seems most likely
to be a bug or at least an unintended consequence.

As Tom points out, the multiple flavours of weirdness that can result
even if we do fix this are not things we should do silently. I think
we should do the best job we can without throwing an error, but we
must make some attempt to let the developer know we did that for them
so they can investigate.

--
 Simon Riggs                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

#39Kevin Grittner
Kevin.Grittner@wicourts.gov
In reply to: Tom Lane (#37)
Re: Remembering bug #6123

Tom Lane <tgl@sss.pgh.pa.us> wrote:

http://archives.postgresql.org/pgsql-hackers/2012-01/msg01241.php

OK, here's an updated version of the patch.

I was on vacation after PGCon and just got back to the office
yesterday, so I have just now been able to check on the status of
our testing of this after being asked about it by Tom at PGCon.
He has this listed as an open bug, with testing of his fix by my
organization as the hold-up.

There was some testing of this in January while I was on another
vacation, some triggers were found which worked as intended with the
patch I had hacked together, but which got errors with the stricter
(and safer) patch created by Tom. I pointed out to the developers
some triggers which needed to be changed, and what should be
considered safe coding techniques, but I was then assigned to
several urgent issues and lost track of this one. I have arranged
for testing over the next few days. I will post again with results
when I have them.

-Kevin