Per-function GUC settings: trickier than it looked

So I coded up a patch for this, based on the idea of creating a
quasi-subtransaction that affects only GUC while entering/exiting a
function that has GUC settings attached. The specified settings are
applied as if by SET LOCAL before starting function execution, and then
they drop out during "subtransaction" exit. (I'll post the code to
pgsql-patches in a moment.)

But on reflection I realize that there are some interesting properties
to this approach:

* if you do "SET LOCAL foo" when you are in a function that has a
"SET foo" property, the setting disappears at function exit. But if
you do "SET foo" it persists. This might be OK, but it seems a bit odd.

* in fact, if you do "SET LOCAL foo" when you are in a function that has
any "SET" property at all, the setting disappears at function exit,
whether foo was one of the variables SET by the function definition or
not.

We could perhaps get away with defining that as being the behavior,
but it doubtless will surprise someone sometime. What *should* these
interactions be like, and has anyone got an idea how to implement their
suggestion?

regards, tom lane

Tom Lane wrote:
> So I coded up a patch for this, based on the idea of creating a
> quasi-subtransaction that affects only GUC while entering/exiting a
> function that has GUC settings attached. The specified settings are
> applied as if by SET LOCAL before starting function execution, and then
> they drop out during "subtransaction" exit. (I'll post the code to
> pgsql-patches in a moment.)
>
> But on reflection I realize that there are some interesting properties
> to this approach:
>
> * if you do "SET LOCAL foo" when you are in a function that has a
> "SET foo" property, the setting disappears at function exit. But if
> you do "SET foo" it persists. This might be OK, but it seems a bit odd.
That seems OK - the same happens inside a BEGIN/EXCEPTION/END block, no?

> * in fact, if you do "SET LOCAL foo" when you are in a function that has
> any "SET" property at all, the setting disappears at function exit,
> whether foo was one of the variables SET by the function definition or
> not.
Hm... That is a bit surprising... Maybe all functions should create a
such GUC-only substransaction-like thing. That might create problems
for inlining - but only if you can actually change GUCs from plsql
function, which maybe you cant...

> We could perhaps get away with defining that as being the behavior,
> but it doubtless will surprise someone sometime. What *should* these
> interactions be like, and has anyone got an idea how to implement their
> suggestion?
What will happen if you have two functions, foo and bar, were the search-path
is overridden for foo, and foo calls bar. I guess bar would be executed with
foo's overridden searchpath. Thats seems a bit surprising - I'd make more
sense to me if bar would use the session's search-path, but that seems hard
to do... Especially because bar *should* use foo's searchpath if foo contained
an explicit "SET LOCAL search_path"

Or maybe I'm just crazy, and the current behavior is fine....

greetings, Florian Pflug

"Florian G. Pflug" <fgp(at)phlo(dot)org> writes:
> Tom Lane wrote:
>> We could perhaps get away with defining that as being the behavior,
>> but it doubtless will surprise someone sometime. What *should* these
>> interactions be like, and has anyone got an idea how to implement their
>> suggestion?

> What will happen if you have two functions, foo and bar, were the search-path
> is overridden for foo, and foo calls bar. I guess bar would be executed with
> foo's overridden searchpath. Thats seems a bit surprising -

I think it's correct; if bar doesn't SET a search_path then it should
use the caller's.

I thought a bit more about this and there are at least some cases we can
probably agree on without trouble:

* If a transaction or subtransaction aborts, all GUC changes made within
it disappear, whether they're from per-function GUC attributes or SET
commands. This seems clearly correct. So we need only consider cases
where no error occurs.

* A regular SET (without LOCAL) propagates clear out to the top level
and becomes the session setting, if not aborted. Hence it must/will
override any per-function settings, either in its own function or
callers.

So it seems that only SET LOCAL within a function with per-function
GUC settings is at issue. I think that there is a pretty strong
use-case for saying that if you have a per-function setting of a
particular variable foo, then any "SET LOCAL foo" within the function
ought to vanish at function end --- for instance a function could want
to try a few different search_path settings and automatically revert to
the caller's setting on exit. The question is what about SET LOCAL
on a variable that *hasn't* been explicitly SET by the function
definition. Either approach we take with it could be surprising,
but probably having it revert at function end is more surprising...

I notice BTW that we have never updated the SET reference page since
subtransactions were introduced --- it still says only that SET LOCAL
is "local to the current transaction", without a word about
subtransactions. So we have a documentation problem anyway. I recall
that we had some discussion during the 8.0 dev cycle about whether
having SET LOCAL's effects end at the end of the current subtransaction
was really a good idea, given that subtransactions aren't the conceptual
model the SQL spec defines, but nothing was ever done about changing
the implementation. In fact, our current recommendation for
implementing secure SECURITY DEFINER functions (use SET LOCAL to change
search_path) really depends on that nowhere-documented behavior ...
so it's probably too late to consider changing it now. But this would
be the time, if we ever are going to reconsider it.

