On Jan29, 2011, at 00:15 , Kevin Grittner wrote:
> The people who write my paychecks have insisted on me chunking out
> some items which are part of our long-term plan besides the one I've
> been focusing on lately. Most of it isn't of interest to anyone
> outside Wisconsin Courts, but this piece might be; so I'm posting it
> and putting onto the first 9.2 CF. We'll be using it for
> development starting Monday and in production in two or three
> months, so it should be pretty well tested by July. :-)
Here is review of the patch.
The patch didn't apply cleanly anymore due to changes to the plpgsql
regression test. Merging the changes was trivial though. Updated
patch attached.
* Regression Tests
Since I had to merge them anyway, I started with looking at the
regression tests. If I'm reading them correctly, the second
'raise' in tg_depth_a_tf() is never executed. tg_depth_b_tf() ends
with an insert into tg_depth_c, which unconditionally throws
U9999. Thus, tg_depth_a_tf() never gets further than the insert
into tg_depth_b.
This effectively means that the test fails to verify that TG_DEPTH
is correctly reset after a non-exceptional return from a nested
trigger invokation.
* Implementation
Now to the implementation. The trigger depth is incremented before
calling the trigger function in ExecCallTriggerFunc() and
decremented right afterwards, which seems fine - apart from the
fact that the decrement is skipped in case of an error. The patch
handles that by saving respectively restoring the value of pg_depth in
PushTransaction() respectively {Commit,Abort}SubTransaction().
While I can't come up with a failure case for that approach, it seems
rather fragile - who's to say that nested trigger invocations correspond
that tightly to sub-transaction?
At the very least this needs a comment explaining why this is safe,
but I believe the same effect could be achieved more cleanly by
a TRY/CATCH block in ExecCallTriggerFunc().
* Other in-core PLs
As it stands, the patch only export tg_depth to plpgsql functions,
not to functions written in one of the other in-core PLs. It'd be
good to change that, I believe - otherwise the other PLs become
second-class citizens in the long run.
best regards,
Florian Pflug
