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

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

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

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

"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

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

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

"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

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@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

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

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

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

Attached is a patch based on these thoughts.

OK, really attached this time.

-Kevin

"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

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

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

"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