TG_DEPTH patch v1

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. :-)

-Kevin

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

Florian Pflug <fgp(at)phlo(dot)org> wrote:

> Here is review of the patch.

Thanks for the review. I think I'd better try to keep the decks
clear for dealing with any SSI issues which may surface during the
coming month, so I'm going to mark this patch "Returned with
Feedback" and look at this for possible resubmission in light of
your review in a later CF.

On resubmit I will start with a reference to your review and proceed
with discussion from there.

Thanks,

-Kevin

[reviving discussion of another old patch]

In post:

http://archives.postgresql.org/pgsql-hackers/2011-06/msg00870.php

Florian Pflug <fgp(at)phlo(dot)org> wrote:

> Updated patch attached.

Thanks.

> 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?

Good question -- is it reasonable to assume that if an error is
thrown in a trigger, that the state restored when the error is
caught will be at the same trigger depth as when the transaction or
subtransaction started?

FWIW, the patch, using this technique, has been in production use
with about 3,000 OLTP users for about six months, with no apparent
problems. On the other hand, we don't do a lot with explicit
subtransactions.

> At the very least this needs a comment explaining why this is
> safe,

Agreed.

> but I believe the same effect could be achieved more cleanly by
> a TRY/CATCH block in ExecCallTriggerFunc().

That occurred to me, but I was concerned about the cost of setting
and clearing a longjump target for every trigger call. It seems
like I've seen comments about that being relatively expensive.
Tracking one more int in the current subtransaction structures is
pretty cheap, if that works.

> * 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.

Are you suggesting that this be implemented as a special trigger
variable in every PL, or that it simply be a regular function that
returns zero when not in a trigger and some positive value when
called from a trigger? The latter seems like a pretty good idea to
me. If that is done, is there any point to *also* having a TG_DEPTH
trigger variable in plpgsql? (I don't think there is.)

-Kevin