Re: TG_DEPTH patch v1

From: "Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov>
To: "Florian Pflug" <fgp(at)phlo(dot)org>
Cc: <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: TG_DEPTH patch v1
Date: 2012-01-13 16:00:32
Message-ID: 4F1000C00200002500044783@gw.wicourts.gov
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

[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

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Robert Haas 2012-01-13 16:08:16 Re: show Heap Fetches in EXPLAIN for index-only scans
Previous Message Tom Lane 2012-01-13 15:43:37 Re: Text comparison suddenly can't find collation?