Comments?

regards, tom lane

On Sun, Sep 02, 2007 at 12:08:00PM -0400, Tom Lane wrote:
> I notice BTW that we have never updated the SET reference page since
> subtransactions were introduced --- it still says only that SET LOCAL
> is "local to the current transaction", without a word about
> subtransactions. So we have a documentation problem anyway. I recall
> that we had some discussion during the 8.0 dev cycle about whether
> having SET LOCAL's effects end at the end of the current subtransaction
> was really a good idea, given that subtransactions aren't the conceptual
> model the SQL spec defines, but nothing was ever done about changing
> the implementation.

ISTM that's the real problem; SET LOCAL wasn't fully updated/considered
when subtransactions were added.

One way to handle this would be to have 3 different behaviors for SET:
session-level, transaction-level, and sub-transaction level. If we had
that, we could probably make an across-the-board call that all functions
operate as if in their own sub-transaction, at least when it comes to
SET.

Whatever we decide on, least-surprise would dictate that it's the
same whether you apply function-specific settings or not.
--
Decibel!, aka Jim Nasby decibel(at)decibel(dot)org
EnterpriseDB http://enterprisedb.com 512.569.9461 (cell)

On Mon, 2007-09-03 at 04:09 -0500, Decibel! wrote:
> On Sun, Sep 02, 2007 at 12:08:00PM -0400, Tom Lane wrote:
> > I notice BTW that we have never updated the SET reference page since
> > subtransactions were introduced --- it still says only that SET LOCAL
> > is "local to the current transaction", without a word about
> > subtransactions. So we have a documentation problem anyway. I recall
> > that we had some discussion during the 8.0 dev cycle about whether
> > having SET LOCAL's effects end at the end of the current subtransaction
> > was really a good idea, given that subtransactions aren't the conceptual
> > model the SQL spec defines, but nothing was ever done about changing
> > the implementation.
>
> ISTM that's the real problem; SET LOCAL wasn't fully updated/considered
> when subtransactions were added.
>
> One way to handle this would be to have 3 different behaviors for SET:
> session-level, transaction-level, and sub-transaction level. If we had
> that, we could probably make an across-the-board call that all functions
> operate as if in their own sub-transaction, at least when it comes to
> SET.

What would be the use case for that? I can't see a single reason to do a
SET LOCAL SUBTRANSACTION or whatever you'd call it. What you suggest
sounds nicely symmetrical, but I don't think we need it in practice.

ISTM that SET LOCAL is mostly superceded by per-function parameters.
Most parameters need to be tied to code, not transactions. Of course, my
wish to use synchronous_commit *was* tied to a transaction, but not a
subtransaction.

per-function parameters are sorely needed anyhow, since with session
pools we can't easily use the username for SET parameters.

--
Simon Riggs
2ndQuadrant http://www.2ndQuadrant.com

Simon Riggs <simon(at)2ndquadrant(dot)com> writes:
> ISTM that SET LOCAL is mostly superceded by per-function parameters.

Mostly, but not entirely. The case where you still need SET LOCAL is
where the value you want to use locally has to be computed, or where you
need to change it more than once within the function. Yet in such cases
it'd still be handy to let the SET-clause mechanism deal with the detail
of restoring the caller's value at exit.

There is also a fairly nasty backward-compatibility problem. Suppose
that security definer function OldSD uses the recommended-up-to-now
method for setting a secure search path, which I quote from the 8.2
manual:

old_path := pg_catalog.current_setting('search_path');

PERFORM pg_catalog.set_config('search_path', 'admin, pg_temp', true);

-- Do whatever secure work we came for.

PERFORM pg_catalog.set_config('search_path', old_path, true);

(The set_config calls are equivalent to SET LOCAL.) Also suppose that
security definer function NewSD uses the fancy new function-local-
SET-clause method to avoid all that tedious stuff there. Now suppose
that NewSD calls OldSD. If SET LOCAL overrides SET-clauses, this
happens:

* NewSD saves outer search path and sets its own.
* OldSD saves NewSD's search path, then sets its own with SET LOCAL.
* OldSD restores NewSD's search path using SET LOCAL.
* NewSD tries to restore outer search path, but silently fails
because SET LOCAL takes precedence.
* We exit to the caller with NewSD's search path still in effect.

This scenario will surely happen in the field, and therefore I argue
that we *must* not allow SET LOCAL's effects to persist beyond the
exit from a surrounding function-local SET clause on the same variable.

