Re: WIP fix proposal for bug #6123 - Mailing list pgsql-hackers

Florian Pflug <fgp@phlo.org> wrote:
> On Aug2, 2011, at 17:03 , Kevin Grittner wrote:
> Hm, OK I see your point now I believe. This is not so much about
> wanting to put things into BEFORe triggers which doesn't really
> fit there, but instead avoiding weeks of debugging if someones
> messes up.
I would say that is the overriding concern.  But there are arguments
for sometimes putting some things in the BEFORE triggers which could
raise the issue, too.  For us, there would be a lot to dance around
if deletes cascade in the AFTER triggers, since we'd have orphans
hanging around during some validations, as described below.
>> In my case it is mainly an issue in deletes (and the special case
>> of a "purge" process where deletes are not normally allowed)
>> doing a depth-first search for dependent records, and deleting
>> from the bottom up.  In some cases there is redundant information
>> in higher level tables based on primary data in lower level
>> tables -- in the form of counts, sums, or status codes.  If you
>> delete a case, and that "cascades" to a event history table which
>> has a trigger which updates case status according to certain
>> business rules, the delete of the case is canceled because of the
>> delete of a status-changing event.  That's very bad.  If we at
>> least threw an error instead, we could identify the problem
>> reliably, and code around it somehow.  That might be by moving
>> the delete of dependent records to a point after the parent
>> record has already been deleted, but that runs the risk of
>> triggering other validation failures based on business rules in
>> the triggers, because the triggers would then be seeing a state
>> we try very hard to prevent.
> 
> If we go with my suggestion below (which entails throwing the
> error earlier at the time of the offending update or delete), you
> could get the non-throwing behaviour you want by protecting
> UPDATES and DELETES which might touch rows which are already in
> the process of being updated or deleted by EXCEPTION blocks.
>> Would you feel comfortable with a patch which threw an error on
>> the DELETE case, as it does on the UPDATE case?
> 
> Yes, though with a little additional twist. The twist being that
> I'd like the error to be thrown earlier, at the time of the
> offending UPDATE or DELETE, not later, when the original UPDATE or
> DELETE which caused the BEFORE trigger invocation stumbles over
> the updated row. It not only seems more logical that way, but it
> also makes it possible for the trigger to catch the error, and
> react accordingly.
I hadn't thought of that.  It does seem better in every way except
for how much work it takes to do it and how much testing it needs to
have confidence in it.  Definitely not 9.1 material.
That's OK -- we have something which should work for us in 9.0 and
9.1.  I'd really prefer not to "fork" this permanently, but if
there's a better way coming in 9.2, we can use our patch for a year
or so and then switch over to the superior capabilities available in
9.2.
> For example, if a BEFORE trigger on table A modifies table B, and
> one of B's BEFORE triggers in turn modifies A, B's BEFORE trigger
> could then catch the error and e.g. decide to skip the UPDATE.
Yeah, that provides a reasonable default but still gives the
application programmer fine-grained control on this.  That's a good
thing.
> Implementation-wise, I image we'd set a flag in the tuple header 
> after locking the tuple (as I now discovered we do, sorry again)
> but before firing BEFORE triggers. We'd clear the flag again
> once all BEFORE triggers have run, and let the actual UPDATE
> or DELETE proceed. If an UPDATE or DELETE encounters a flagged
> tuple, and the transaction (or one of its parents) is the current
> lock holder, we'd throw an error. To clean up after aborted
> transactions, we'd clear the flag upon acquiring a tuple lock.
> 
> Instead of a flag in the tuple header, we could also keep
> a (backend-local) list of ctids for which we've fired BEFORE
> triggers but haven't done the actual modification yet.
> 
> We might also want to make it possible to distinguish pending
> UPDATES from pending DELETES (i.e., choose a different error code
> for the two cases), because it seems reasonable that code would
> want to react differently in those cases (e.g., skip an UPDATE if
> there's a pending DELETE).
Does anyone else want to weigh in on this overall approach or narrow
down the alternatives Florian has sketched out above?  If we can
reach some consensus on an approach, I can work on a new patch to
implement it.
-Kevin