Skip site navigation (1) Skip section navigation (2)

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
Message-ID: 9E8B5A43-DDAA-43D5-AE43-F8071B4B81AB@phlo.org (view raw or flat)
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: tg_depth.v2.patch
Description: application/octet-stream (12.0 KB)

In response to

Responses

pgsql-hackers by date

Next:From: Marti RaudseppDate: 2011-06-10 08:50:24
Subject: [BUG] Denormal float values break backup/restore
Previous:From: Greg SmithDate: 2011-06-10 06:53:33
Subject: Re: Creating new remote branch in git?

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group