I'm not sure what conclusions that leads to for other cases, though.
We don't necessarily have to be consistent between the case where
SET affects a variable and the case where it doesn't.

regards, tom lane

Tom Lane wrote:
> "Florian G. Pflug" <fgp(at)phlo(dot)org> writes:
>> Tom Lane wrote:
> So it seems that only SET LOCAL within a function with per-function
> GUC settings is at issue. I think that there is a pretty strong
> use-case for saying that if you have a per-function setting of a
> particular variable foo, then any "SET LOCAL foo" within the function
> ought to vanish at function end --- for instance a function could want
> to try a few different search_path settings and automatically revert to
> the caller's setting on exit.
Agreed.

> The question is what about SET LOCAL
> on a variable that *hasn't* been explicitly SET by the function
> definition. Either approach we take with it could be surprising,
> but probably having it revert at function end is more surprising...

At least for me, the least surprising behaviour would be to
revert it too. Than the rule becomes "a function is always
executed in a pseudo-subtransaction that affects only GUCs"

Since at least for pl/pgsql, a function body *alreay* is a
BEGIN/END block - and therefore syntactically even looks
like a subtransaction - this seems quite logical.

And it would mean that the semantics of "SET LOCAL" won't change,
just because you add an EXCEPTION clause to the function's toplevel
BEGIN/END block.

greetings, Florian Pflug

"Florian G. Pflug" <fgp(at)phlo(dot)org> writes:
> At least for me, the least surprising behaviour would be to
> revert it too. Than the rule becomes "a function is always
> executed in a pseudo-subtransaction that affects only GUCs"

Only if it has at least one SET clause. The overhead is too high
to insist on this for every function call.

regards, tom lane

Tom Lane wrote:
> "Florian G. Pflug" <fgp(at)phlo(dot)org> writes:
>> At least for me, the least surprising behaviour would be to
>> revert it too. Than the rule becomes "a function is always
>> executed in a pseudo-subtransaction that affects only GUCs"
>
> Only if it has at least one SET clause. The overhead is too high
> to insist on this for every function call.

In that case, I agree that only variables specified in a SET-clause
should be reverted. Otherwise, adding or removing
SET-clauses (e.g, because you chose a different implementation
of a function that suddenly doesn't need regexps anymore) will
cause quite arbitrary behavior changes.

And the rule becomes (I tend to forget things, so I like simple
rules that I can remember ;-) ) "For each SET-clause, there is
a pseudo-subtransaction affecting only *this* GUC".

greetings, Florian Pflug

"Florian G. Pflug" <fgp(at)phlo(dot)org> writes:
> And the rule becomes (I tend to forget things, so I like simple
> rules that I can remember ;-) ) "For each SET-clause, there is
> a pseudo-subtransaction affecting only *this* GUC".

The other question is whether we want to change the behavior of SET
LOCAL even in the absence of function SET-clauses. The current rule
is that a LOCAL setting goes away at subtransaction commit, leading
to this behavior:

regression=# show regex_flavor;
regex_flavor
--------------
advanced
(1 row)

regression=# begin;
BEGIN
regression=# savepoint x;
SAVEPOINT
regression=# set local regex_flavor to basic;
SET
regression=# release x;
RELEASE
regression=# show regex_flavor;
regex_flavor
--------------
advanced
(1 row)

which makes some sense if you think of "release" as "subtransaction
end", but not a lot if you think of it as forgetting a savepoint.
Likewise, SET LOCAL within a plpgsql exception block goes away at
successful block exit, which is not the first thing you'd expect.
Neither of these behaviors are documented anywhere AFAIR; certainly
the SET reference page doesn't explain 'em.

I think we should probably take this opportunity to fix that, and
make SET LOCAL mean "persists until end of current top-level
transaction, unless rolled back earlier or within a function SET
clause".

So:

* Plain SET takes effect immediately and persists unless rolled back
or overridden by another explicit SET. In particular the value will
escape out of a function that has a SET-clause for the same variable.

* SET LOCAL takes effect immediately and persists until rolled back,
overridden by another SET, or we exit a function that has a SET-clause
for the same variable.

* Rollback of a transaction or subtransaction cancels any SET or SET
LOCAL within it. Otherwise, the latest un-rolled-back SET or SET LOCAL
determines the active value within a transaction, and the latest
un-rolled-back SET determines the value that will prevail after the
transaction commits.

* A function SET clause saves the entry-time value, and restores it at
function exit, except when overridden by an un-rolled-back SET (but not
SET LOCAL) within the function.

Clear to everyone? Any objections?

