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