Re: Remembering bug #6123

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

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

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

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