As far as implementation, I think this can be made to happen by
rejiggering the value stacking and unstacking rules within guc.c.
I'm tempted to try to get rid of the "tentative" value slots at the
same time. That's a hangover from the pre-subtransaction
implementation, when we only had to remember one inactive value for the
case of SET followed by SET LOCAL within a transaction. Now that we
have a stack of saved values, it seems to make more sense to try to
handle this case by stacking the SET value when we hit SET LOCAL at the
same nesting level.

regards, tom lane

Tom Lane wrote:
> "Florian G. Pflug" <fgp(at)phlo(dot)org> writes:
>> And the rule becomes (I tend to forget things, so I like simple
>> rules that I can remember ;-) ) "For each SET-clause, there is
>> a pseudo-subtransaction affecting only *this* GUC".
>
> The other question is whether we want to change the behavior of SET
> LOCAL even in the absence of function SET-clauses. The current rule
> is that a LOCAL setting goes away at subtransaction commit, leading
> to this behavior:
>
> regression=# show regex_flavor;
> regex_flavor
> --------------
> advanced
> (1 row)
>
> regression=# begin;
> BEGIN
> regression=# savepoint x;
> SAVEPOINT
> regression=# set local regex_flavor to basic;
> SET
> regression=# release x;
> RELEASE
> regression=# show regex_flavor;
> regex_flavor
> --------------
> advanced
> (1 row)
>
> which makes some sense if you think of "release" as "subtransaction
> end", but not a lot if you think of it as forgetting a savepoint.
> Likewise, SET LOCAL within a plpgsql exception block goes away at
> successful block exit, which is not the first thing you'd expect.
> Neither of these behaviors are documented anywhere AFAIR; certainly
> the SET reference page doesn't explain 'em.
>
> I think we should probably take this opportunity to fix that, and
> make SET LOCAL mean "persists until end of current top-level
> transaction, unless rolled back earlier or within a function SET
> clause".
>
> So:
>
> * Plain SET takes effect immediately and persists unless rolled back
> or overridden by another explicit SET. In particular the value will
> escape out of a function that has a SET-clause for the same variable.
>
> * SET LOCAL takes effect immediately and persists until rolled back,
> overridden by another SET, or we exit a function that has a SET-clause
> for the same variable.
>
> * Rollback of a transaction or subtransaction cancels any SET or SET
> LOCAL within it. Otherwise, the latest un-rolled-back SET or SET LOCAL
> determines the active value within a transaction, and the latest
> un-rolled-back SET determines the value that will prevail after the
> transaction commits.
>
> * A function SET clause saves the entry-time value, and restores it at
> function exit, except when overridden by an un-rolled-back SET (but not
> SET LOCAL) within the function.
>
> Clear to everyone? Any objections?
That makes "SET LOCAL" completely equivalent to "SET", except
when used inside a function that has a corresponding SET-clause, right?
So I think *if* this is done, "SET LOCAL" should be renamed to
"SET FUNCTION". This would also prevent confusion, because everyone
who currently uses SET LOCAL will have to change his code anyway,
since the semantics change for every use-case apart from functions
with SET-clauses, which don't exist in 8.2.
Or am I overlooking something?

And renaming SET LOCAL also emphasized that point that we are taking
away functionality here - even if that functionality might not seem
very useful.

BTW, I *did* check the documentation before responding to Simon's original
mail, and I *did* read it as "SET LOCAL goes away a subtransaction end".
I figured that since there is no word on subtransactions in that part
of the documentation, transaction will apply equally to both toplevel
and subtransaction. It might very well be that I'm the only one who
read it that way, though ;-) And I must admin that I wasn't completely
sure, so I *did* try it out before I posted...

I'd strong prefer "SET LOCAL" to kept it's current semantics, only that "SET
LOCAL" changes will now be rolled back if the function has a matching
SET-clause. For multiple reasons:
.) It's useful to be able to temporarily change GUCs from a client, and
being able to reset them afterwards. Using a subtransaction for this
is maybe a bit wastefull, but at least it works.
.) In pl/pgsql, that fact that "SET LOCAL" goes away after the current
BEGIN/END block seems entirely logical.
.) It doesn't take away existing functionality

greetings, Florian Pflug

"Florian G. Pflug" <fgp(at)phlo(dot)org> writes:
> Tom Lane wrote:
>> Clear to everyone? Any objections?

> That makes "SET LOCAL" completely equivalent to "SET", except
> when used inside a function that has a corresponding SET-clause, right?

Maybe it wasn't clear :-(. They aren't equivalent because in the
absence of rollback, SET's effects persist past main-transaction end;
SET LOCAL's don't. That's the way they were defined originally
(pre-subtransactions) and it still seems to make sense.

