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

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)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

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

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

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

Florian Pflug <fgp(at)phlo(dot)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

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Florian Pflug <fgp(at)phlo(dot)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

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:

> Attached is a patch based on these thoughts.

OK, really attached this time.

-Kevin

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)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

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)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

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)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

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)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

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)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

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

On Thu, Jan 12, 2012 at 4:30 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
>> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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