From: | Florian Pflug <fgp(at)phlo(dot)org> |
---|---|
To: | Kevin Grittner <Kevin(dot)Grittner(at)wicourts(dot)gov> |
Cc: | <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: TG_DEPTH patch v1 |
Date: | 2011-06-10 08:47:49 |
Message-ID: | 9E8B5A43-DDAA-43D5-AE43-F8071B4B81AB@phlo.org |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
Attachment | Content-Type | Size |
---|---|---|
tg_depth.v2.patch | application/octet-stream | 12.0 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Marti Raudsepp | 2011-06-10 08:50:24 | [BUG] Denormal float values break backup/restore |
Previous Message | Greg Smith | 2011-06-10 06:53:33 | Re: Creating new remote branch in git? |