> So I think *if* this is done, "SET LOCAL" should be renamed to
> "SET FUNCTION". This would also prevent confusion, because everyone
> who currently uses SET LOCAL will have to change his code anyway,
> since the semantics change for every use-case apart from functions
> with SET-clauses, which don't exist in 8.2.

I'm not sure how many people have really written code that depends on
the behavior of SET LOCAL rolling back at successful subtransaction end.
I think we'd have heard about it if very many people had noticed,
because it's not what the manual says.

For the one use we've actually advocated (setting a temporary value
within a function and then reverting to the old setting before exit),
there isn't any visible change in behavior, since abandonment of the
restored value at subtransaction end still ends up with the same result.

> And renaming SET LOCAL also emphasized that point that we are taking
> away functionality here - even if that functionality might not seem
> very useful.

We can't break the officially advocated solution for secure search_path.
However, that particular coding pattern will still work with the change
I'm proposing. It's only where you *don't* manually restore the prior
value that you might notice a difference.

> BTW, I *did* check the documentation before responding to Simon's original
> mail, and I *did* read it as "SET LOCAL goes away a subtransaction end".
> I figured that since there is no word on subtransactions in that part
> of the documentation, transaction will apply equally to both toplevel
> and subtransaction.

Yeah, but you know that it's subtransactions under the hood, whereas
someone who's thinking in terms of SAVEPOINT/RELEASE and BEGIN/EXCEPTION
probably hasn't a clue about that.

> .) In pl/pgsql, that fact that "SET LOCAL" goes away after the current
> BEGIN/END block seems entirely logical.

I don't think so ... your other side-effects such as table updates don't
disappear, so why should SET's?

I'm not necessarily averse to inventing a third version of SET, but I
don't see a well-thought-out proposal here. In particular, we should be
making an effort to *not* expose the concept of subtransaction at the
SQL level at all, because that's not what the spec has.

regards, tom lane

Tom Lane wrote:
> "Florian G. Pflug" <fgp(at)phlo(dot)org> writes:
>> Tom Lane wrote:
>>> Clear to everyone? Any objections?
>
>> That makes "SET LOCAL" completely equivalent to "SET", except
>> when used inside a function that has a corresponding SET-clause, right?
>
> Maybe it wasn't clear :-(. They aren't equivalent because in the
> absence of rollback, SET's effects persist past main-transaction end;
> SET LOCAL's don't. That's the way they were defined originally
> (pre-subtransactions) and it still seems to make sense.
Ah, OK - things make much more sense now.

>> So I think *if* this is done, "SET LOCAL" should be renamed to
>> "SET FUNCTION". This would also prevent confusion, because everyone
>> who currently uses SET LOCAL will have to change his code anyway,
>> since the semantics change for every use-case apart from functions
>> with SET-clauses, which don't exist in 8.2.
>
> I'm not sure how many people have really written code that depends on
> the behavior of SET LOCAL rolling back at successful subtransaction end.
> I think we'd have heard about it if very many people had noticed,
> because it's not what the manual says.
>
> For the one use we've actually advocated (setting a temporary value
> within a function and then reverting to the old setting before exit),
> there isn't any visible change in behavior, since abandonment of the
> restored value at subtransaction end still ends up with the same result.
>
>> And renaming SET LOCAL also emphasized that point that we are taking
>> away functionality here - even if that functionality might not seem
>> very useful.
>
> We can't break the officially advocated solution for secure search_path.
> However, that particular coding pattern will still work with the change
> I'm proposing. It's only where you *don't* manually restore the prior
> value that you might notice a difference.
>
>> BTW, I *did* check the documentation before responding to Simon's original
>> mail, and I *did* read it as "SET LOCAL goes away a subtransaction end".
>> I figured that since there is no word on subtransactions in that part
>> of the documentation, transaction will apply equally to both toplevel
>> and subtransaction.
>
> Yeah, but you know that it's subtransactions under the hood, whereas
> someone who's thinking in terms of SAVEPOINT/RELEASE and BEGIN/EXCEPTION
> probably hasn't a clue about that.
I plead guilty here ;-). That whole SAVEPOINT/RELEASE thing always seemed
strange to me - I just accepted it at some point, but still translated it
into something hierarchical I guess....

>
>> .) In pl/pgsql, that fact that "SET LOCAL" goes away after the current
>> BEGIN/END block seems entirely logical.
>
> I don't think so ... your other side-effects such as table updates don't
> disappear, so why should SET's
I guess because LOCAL to me implies some lexical locality - like the
surrounding BEGIN/END block.

