Re: TG_DEPTH patch v1

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
Views: Raw Message | Whole Thread | Download mbox | Resend email
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

In response to


Browse pgsql-hackers by date

  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?