> I'm not necessarily averse to inventing a third version of SET, but I
> don't see a well-thought-out proposal here. In particular, we should be
> making an effort to *not* expose the concept of subtransaction at the
> SQL level at all, because that's not what the spec has.
Thanks for your explanation - I can see your point now, after realizing
why the spec has SAVEPOINT/RELEASE and *not* nested BEGIN/COMMIT blocks.

So, at least on the SQL-level, I guess I agree - your new semantics fit
better with the sql spec, even if they seemed quite strange to me at
first sight. Though maybe we should add "SET TRANSACTION" as a synonym for
"SET LOCAL"? - the former seems to convey your new semantics much better than
the later.

It still seems a bit strange that "SET LOCAL" is undone at function-exit,
if the function has a matching SET-clause. But we need that for backwards-
compatibility of the secure-search_path workaround, right? Maybe we could
make "SET TRANSACTION" different from "SET LOCAL" in pl/pgsql, and warn
if "SET LOCAL" is used? That would enable us either get rid of "SET LOCAL"
in the long term, or to really make it local to the surrounding BEGIN/END
block.

So, to reiterate, my idea is
.) Make "SET TRANSACTION" a synonym for "SET LOCAL" at the SQL-Level.
.) In pl/pgsql, "SET TRANSACTION" sets a new value that is kept after the
function exits, even if the function has a matching SET-clause.
.) "SET LOCAL" in pl/pgsql set a new value that is kept if the function
has no matching SET-clause. If it has one, the value is restored.
In any case, we emit a warning that "SET LOCAL" is going away.
.) One day, make "SET LOCAL" in pl/pgsql mean "local to the surrounding
BEGIN/END block". Independent of any SET-clauses the function
might or might not have.

The last idea might seem to create a inconsistency between the SQL-level
and pl/pgsql, but I think it does not. "SET LOCAL" is local to the
surrounding BEGIN/{END|COMMIT} block in both cases - it's just that
you have nested such blocks in pl/pgsql, but not in plain SQL.

greetings, Florian Pflug

"Florian G. Pflug" <fgp(at)phlo(dot)org> writes:
> It still seems a bit strange that "SET LOCAL" is undone at function-exit,
> if the function has a matching SET-clause. But we need that for backwards-
> compatibility of the secure-search_path workaround, right?

Yeah, I'm afraid we backed ourselves into a corner on that one.

> So, to reiterate, my idea is
> .) Make "SET TRANSACTION" a synonym for "SET LOCAL" at the SQL-Level.
> .) In pl/pgsql, "SET TRANSACTION" sets a new value that is kept after the
> function exits, even if the function has a matching SET-clause.
> .) "SET LOCAL" in pl/pgsql set a new value that is kept if the function
> has no matching SET-clause. If it has one, the value is restored.
> In any case, we emit a warning that "SET LOCAL" is going away.
> .) One day, make "SET LOCAL" in pl/pgsql mean "local to the surrounding
> BEGIN/END block". Independent of any SET-clauses the function
> might or might not have.

I don't think it's a good idea to change SET LOCAL now and plan on
changing it again later ;-). If we really want BEGIN-block-local
SET capability, I'd prefer to think of some new keyword for that.
But I'm not convinced it's interesting --- given the proposed behavior
of function SET-clauses, attaching a SET to your function seems like
it'll cover the need for restoring outer values.

regards, tom lane

Tom Lane wrote:
>> So, to reiterate, my idea is
>> .) Make "SET TRANSACTION" a synonym for "SET LOCAL" at the SQL-Level.
>> .) In pl/pgsql, "SET TRANSACTION" sets a new value that is kept after the
>> function exits, even if the function has a matching SET-clause.
>> .) "SET LOCAL" in pl/pgsql set a new value that is kept if the function
>> has no matching SET-clause. If it has one, the value is restored.
>> In any case, we emit a warning that "SET LOCAL" is going away.
>> .) One day, make "SET LOCAL" in pl/pgsql mean "local to the surrounding
>> BEGIN/END block". Independent of any SET-clauses the function
>> might or might not have.
>
> I don't think it's a good idea to change SET LOCAL now and plan on
> changing it again later ;-). If we really want BEGIN-block-local
> SET capability, I'd prefer to think of some new keyword for that.
> But I'm not convinced it's interesting --- given the proposed behavior
> of function SET-clauses, attaching a SET to your function seems like
> it'll cover the need for restoring outer values.

Hm... could we still have "SET TRANSACTION" as a synonym for "SET LOCAL"?
That would blend nicely with "SET TRANSACTION ISOLATION LEVEL" and
"SET TRANSACTION READ ONLY".

[ thinking... ] Hey, wait a moment. Regarding "SET TRANSACTION READ ONLY" -
This is not strictly speaking a GUC, but still, if we pretend that
there are no subtransaction, that command should too propage to the
outermost transaction on release, shouldn't it?

This is what happens currently (CVS HEAD with at least your initial
function-SET-clause patch already in)
regression=# begin ;
BEGIN
regression=# savepoint s1 ;
SAVEPOINT
regression=# set transaction read only ;
SET
regression=# release s1 ;
RELEASE
regression=# create table test (id int) ;
CREATE TABLE
regression=# commit ;
COMMIT

compared to:
regression=# begin ;
BEGIN
regression=# set transaction read only ;
SET
regression=# create table test (id int) ;
ERROR: transaction is read-only

I believe that for consistencies sake, the "set transaction read only" should
have propagated to the outermost transaction on "release s1".

greetings, Florian Pflug

Florian G. Pflug wrote:
> Tom Lane wrote:
>>> So, to reiterate, my idea is
>>> .) Make "SET TRANSACTION" a synonym for "SET LOCAL" at the SQL-Level.
>>> .) In pl/pgsql, "SET TRANSACTION" sets a new value that is kept after
>>> the
>>> function exits, even if the function has a matching SET-clause.
>>> .) "SET LOCAL" in pl/pgsql set a new value that is kept if the function
>>> has no matching SET-clause. If it has one, the value is restored.
>>> In any case, we emit a warning that "SET LOCAL" is going away.
>>> .) One day, make "SET LOCAL" in pl/pgsql mean "local to the surrounding
>>> BEGIN/END block". Independent of any SET-clauses the function
>>> might or might not have.
>>
>> I don't think it's a good idea to change SET LOCAL now and plan on
>> changing it again later ;-). If we really want BEGIN-block-local
>> SET capability, I'd prefer to think of some new keyword for that.
>> But I'm not convinced it's interesting --- given the proposed behavior
>> of function SET-clauses, attaching a SET to your function seems like
>> it'll cover the need for restoring outer values.
>
> Hm... could we still have "SET TRANSACTION" as a synonym for "SET LOCAL"?
> That would blend nicely with "SET TRANSACTION ISOLATION LEVEL" and
> "SET TRANSACTION READ ONLY".

I don't think it's a very good idea to make SET TRANSACTION an alias for
SET LOCAL, because SET TRANSACTION has already got its own meaning in the
SQL spec - it sets transaction modes. Although I agree with you that
variables set with SET LOCAL are also attached to the transaction (by
definition), I would still rather separate transaction-local GUCs from
spec-defined transaction modes.

As precedence, they have two separate reference pages already:
http://www.postgresql.org/docs/8.1/interactive/sql-set.html
http://www.postgresql.org/docs/8.1/interactive/sql-set-transaction.html

> [ thinking... ] Hey, wait a moment. Regarding "SET TRANSACTION READ ONLY" -
> This is not strictly speaking a GUC, but still, if we pretend that
> there are no subtransaction, that command should too propage to the
> outermost transaction on release, shouldn't it?
>
...
>
> I believe that for consistencies sake, the "set transaction read only"
> should have propagated to the outermost transaction on "release s1".

Sounds reasonable to me. I understand SAVEPOINT/RELEASE come from the SQL
standard. So does the SQL standard say anything about this?

Best Regards
Michael Paesold

Michael Paesold <mpaesold(at)gmx(dot)at> writes:
> Florian G. Pflug wrote:
>> Hm... could we still have "SET TRANSACTION" as a synonym for "SET LOCAL"?
>> That would blend nicely with "SET TRANSACTION ISOLATION LEVEL" and
>> "SET TRANSACTION READ ONLY".

> I don't think it's a very good idea to make SET TRANSACTION an alias for
> SET LOCAL, because SET TRANSACTION has already got its own meaning in the
> SQL spec - it sets transaction modes.

Yeah --- I'm not sure we could even do it without getting shift/reduce
conflicts in bison.

There is some attraction to the idea of keeping SET LOCAL's current
behavior and inventing a third form of SET that has the
lasts-till-end-of-current-main-transaction behavior. However
(1) we'd have to pick some other keyword than TRANSACTION;
(2) I still don't see how to document SET LOCAL's current behavior
without introducing the concept of "subtransaction" into it, and
I think we shouldn't do that.

Basically my perspective on SET LOCAL is that its current behavior is a
bug, and even though it's been that way for a couple major releases now,
it's still something we oughta fix while we are busy whacking that part
of the code around. Florian's example with SET TRANSACTION READ ONLY
proves that it's a bug --- RELEASE is not defined to change any
transaction modes.

regards, tom lane

Tom Lane wrote:
> Michael Paesold <mpaesold(at)gmx(dot)at> writes:
>> I don't think it's a very good idea to make SET TRANSACTION an alias for
>> SET LOCAL, because SET TRANSACTION has already got its own meaning in the
>> SQL spec - it sets transaction modes.
>
> Yeah --- I'm not sure we could even do it without getting shift/reduce
> conflicts in bison.
>
> There is some attraction to the idea of keeping SET LOCAL's current
> behavior and inventing a third form of SET that has the
> lasts-till-end-of-current-main-transaction behavior. However
> (1) we'd have to pick some other keyword than TRANSACTION;
> (2) I still don't see how to document SET LOCAL's current behavior
> without introducing the concept of "subtransaction" into it, and
> I think we shouldn't do that.
>
> Basically my perspective on SET LOCAL is that its current behavior is a
> bug, and even though it's been that way for a couple major releases now,
> it's still something we oughta fix while we are busy whacking that part
> of the code around. Florian's example with SET TRANSACTION READ ONLY
> proves that it's a bug --- RELEASE is not defined to change any
> transaction modes.

Yeah, I think your original proposal was really sound. I would not
expect the current SET LOCAL behaviour in the context of savepoints.
If we really need the current behaviour, we should find a new name for
this lasts-until-savepoint-release-or-transaction-end thingy.

Best Regards
Michael Paesold

On 9/5/07, Michael Paesold <mpaesold(at)gmx(dot)at> wrote:
> Tom Lane wrote:
> > Basically my perspective on SET LOCAL is that its current behavior is a
> > bug, and even though it's been that way for a couple major releases now,
> > it's still something we oughta fix while we are busy whacking that part
> > of the code around. Florian's example with SET TRANSACTION READ ONLY
> > proves that it's a bug --- RELEASE is not defined to change any
> > transaction modes.
>
> Yeah, I think your original proposal was really sound. I would not
> expect the current SET LOCAL behaviour in the context of savepoints.
> If we really need the current behaviour, we should find a new name for
> this lasts-until-savepoint-release-or-transaction-end thingy.

So, if I read you correctly, in summary we'd like to:

* make SET LOCAL local to the transaction (i.e., make it behave as documented),
* abandon the idea of a subtransaction-local SET, because the new
function-local SET takes care of the interesting use-cases for that,
* somehow deal with the incompatibility with the 8.2 "security
definer" workaround.

Tom's proposal to handle the latter was that when a function-local SET
reverts, it overrides any inner SET LOCALs.

Am I on the right page?

Cheers,
BJ

"Brendan Jurd" <direvus(at)gmail(dot)com> writes:
> So, if I read you correctly, in summary we'd like to:

> * make SET LOCAL local to the transaction (i.e., make it behave as documented),
> * abandon the idea of a subtransaction-local SET, because the new
> function-local SET takes care of the interesting use-cases for that,
> * somehow deal with the incompatibility with the 8.2 "security
> definer" workaround.

> Tom's proposal to handle the latter was that when a function-local SET
> reverts, it overrides any inner SET LOCALs.

> Am I on the right page?

Got it in one, I believe.

regards, tom lane

On 9/5/07, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "Brendan Jurd" <direvus(at)gmail(dot)com> writes:
> > Am I on the right page?
>
> Got it in one, I believe.

In that case, +1 for your proposed changes.

At first, like Florian, I found the idea of a SET LOCAL ever
persisting beyond a function astonishing, but that's because I was
approaching the term LOCAL from a programming frame of mind, not an
SQL one. Once you appreciate that LOCAL means local to the
transaction, rather than local to the programming context, it all
makes sense.

Cheers,
BJ

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Wed, Sep 05, 2007 at 02:13:57PM +1000, Brendan Jurd wrote:
> On 9/5/07, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > "Brendan Jurd" <direvus(at)gmail(dot)com> writes:
> > > Am I on the right page?
> >
> > Got it in one, I believe.
>
> In that case, +1 for your proposed changes.
>
> At first, like Florian, I found the idea of a SET LOCAL ever
> persisting beyond a function astonishing, but that's because I was
> approaching the term LOCAL from a programming frame of mind, not an
> SQL one [...]

As an unqualified POV, seeing that this got at least two people confused
- -- wouldn't it make sense to be more verbose and call the thing SET
TRANSACTION LOCAL (not just TRANSACTION, which is ambiguous as we have
already seen). May be even SET LOCAL TO TRANSACTION (that gives at least
some room for possible extensibility).

I know too little about the parser to have even an idea whether this
would be feasible at all.

Regards
- -- tomás
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)

iD8DBQFG3mFABcgs9XrR2kYRAug1AJ9FJdFEjDGpYWSj09+LgRv218efdwCcDBR8
kjE8O+QCdD/DMntr6mjHBoA=
=FI+2
-----END PGP SIGNATURE-----