Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block

Hi!

Calling "SET TRANSACTION ISOLATION LEVEL ..." outside a transaction
block has no effect. This is unlike "LOCK ..." and "DECLARE foo
CURSOR FOR ...", which both raise an error. This is also unlike
MySQL, where such a statement will affect the next transaction
performed. There's some risk of data corruption, as a user might
assume he's working on a snapshot, while in fact he's not.

I suggest issuing a warning, notice or error message when "SET
TRANSACTION ..." is called outside a transaction block, possibly
directing the user to the "SET SESSION CHARACTERISTICS AS TRANSACTION
..." syntax.

I'm not familiar with the PostgreSQL source code, but it seems this
would have to be added to check_XactIsoLevel() or by calling
RequireTransactionChain() at some appropriate location.

On Wednesday, January 30, 2013 6:53 AM Morten Hustveit wrote:
> Hi!
>
> Calling "SET TRANSACTION ISOLATION LEVEL ..." outside a transaction
> block has no effect. This is unlike "LOCK ..." and "DECLARE foo
> CURSOR FOR ...", which both raise an error. This is also unlike
> MySQL, where such a statement will affect the next transaction
> performed. There's some risk of data corruption, as a user might
> assume he's working on a snapshot, while in fact he's not.

The behavior of "SET TRANSACTION ISOLATION LEVEL ..." needs to be compared with "SET LOCAL ..".
These commands are used to set property of current transaction in which they are executed.

The usage can be clear with below function, where it is used to set the current transaction property.

Create or Replace function temp_trans() Returns boolean AS $$
Declare sync_status boolean;
Begin
Set LOCAL synchronous_commit=off;
show synchronous_commit into sync_status;
return sync_status;
End;
$$ Language plpgsql;

> I suggest issuing a warning, notice or error message when "SET
> TRANSACTION ..." is called outside a transaction block, possibly
> directing the user to the "SET SESSION CHARACTERISTICS AS TRANSACTION
> ..." syntax.

It is already mentioned in documentation that SET Transaction command is used to set characteristics of current transaction (http://www.postgresql.org/docs/9.2/static/sql-set-transaction.html).

I think user should be aware of effect before using SET commands, as these are used at various levels (TRANSACTION, SESSION, ...).

With Regards,
Amit Kapila.

On Fri, Feb 1, 2013 at 12:04 AM, Amit Kapila <amit(dot)kapila(at)huawei(dot)com> wrote:
> I think user should be aware of effect before using SET commands, as these are used at various levels (TRANSACTION, SESSION, ...).

Ideally, sure. But these kinds of mistakes are easy to make. That's
why LOCK and DECLARE CURSOR already emit errors in this case - why
should this one be any different?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Saturday, February 02, 2013 9:08 PM Robert Haas wrote:
On Fri, Feb 1, 2013 at 12:04 AM, Amit Kapila <amit(dot)kapila(at)huawei(dot)com> wrote:
>> I think user should be aware of effect before using SET commands, as these are used at various levels (TRANSACTION, SESSION, ...).

> Ideally, sure. But these kinds of mistakes are easy to make.

You mean to say that after using SET Transaction, user can think below statements will
use modified transaction property. But I think if user doesn't understand by default
transaction will be committed after the statement execution, he might expect after
few statements he can rollback.

> That's why LOCK and DECLARE CURSOR already emit errors in this case - why
> should this one be any different?

IMO, I think error should be given when it is not possible to execute current statement.

Not only LOCK,DECLARE CURSOR but SAVEPOINT and some other statements also give the same error,
so if we want to throw error for such behavior, we can find all such similar statements
(SET TRANSACTION, SET LOCAL, etc) and do it for all.

This can be helpful to some users, but not sure if such behavior (statement can be executed but it will not have any sense)
can be always detectable and maintaible.

With Regards,
Amit Kapila.

On Sun, Feb 3, 2013 at 07:19:14AM +0000, Amit kapila wrote:
>
> On Saturday, February 02, 2013 9:08 PM Robert Haas wrote:
> On Fri, Feb 1, 2013 at 12:04 AM, Amit Kapila <amit(dot)kapila(at)huawei(dot)com> wrote:
> >> I think user should be aware of effect before using SET commands, as these are used at various levels (TRANSACTION, SESSION, ...).
>
> > Ideally, sure. But these kinds of mistakes are easy to make.
>
> You mean to say that after using SET Transaction, user can think below statements will
> use modified transaction property. But I think if user doesn't understand by default
> transaction will be committed after the statement execution, he might expect after
> few statements he can rollback.
>
> > That's why LOCK and DECLARE CURSOR already emit errors in this case - why
> > should this one be any different?
>
> IMO, I think error should be given when it is not possible to execute current statement.
>
> Not only LOCK,DECLARE CURSOR but SAVEPOINT and some other statements also give the same error,
> so if we want to throw error for such behavior, we can find all such similar statements
> (SET TRANSACTION, SET LOCAL, etc) and do it for all.
>
> This can be helpful to some users, but not sure if such behavior (statement can be executed but it will not have any sense)
> can be always detectable and maintaible.

I have created the attached patch which issues an error when SET
TRANSACTION and SET LOCAL are used outside of transactions:

test=> set transaction isolation level serializable;
ERROR: SET TRANSACTION can only be used in transaction blocks
test=> reset transaction isolation level;
ERROR: RESET TRANSACTION can only be used in transaction blocks

test=> set local effective_cache_size = '3MB';
ERROR: SET LOCAL can only be used in transaction blocks
test=> set local effective_cache_size = default;
ERROR: SET LOCAL can only be used in transaction blocks

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On Wed, Sep 11, 2013 at 4:19 AM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> On Sun, Feb 3, 2013 at 07:19:14AM +0000, Amit kapila wrote:
>>
>> On Saturday, February 02, 2013 9:08 PM Robert Haas wrote:
>> On Fri, Feb 1, 2013 at 12:04 AM, Amit Kapila <amit(dot)kapila(at)huawei(dot)com> wrote:
>> >> I think user should be aware of effect before using SET commands, as these are used at various levels (TRANSACTION, SESSION, ...).
>>
>> > Ideally, sure. But these kinds of mistakes are easy to make.
>>
>> You mean to say that after using SET Transaction, user can think below statements will
>> use modified transaction property. But I think if user doesn't understand by default
>> transaction will be committed after the statement execution, he might expect after
>> few statements he can rollback.
>>
>> > That's why LOCK and DECLARE CURSOR already emit errors in this case - why
>> > should this one be any different?
>>
>> IMO, I think error should be given when it is not possible to execute current statement.
>>
>> Not only LOCK,DECLARE CURSOR but SAVEPOINT and some other statements also give the same error,
>> so if we want to throw error for such behavior, we can find all such similar statements
>> (SET TRANSACTION, SET LOCAL, etc) and do it for all.
>>
>> This can be helpful to some users, but not sure if such behavior (statement can be executed but it will not have any sense)
>> can be always detectable and maintaible.
>
> I have created the attached patch which issues an error when SET
> TRANSACTION and SET LOCAL are used outside of transactions:
>
> test=> set transaction isolation level serializable;
> ERROR: SET TRANSACTION can only be used in transaction blocks
> test=> reset transaction isolation level;
> ERROR: RESET TRANSACTION can only be used in transaction blocks
>
> test=> set local effective_cache_size = '3MB';
> ERROR: SET LOCAL can only be used in transaction blocks
> test=> set local effective_cache_size = default;
> ERROR: SET LOCAL can only be used in transaction blocks

Shouldn't we do it for Set Constraints as well?

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Thu, Sep 12, 2013 at 09:38:43AM +0530, Amit Kapila wrote:
> > I have created the attached patch which issues an error when SET
> > TRANSACTION and SET LOCAL are used outside of transactions:
> >
> > test=> set transaction isolation level serializable;
> > ERROR: SET TRANSACTION can only be used in transaction blocks
> > test=> reset transaction isolation level;
> > ERROR: RESET TRANSACTION can only be used in transaction blocks
> >
> > test=> set local effective_cache_size = '3MB';
> > ERROR: SET LOCAL can only be used in transaction blocks
> > test=> set local effective_cache_size = default;
> > ERROR: SET LOCAL can only be used in transaction blocks
>
> Shouldn't we do it for Set Constraints as well?

Oh, very good point. I missed that one. Updated patch attached.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On Wed, Sep 25, 2013 at 2:55 AM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> On Thu, Sep 12, 2013 at 09:38:43AM +0530, Amit Kapila wrote:
>> > I have created the attached patch which issues an error when SET
>> > TRANSACTION and SET LOCAL are used outside of transactions:
>> >
>> > test=> set transaction isolation level serializable;
>> > ERROR: SET TRANSACTION can only be used in transaction blocks
>> > test=> reset transaction isolation level;
>> > ERROR: RESET TRANSACTION can only be used in transaction blocks
>> >
>> > test=> set local effective_cache_size = '3MB';
>> > ERROR: SET LOCAL can only be used in transaction blocks
>> > test=> set local effective_cache_size = default;
>> > ERROR: SET LOCAL can only be used in transaction blocks
>>
>> Shouldn't we do it for Set Constraints as well?
>
> Oh, very good point. I missed that one. Updated patch attached.

1. The function set_config also needs similar functionality, else
there will be inconsistency, the SQL statement will give error but
equivalent function set_config() will succeed.

SQL Command
postgres=# set local search_path='public';
ERROR: SET LOCAL can only be used in transaction blocks

Function
postgres=# select set_config('search_path', 'public', true);
set_config
------------
public
(1 row)

2. I think we should update the documentation as well.

For example:
The documentation of LOCK TABLE
(http://www.postgresql.org/docs/devel/static/sql-lock.html) clearly
indicates that it will give error if used outside transaction block.

"LOCK TABLE is useless outside a transaction block: the lock would
remain held only to the completion of the statement. Therefore
PostgreSQL reports an error if LOCK is used outside a transaction
block. Use BEGINand COMMIT (or ROLLBACK) to define a transaction
block."

The documentation of SET TRANSACTION
(http://www.postgresql.org/docs/devel/static/sql-set-transaction.html)
has below line which doesn't indicate that it will give error if used
outside transaction block.

"If SET TRANSACTION is executed without a prior START TRANSACTION or
BEGIN, it will appear to have no effect, since the transaction will
immediately end."

3.

void
ExecSetVariableStmt(VariableSetStmt *stmt, bool isTopLevel)
{
..
..
else if (strcmp(stmt->name, "TRANSACTION SNAPSHOT") == 0)
{
A_Const *con = (A_Const *) linitial(stmt->args);

RequireTransactionChain(isTopLevel, "SET TRANSACTION");

if (stmt->is_local)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("SET LOCAL TRANSACTION SNAPSHOT is not implemented")));
..
}
..
..
}

Wouldn't it be better if call to RequireTransactionChain() is done
after if (stmt->is_local)?

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Sun, Sep 29, 2013 at 11:40:51AM +0530, Amit Kapila wrote:
> >> Shouldn't we do it for Set Constraints as well?
> >
> > Oh, very good point. I missed that one. Updated patch attached.

I am glad you are seeing things I am not. :-)

> 1. The function set_config also needs similar functionality, else
> there will be inconsistency, the SQL statement will give error but
> equivalent function set_config() will succeed.
>
> SQL Command
> postgres=# set local search_path='public';
> ERROR: SET LOCAL can only be used in transaction blocks
>
> Function
> postgres=# select set_config('search_path', 'public', true);
> set_config
> ------------
> public
> (1 row)

I looked at this but could not see how to easily pass the value of
'isTopLevel' down to the SELECT. All the other checks have isTopLevel
passed down from the utility case statement.

> 2. I think we should update the documentation as well.
>
> For example:
> The documentation of LOCK TABLE
> (http://www.postgresql.org/docs/devel/static/sql-lock.html) clearly
> indicates that it will give error if used outside transaction block.
>
> "LOCK TABLE is useless outside a transaction block: the lock would
> remain held only to the completion of the statement. Therefore
> PostgreSQL reports an error if LOCK is used outside a transaction
> block. Use BEGINand COMMIT (or ROLLBACK) to define a transaction
> block."
>
>
> The documentation of SET TRANSACTION
> (http://www.postgresql.org/docs/devel/static/sql-set-transaction.html)
> has below line which doesn't indicate that it will give error if used
> outside transaction block.
>
> "If SET TRANSACTION is executed without a prior START TRANSACTION or
> BEGIN, it will appear to have no effect, since the transaction will
> immediately end."

Yes, you are right. All cases I changed had mentions of the command
having no effect; I have updated it to mention an error is generated.

> 3.
>
> void
> ExecSetVariableStmt(VariableSetStmt *stmt, bool isTopLevel)
> {
> ..
> ..
> else if (strcmp(stmt->name, "TRANSACTION SNAPSHOT") == 0)
> {
> A_Const *con = (A_Const *) linitial(stmt->args);
>
> RequireTransactionChain(isTopLevel, "SET TRANSACTION");
>
> if (stmt->is_local)
> ereport(ERROR,
> (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> errmsg("SET LOCAL TRANSACTION SNAPSHOT is not implemented")));
> ..
> }
> ..
> ..
> }
>
> Wouldn't it be better if call to RequireTransactionChain() is done
> after if (stmt->is_local)?

Yes, good point. Done.

Thanks much for your review. Updated patch attached.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On Tue, Oct 1, 2013 at 7:49 AM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> On Sun, Sep 29, 2013 at 11:40:51AM +0530, Amit Kapila wrote:
>> >> Shouldn't we do it for Set Constraints as well?
>> >
>> > Oh, very good point. I missed that one. Updated patch attached.
>
> I am glad you are seeing things I am not. :-)
>
>> 1. The function set_config also needs similar functionality, else
>> there will be inconsistency, the SQL statement will give error but
>> equivalent function set_config() will succeed.
>>
>> SQL Command
>> postgres=# set local search_path='public';
>> ERROR: SET LOCAL can only be used in transaction blocks
>>
>> Function
>> postgres=# select set_config('search_path', 'public', true);
>> set_config
>> ------------
>> public
>> (1 row)
>
> I looked at this but could not see how to easily pass the value of
> 'isTopLevel' down to the SELECT. All the other checks have isTopLevel
> passed down from the utility case statement.

Yes, we cannot pass isTopLevel, but as isTopLevel is used to decide
whether we are in function (user defined) call, so if we can find
during statement execution (current case set_config execution) that
current statement is inside user function execution, then it can be
handled.
For example, one of the ways could be to use a mechanism similar to
setting of user id and sec context used by fmgr_security_definer() (by
calling function SetUserIdAndSecContext()), once userid and sec
context are set by fmgr_security_definer(), later we can use
InSecurityRestrictedOperation() anywhere to give error.

For current case, what we can do is after analyze
(pg_analyze_and_rewrite), check if its not a builtin function (as we
can have functionid after analyze, so it can be checked
fmgr_isbuiltin(functionId)) and set variable to indicate that we are
in function call.

Any better or simpler idea can also be used to identify isTopLevel
during function execution.

Doing it only for detection of transaction chain in set_config path
might seem to be more work, but I think it can be used at other places
for detection of transaction chain as well.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Thu, Oct 3, 2013 at 11:50:09AM +0530, Amit Kapila wrote:
> > I looked at this but could not see how to easily pass the value of
> > 'isTopLevel' down to the SELECT. All the other checks have isTopLevel
> > passed down from the utility case statement.
>
> Yes, we cannot pass isTopLevel, but as isTopLevel is used to decide
> whether we are in function (user defined) call, so if we can find
> during statement execution (current case set_config execution) that
> current statement is inside user function execution, then it can be
> handled.
> For example, one of the ways could be to use a mechanism similar to
> setting of user id and sec context used by fmgr_security_definer() (by
> calling function SetUserIdAndSecContext()), once userid and sec
> context are set by fmgr_security_definer(), later we can use
> InSecurityRestrictedOperation() anywhere to give error.
>
> For current case, what we can do is after analyze
> (pg_analyze_and_rewrite), check if its not a builtin function (as we
> can have functionid after analyze, so it can be checked
> fmgr_isbuiltin(functionId)) and set variable to indicate that we are
> in function call.
>
> Any better or simpler idea can also be used to identify isTopLevel
> during function execution.
>
> Doing it only for detection of transaction chain in set_config path
> might seem to be more work, but I think it can be used at other places
> for detection of transaction chain as well.

I am also worried about over-engineering this. I will wait to see if
anyone else would find top-level detection useful, and if not, I will
just apply my version of that patch that does not handle set_config.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On 2013-09-30 22:19:31 -0400, Bruce Momjian wrote:
> On Sun, Sep 29, 2013 at 11:40:51AM +0530, Amit Kapila wrote:
> > >> Shouldn't we do it for Set Constraints as well?
> > >
> > > Oh, very good point. I missed that one. Updated patch attached.
>
> I am glad you are seeing things I am not. :-)
>
> > 1. The function set_config also needs similar functionality, else
> > there will be inconsistency, the SQL statement will give error but
> > equivalent function set_config() will succeed.
> >
> > SQL Command
> > postgres=# set local search_path='public';
> > ERROR: SET LOCAL can only be used in transaction blocks
> >
> > Function
> > postgres=# select set_config('search_path', 'public', true);
> > set_config
> > ------------
> > public
> > (1 row)
>
> I looked at this but could not see how to easily pass the value of
> 'isTopLevel' down to the SELECT. All the other checks have isTopLevel
> passed down from the utility case statement.

Doesn't sound like a good idea to prohibit that anyway, it might
intentionally be used as part of a more complex statement where it only
should take effect during that single statement.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Thu, Oct 3, 2013 at 8:35 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2013-09-30 22:19:31 -0400, Bruce Momjian wrote:
>> On Sun, Sep 29, 2013 at 11:40:51AM +0530, Amit Kapila wrote:
>> > >> Shouldn't we do it for Set Constraints as well?
>> > >
>> > > Oh, very good point. I missed that one. Updated patch attached.
>>
>> I am glad you are seeing things I am not. :-)
>>
>> > 1. The function set_config also needs similar functionality, else
>> > there will be inconsistency, the SQL statement will give error but
>> > equivalent function set_config() will succeed.
>> >
>> > SQL Command
>> > postgres=# set local search_path='public';
>> > ERROR: SET LOCAL can only be used in transaction blocks
>> >
>> > Function
>> > postgres=# select set_config('search_path', 'public', true);
>> > set_config
>> > ------------
>> > public
>> > (1 row)
>>
>> I looked at this but could not see how to easily pass the value of
>> 'isTopLevel' down to the SELECT. All the other checks have isTopLevel
>> passed down from the utility case statement.
>
> Doesn't sound like a good idea to prohibit that anyway, it might
> intentionally be used as part of a more complex statement where it only
> should take effect during that single statement.

Agreed and I think it is good reason for not changing behaviour of
set_config().

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Thu, Oct 3, 2013 at 8:32 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> On Thu, Oct 3, 2013 at 11:50:09AM +0530, Amit Kapila wrote:
>> > I looked at this but could not see how to easily pass the value of
>> > 'isTopLevel' down to the SELECT. All the other checks have isTopLevel
>> > passed down from the utility case statement.
>>
>> Yes, we cannot pass isTopLevel, but as isTopLevel is used to decide
>> whether we are in function (user defined) call, so if we can find
>> during statement execution (current case set_config execution) that
>> current statement is inside user function execution, then it can be
>> handled.
>> For example, one of the ways could be to use a mechanism similar to
>> setting of user id and sec context used by fmgr_security_definer() (by
>> calling function SetUserIdAndSecContext()), once userid and sec
>> context are set by fmgr_security_definer(), later we can use
>> InSecurityRestrictedOperation() anywhere to give error.
>>
>> For current case, what we can do is after analyze
>> (pg_analyze_and_rewrite), check if its not a builtin function (as we
>> can have functionid after analyze, so it can be checked
>> fmgr_isbuiltin(functionId)) and set variable to indicate that we are
>> in function call.
>>
>> Any better or simpler idea can also be used to identify isTopLevel
>> during function execution.
>>
>> Doing it only for detection of transaction chain in set_config path
>> might seem to be more work, but I think it can be used at other places
>> for detection of transaction chain as well.
>
> I am also worried about over-engineering this.

I had tried to think hard but could not come up with a simpler
change which could have handled all cases.
We can leave the handling for set_config() and proceed with patch
as Andres already given a reason where set_config() can be useful
within a
statement as well.

> I will wait to see if
> anyone else would find top-level detection useful, and if not, I will
> just apply my version of that patch that does not handle set_config.

I had verified the patch once again and ran regression, everything looks fine.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Fri, Oct 4, 2013 at 09:40:38AM +0530, Amit Kapila wrote:
> On Thu, Oct 3, 2013 at 8:35 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > On 2013-09-30 22:19:31 -0400, Bruce Momjian wrote:
> >> On Sun, Sep 29, 2013 at 11:40:51AM +0530, Amit Kapila wrote:
> >> > >> Shouldn't we do it for Set Constraints as well?
> >> > >
> >> > > Oh, very good point. I missed that one. Updated patch attached.
> >>
> >> I am glad you are seeing things I am not. :-)
> >>
> >> > 1. The function set_config also needs similar functionality, else
> >> > there will be inconsistency, the SQL statement will give error but
> >> > equivalent function set_config() will succeed.
> >> >
> >> > SQL Command
> >> > postgres=# set local search_path='public';
> >> > ERROR: SET LOCAL can only be used in transaction blocks
> >> >
> >> > Function
> >> > postgres=# select set_config('search_path', 'public', true);
> >> > set_config
> >> > ------------
> >> > public
> >> > (1 row)
> >>
> >> I looked at this but could not see how to easily pass the value of
> >> 'isTopLevel' down to the SELECT. All the other checks have isTopLevel
> >> passed down from the utility case statement.
> >
> > Doesn't sound like a good idea to prohibit that anyway, it might
> > intentionally be used as part of a more complex statement where it only
> > should take effect during that single statement.
>
> Agreed and I think it is good reason for not changing behaviour of
> set_config().

Applied. Thank you for all your suggestions.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

[ I'm so far behind ... ]

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> Applied. Thank you for all your suggestions.

I thought the suggestion had been to issue a *warning*. How did that
become an error? This patch seems likely to break applications that
may have just been harmlessly sloppy about when they were issuing
SETs and/or what flavor of SET they use. We don't for example throw
an error for START TRANSACTION with an open transaction or COMMIT or
ROLLBACK without one --- how can it possibly be argued that these
operations are more dangerous than those cases?

I'd personally have voted for using NOTICE.

regards, tom lane

On Fri, Nov 8, 2013 at 5:36 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> [ I'm so far behind ... ]
>
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
>> Applied. Thank you for all your suggestions.
>
> I thought the suggestion had been to issue a *warning*. How did that
> become an error? This patch seems likely to break applications that
> may have just been harmlessly sloppy about when they were issuing
> SETs and/or what flavor of SET they use. We don't for example throw
> an error for START TRANSACTION with an open transaction or COMMIT or
> ROLLBACK without one --- how can it possibly be argued that these
> operations are more dangerous than those cases?
>
> I'd personally have voted for using NOTICE.

Well, LOCK TABLE throws an error, so it's not without precedent.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Sat, Nov 9, 2013 at 02:15:52PM -0500, Robert Haas wrote:
> On Fri, Nov 8, 2013 at 5:36 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > [ I'm so far behind ... ]
> >
> > Bruce Momjian <bruce(at)momjian(dot)us> writes:
> >> Applied. Thank you for all your suggestions.
> >
> > I thought the suggestion had been to issue a *warning*. How did that
> > become an error? This patch seems likely to break applications that
> > may have just been harmlessly sloppy about when they were issuing
> > SETs and/or what flavor of SET they use. We don't for example throw
> > an error for START TRANSACTION with an open transaction or COMMIT or
> > ROLLBACK without one --- how can it possibly be argued that these
> > operations are more dangerous than those cases?
> >
> > I'd personally have voted for using NOTICE.
>
> Well, LOCK TABLE throws an error, so it's not without precedent.

Yeah, I just copied the LOCK TABLE usage. You could argue that SET
SESSION ISOLATION only affects later commands and doesn't do something
like LOCK, so it should be a warning. Not sure how to interpret the
COMMIT/ROLLBACK behavior you mentioned.

Considering we are doing this outside of a transaction, and WARNING or
ERROR is pretty much the same, from a behavioral perspective.

Should we change this and LOCK to be a warning?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

Bruce Momjian wrote
> Considering we are doing this outside of a transaction, and WARNING or
> ERROR is pretty much the same, from a behavioral perspective.
>
> Should we change this and LOCK to be a warning?

From the calling application's perspective an error and a warning are
definitely behaviorally different.

For this I'd vote for a warning (haven't pondered the LOCK scenario) as
using SET out of context means the user has a fairly serious
mis-understanding of the code path they have written (accedentially or
otherwise). Notice makes sense (speaking generally and without much
research here) for stuff where the ultimate outcome matches the statement
but the statement itself didn't actually do anything. Auto-sequence and
index generation fell into this but even notice was too noisy. In this case
we'd expect that the no-op statement was issued in error and thus should be
changed making a warning the level of incorrect-ness to communicate. A
notice would be more appropriate if there were valid use-cases for the user
doing this and we just want to make sure they are conscious of the
unusualness of the situation.

I dislike error for backward compatibility reasons. And saving the user
from this kind of mistake doesn't warrant breaking what could be properly
functioning code. Just because PostgreSQL isn't in a transaction does not
mean the client is expecting the current code to work correctly - even if by
accident - as part of a sequence of queries.

David J.

--
View this message in context: http://postgresql.1045698.n5.nabble.com/Suggestion-Issue-warning-when-calling-SET-TRANSACTION-outside-transaction-block-tp5743139p5778994.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.

On Mon, Nov 18, 2013 at 05:05:45PM -0800, David Johnston wrote:
> Bruce Momjian wrote
> > Considering we are doing this outside of a transaction, and WARNING or
> > ERROR is pretty much the same, from a behavioral perspective.
> >
> > Should we change this and LOCK to be a warning?
>
> >From the calling application's perspective an error and a warning are
> definitely behaviorally different.
>
> For this I'd vote for a warning (haven't pondered the LOCK scenario) as
> using SET out of context means the user has a fairly serious
> mis-understanding of the code path they have written (accedentially or
> otherwise). Notice makes sense (speaking generally and without much
> research here) for stuff where the ultimate outcome matches the statement
> but the statement itself didn't actually do anything. Auto-sequence and
> index generation fell into this but even notice was too noisy. In this case
> we'd expect that the no-op statement was issued in error and thus should be
> changed making a warning the level of incorrect-ness to communicate. A
> notice would be more appropriate if there were valid use-cases for the user
> doing this and we just want to make sure they are conscious of the
> unusualness of the situation.
>
> I dislike error for backward compatibility reasons. And saving the user
> from this kind of mistake doesn't warrant breaking what could be properly
> functioning code. Just because PostgreSQL isn't in a transaction does not
> mean the client is expecting the current code to work correctly - even if by
> accident - as part of a sequence of queries.

Well, ERROR is what LOCK returns, so if we change SET TRANSACTION to be
WARNING, we should change LOCK too, so on backward-compatibility
grounds, ERROR makes more sense.

Personally, I am fine with changing them all to WARNING.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

Bruce Momjian wrote
> On Mon, Nov 18, 2013 at 05:05:45PM -0800, David Johnston wrote:
>> Bruce Momjian wrote
>> > Considering we are doing this outside of a transaction, and WARNING or
>> > ERROR is pretty much the same, from a behavioral perspective.
>> >
>> > Should we change this and LOCK to be a warning?
>>
>> >From the calling application's perspective an error and a warning are
>> definitely behaviorally different.
>>
>> For this I'd vote for a warning (haven't pondered the LOCK scenario) as
>> using SET out of context means the user has a fairly serious
>> mis-understanding of the code path they have written (accedentially or
>> otherwise). Notice makes sense (speaking generally and without much
>> research here) for stuff where the ultimate outcome matches the statement
>> but the statement itself didn't actually do anything. Auto-sequence and
>> index generation fell into this but even notice was too noisy. In this
>> case
>> we'd expect that the no-op statement was issued in error and thus should
>> be
>> changed making a warning the level of incorrect-ness to communicate. A
>> notice would be more appropriate if there were valid use-cases for the
>> user
>> doing this and we just want to make sure they are conscious of the
>> unusualness of the situation.
>>
>> I dislike error for backward compatibility reasons. And saving the user
>> from this kind of mistake doesn't warrant breaking what could be properly
>> functioning code. Just because PostgreSQL isn't in a transaction does
>> not
>> mean the client is expecting the current code to work correctly - even if
>> by
>> accident - as part of a sequence of queries.
>
> Well, ERROR is what LOCK returns, so if we change SET TRANSACTION to be
> WARNING, we should change LOCK too, so on backward-compatibility
> grounds, ERROR makes more sense.
>
> Personally, I am fine with changing them all to WARNING.

Error makes more sense if the goal is internal consistency. That goal
should be subservient to backward compatibility. Changing LOCK to warning
is less problematic since the likelihood of current code functioning in such
a way that after upgrade it would begin working differently in the absence
of an error does not seem probable. Basically someone would have be
trapping on the error and conditionally branching their logic.

That said, if this was a day 0 decision I'd likely raise an error.
Weakening LOCK doesn't make sense since it is day 0 behavior. Document the
warning for SET as being weaker than ideal because of backward compatibility
and call it a day (i.e. leave LOCK at error). The documentation, not the
code, then enforces the feeling that such usage is considered wrong without
possibly breaking wrong but working code.

David J.

--
View this message in context: http://postgresql.1045698.n5.nabble.com/Suggestion-Issue-warning-when-calling-SET-TRANSACTION-outside-transaction-block-tp5743139p5779006.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.

On Mon, Nov 18, 2013 at 06:30:32PM -0800, David Johnston wrote:
> > Personally, I am fine with changing them all to WARNING.
>
> Error makes more sense if the goal is internal consistency. That goal
> should be subservient to backward compatibility. Changing LOCK to warning
> is less problematic since the likelihood of current code functioning in such
> a way that after upgrade it would begin working differently in the absence
> of an error does not seem probable. Basically someone would have be
> trapping on the error and conditionally branching their logic.
>
> That said, if this was a day 0 decision I'd likely raise an error.
> Weakening LOCK doesn't make sense since it is day 0 behavior. Document the
> warning for SET as being weaker than ideal because of backward compatibility
> and call it a day (i.e. leave LOCK at error). The documentation, not the
> code, then enforces the feeling that such usage is considered wrong without
> possibly breaking wrong but working code.

We normally don't approach warts with documentation --- we usually just
fix them and document them in the release notes. If we did, our docs
would be a whole lot uglier.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

Bruce Momjian wrote
> On Mon, Nov 18, 2013 at 06:30:32PM -0800, David Johnston wrote:
>> > Personally, I am fine with changing them all to WARNING.
>>
>> Error makes more sense if the goal is internal consistency. That goal
>> should be subservient to backward compatibility. Changing LOCK to
>> warning
>> is less problematic since the likelihood of current code functioning in
>> such
>> a way that after upgrade it would begin working differently in the
>> absence
>> of an error does not seem probable. Basically someone would have be
>> trapping on the error and conditionally branching their logic.
>>
>> That said, if this was a day 0 decision I'd likely raise an error.
>> Weakening LOCK doesn't make sense since it is day 0 behavior. Document
>> the
>> warning for SET as being weaker than ideal because of backward
>> compatibility
>> and call it a day (i.e. leave LOCK at error). The documentation, not the
>> code, then enforces the feeling that such usage is considered wrong
>> without
>> possibly breaking wrong but working code.
>
> We normally don't approach warts with documentation --- we usually just
> fix them and document them in the release notes. If we did, our docs
> would be a whole lot uglier.

That is a fair point - though it may be that this instance needs to be one
of those "usually" exceptions.

For any sane use-case turning this into an error shouldn't cause any grief;
and those cases where there is grief should be evaluated and changed anyway.

I could honestly live with either change to "SET TRANSACTION" but regardless
would leave "LOCK" as-is. The backward compatibility concern, while valid,
does indeed seem weak and worth breaking in order to maintain a consistent
ABI going forward.

David J.

--
View this message in context: http://postgresql.1045698.n5.nabble.com/Suggestion-Issue-warning-when-calling-SET-TRANSACTION-outside-transaction-block-tp5743139p5779028.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.

On Mon, Nov 18, 2013 at 9:07 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> Well, ERROR is what LOCK returns, so if we change SET TRANSACTION to be
> WARNING, we should change LOCK too, so on backward-compatibility
> grounds, ERROR makes more sense.
>
> Personally, I am fine with changing them all to WARNING.

I don't think it's worth breaking backward compatibility. I'm not
entirely sure what I would have decided here in a vacuum, but at this
point existing precedent seems determinative.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas wrote
> On Mon, Nov 18, 2013 at 9:07 PM, Bruce Momjian &lt;

> bruce@

> &gt; wrote:
>> Well, ERROR is what LOCK returns, so if we change SET TRANSACTION to be
>> WARNING, we should change LOCK too, so on backward-compatibility
>> grounds, ERROR makes more sense.
>>
>> Personally, I am fine with changing them all to WARNING.
>
> I don't think it's worth breaking backward compatibility. I'm not
> entirely sure what I would have decided here in a vacuum, but at this
> point existing precedent seems determinative.

Well, at this point we have already broken backward compatibility by
releasing this. With Tom's thread necromancy I missed the fact this got
released in 9.3

Now, given normal upgrade realities the people likely to have this bite them
probably are a ways out from upgrading so I wouldn't expect to have seen
many complaints yet - but at the same time I do not recall seeing any
complaints yet (limited to -bugs and -general)

The referenced patch:

is released
is documented
is consistent with precedent established by similar codepaths
causes an obvious error in what is considered broken code
can be trivially corrected by a user willing and able to update their
application

I'd say leave this as-is and only re-evaluate the decision if complaints are
brought forth.

David J.

--
View this message in context: http://postgresql.1045698.n5.nabble.com/Suggestion-Issue-warning-when-calling-SET-TRANSACTION-outside-transaction-block-tp5743139p5779170.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.

On Tue, Nov 19, 2013 at 11:53 AM, David Johnston <polobo(at)yahoo(dot)com> wrote:
> Well, at this point we have already broken backward compatibility by
> releasing this. With Tom's thread necromancy I missed the fact this got
> released in 9.3

Eh, really? I don't see it in 9.3.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

David Johnston <polobo(at)yahoo(dot)com> writes:
> Robert Haas wrote
>> I don't think it's worth breaking backward compatibility. I'm not
>> entirely sure what I would have decided here in a vacuum, but at this
>> point existing precedent seems determinative.

> Well, at this point we have already broken backward compatibility by
> releasing this. With Tom's thread necromancy I missed the fact this got
> released in 9.3

Uh, what? The commit I'm objecting to is certainly not in 9.3.
It's this one:

Author: Bruce Momjian <bruce(at)momjian(dot)us>
Branch: master [a54141aeb] 2013-10-04 13:50:28 -0400

Issue error on SET outside transaction block in some cases

Issue error for SET LOCAL/CONSTRAINTS/TRANSACTION outside a transaction
block, as they have no effect.

Per suggestion from Morten Hustveit

I agree that it's too late to reconsider the behavior of pre-existing
cases such as LOCK TABLE, but that doesn't mean I can't complain about
this one.

regards, tom lane

On Sat, Nov 9, 2013 at 02:15:52PM -0500, Robert Haas wrote:
> On Fri, Nov 8, 2013 at 5:36 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > [ I'm so far behind ... ]
> >
> > Bruce Momjian <bruce(at)momjian(dot)us> writes:
> >> Applied. Thank you for all your suggestions.
> >
> > I thought the suggestion had been to issue a *warning*. How did that
> > become an error? This patch seems likely to break applications that
> > may have just been harmlessly sloppy about when they were issuing
> > SETs and/or what flavor of SET they use. We don't for example throw
> > an error for START TRANSACTION with an open transaction or COMMIT or
> > ROLLBACK without one --- how can it possibly be argued that these
> > operations are more dangerous than those cases?
> >
> > I'd personally have voted for using NOTICE.
>
> Well, LOCK TABLE throws an error, so it's not without precedent.

OK, I dug into all cases where commands that are meaningless outside of
transactions currently throw an error; they are:

DECLARE
LOCK
ROLLBACK TO SAVEPOINT
SET LOCAL*
SET CONSTRAINTS*
SET TRANSACTION*
SAVEPOINT

The stared items are new in 9.4. Here is the current behavior:

test=> LOCK lkjasdf;
ERROR: LOCK TABLE can only be used in transaction blocks
test=> SET LOCAL x = 3;
ERROR: SET LOCAL can only be used in transaction blocks
test=> SAVEPOINT lkjsafd;
ERROR: SAVEPOINT can only be used in transaction blocks
test=> ROLLBACK TO SAVEPOINT asdf;
ERROR: ROLLBACK TO SAVEPOINT can only be used in transaction blocks

Notice that they do _not_ check their arguments; they just throw
errors. With this patch they issue warnings and evaluate their
arguments:

test=> LOCK lkjasdf;
WARNING: LOCK TABLE can only be used in transaction blocks
ERROR: relation "lkjasdf" does not exist
test=> SET LOCAL x = 3;
WARNING: SET LOCAL can only be used in transaction blocks
ERROR: unrecognized configuration parameter "x"
test=> SAVEPOINT lkjsafd;
WARNING: SAVEPOINT can only be used in transaction blocks
SAVEPOINT
test=> ROLLBACK TO SAVEPOINT asdf;
WARNING: ROLLBACK TO SAVEPOINT can only be used in transaction blocks
ROLLBACK

However, SAVEPOINT/ROLLBACK throw weird errors when they are evaluated
outside a multi-statement transaction, so their arguments are not
evaluated. This might be why they were originally marked as errors.

A patch to issue only warnings is attached. In a way this change
improves the code by throwing errors only when the commands are invalid,
rather than just useless. You could argue that ROLLBACK TO SAVEPOINT
should throw an error because no savepoint name is valid in that
context.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

On Tue, Nov 19, 2013 at 12:24:50PM -0500, Tom Lane wrote:
> David Johnston <polobo(at)yahoo(dot)com> writes:
> > Robert Haas wrote
> >> I don't think it's worth breaking backward compatibility. I'm not
> >> entirely sure what I would have decided here in a vacuum, but at this
> >> point existing precedent seems determinative.
>
> > Well, at this point we have already broken backward compatibility by
> > releasing this. With Tom's thread necromancy I missed the fact this got
> > released in 9.3
>
> Uh, what? The commit I'm objecting to is certainly not in 9.3.
> It's this one:

Right.

> Author: Bruce Momjian <bruce(at)momjian(dot)us>
> Branch: master [a54141aeb] 2013-10-04 13:50:28 -0400
>
> Issue error on SET outside transaction block in some cases
>
> Issue error for SET LOCAL/CONSTRAINTS/TRANSACTION outside a transaction
> block, as they have no effect.
>
> Per suggestion from Morten Hustveit
>
> I agree that it's too late to reconsider the behavior of pre-existing
> cases such as LOCK TABLE, but that doesn't mean I can't complain about
> this one.

OK, so I just posted a summary of what we have now, and a patch that
switches them all to warning. Are you saying we should just switch the
new ones to warnings?

Seeing as these commands have always been useless, I don't see the big
argument for backward compatibility myself.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

On 2013-11-19 13:05:01 -0500, Bruce Momjian wrote:
> SAVEPOINT

> test=> ROLLBACK TO SAVEPOINT asdf;
> ERROR: ROLLBACK TO SAVEPOINT can only be used in transaction blocks
>
> Notice that they do _not_ check their arguments; they just throw
> errors. With this patch they issue warnings and evaluate their
> arguments:

> test=> ROLLBACK TO SAVEPOINT asdf;
> WARNING: ROLLBACK TO SAVEPOINT can only be used in transaction blocks
> ROLLBACK
>
> However, SAVEPOINT/ROLLBACK throw weird errors when they are evaluated
> outside a multi-statement transaction, so their arguments are not
> evaluated. This might be why they were originally marked as errors.

Why change the historical behaviour for savepoints?

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Tue, Nov 19, 2013 at 07:08:05PM +0100, Andres Freund wrote:
> On 2013-11-19 13:05:01 -0500, Bruce Momjian wrote:
> > SAVEPOINT
>
> > test=> ROLLBACK TO SAVEPOINT asdf;
> > ERROR: ROLLBACK TO SAVEPOINT can only be used in transaction blocks
> >
> > Notice that they do _not_ check their arguments; they just throw
> > errors. With this patch they issue warnings and evaluate their
> > arguments:
>
> > test=> ROLLBACK TO SAVEPOINT asdf;
> > WARNING: ROLLBACK TO SAVEPOINT can only be used in transaction blocks
> > ROLLBACK
> >
> > However, SAVEPOINT/ROLLBACK throw weird errors when they are evaluated
> > outside a multi-statement transaction, so their arguments are not
> > evaluated. This might be why they were originally marked as errors.
>
> Why change the historical behaviour for savepoints?

Because as Tom stated, we already do warnings for other useless
transaction commands like BEGIN WORK inside a transaction block:

test=> begin work;
BEGIN
test=> begin work;
--> WARNING: there is already a transaction in progress
BEGIN
test=> commit;
COMMIT
test=>

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

On Tue, Nov 19, 2013 at 1:05 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> A patch to issue only warnings is attached. In a way this change
> improves the code by throwing errors only when the commands are invalid,
> rather than just useless. You could argue that ROLLBACK TO SAVEPOINT
> should throw an error because no savepoint name is valid in that
> context.

-1 from me. I don't see this as a step forward in any way. The
output is a complete muddle, and it's not solving any problem that I
can see.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2013-11-19 13:09:16 -0500, Bruce Momjian wrote:
> > Why change the historical behaviour for savepoints?
>
> Because as Tom stated, we already do warnings for other useless
> transaction commands like BEGIN WORK inside a transaction block:

Which imo is a bad, bad historical accident. I've repeatedly seen this
hide bugs causing corrupted data in the end.

But even if that weren't a concern, the fact that BEGIN does it one way
currently doesn't seem very indicative of changing other historical behaviour.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Tue, Nov 19, 2013 at 07:12:32PM +0100, Andres Freund wrote:
> On 2013-11-19 13:09:16 -0500, Bruce Momjian wrote:
> > > Why change the historical behaviour for savepoints?
> >
> > Because as Tom stated, we already do warnings for other useless
> > transaction commands like BEGIN WORK inside a transaction block:
>
> Which imo is a bad, bad historical accident. I've repeatedly seen this
> hide bugs causing corrupted data in the end.
>
> But even if that weren't a concern, the fact that BEGIN does it one way
> currently doesn't seem very indicative of changing other historical behaviour.

Look at this gem, which returns notice:

test=> ABORT;
NOTICE: there is no transaction in progress
ROLLBACK
test=>

We are all over the map on this! The big question is whether we want to
add some sanity to this, or just leave it alone, and if we leave it
alone, what pattern do we use for the new checks?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

Tom Lane-2 wrote
> David Johnston &lt;

> polobo@

> &gt; writes:
>> Robert Haas wrote
>>> I don't think it's worth breaking backward compatibility. I'm not
>>> entirely sure what I would have decided here in a vacuum, but at this
>>> point existing precedent seems determinative.
>
>> Well, at this point we have already broken backward compatibility by
>> releasing this. With Tom's thread necromancy I missed the fact this got
>> released in 9.3
>
> Uh, what? The commit I'm objecting to is certainly not in 9.3.
> It's this one:
>
> Author: Bruce Momjian &lt;

> bruce@

> &gt;
> Branch: master [a54141aeb] 2013-10-04 13:50:28 -0400
>
> Issue error on SET outside transaction block in some cases
>
> Issue error for SET LOCAL/CONSTRAINTS/TRANSACTION outside a
> transaction
> block, as they have no effect.
>
> Per suggestion from Morten Hustveit
>
> I agree that it's too late to reconsider the behavior of pre-existing
> cases such as LOCK TABLE, but that doesn't mean I can't complain about
> this one.

My bad, I was relaying an assertion without checking it myself. I believe
my source meant 9.4/head and simply mis-typed 9.3 which I then copied.

David J.

--
View this message in context: http://postgresql.1045698.n5.nabble.com/Suggestion-Issue-warning-when-calling-SET-TRANSACTION-outside-transaction-block-tp5743139p5779205.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.

On 2013-11-19 13:14:34 -0500, Bruce Momjian wrote:
> On Tue, Nov 19, 2013 at 07:12:32PM +0100, Andres Freund wrote:
> > But even if that weren't a concern, the fact that BEGIN does it one way
> > currently doesn't seem very indicative of changing other historical behaviour.
>
> Look at this gem, which returns notice:
>
> test=> ABORT;
> NOTICE: there is no transaction in progress
> ROLLBACK
> test=>
>
> We are all over the map on this! The big question is whether we want to
> add some sanity to this, or just leave it alone, and if we leave it
> alone, what pattern do we use for the new checks?

I think changing a NOTICE into a WARNING or the reverse is fine,
changing a WARNING/NOTICE into an ERROR or the reverse is something
which should be done only very hesitantly.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Tue, Nov 19, 2013 at 1:14 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> On Tue, Nov 19, 2013 at 07:12:32PM +0100, Andres Freund wrote:
>> On 2013-11-19 13:09:16 -0500, Bruce Momjian wrote:
>> > > Why change the historical behaviour for savepoints?
>> >
>> > Because as Tom stated, we already do warnings for other useless
>> > transaction commands like BEGIN WORK inside a transaction block:
>>
>> Which imo is a bad, bad historical accident. I've repeatedly seen this
>> hide bugs causing corrupted data in the end.
>>
>> But even if that weren't a concern, the fact that BEGIN does it one way
>> currently doesn't seem very indicative of changing other historical behaviour.
>
> Look at this gem, which returns notice:
>
> test=> ABORT;
> NOTICE: there is no transaction in progress
> ROLLBACK
> test=>
>
> We are all over the map on this! The big question is whether we want to
> add some sanity to this, or just leave it alone, and if we leave it
> alone, what pattern do we use for the new checks?

I think the pattern is and should be different for toplevel
transaction control commands than for other things. If you issue a
BEGIN, we want it to end up that you're definitely in a transaction at
that point, and if you issue a COMMIT or ROLLBACK or ABORT, we want
you to definitely be out of a transaction after that. This is
important for reasons discussed on Andrew's thread about pre-commit
triggers just today.

The same considerations don't apply elsewhere; the user has made a
mistake, and there's no particular reason not to throw an ERROR. We
could throw a WARNING or NOTICE and pretend like things are OK, but
there doesn't seem to be much point, certainly not enough to justify
changing long-established behavior.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Nov 19, 2013 at 01:20:47PM -0500, Robert Haas wrote:
> I think the pattern is and should be different for toplevel
> transaction control commands than for other things. If you issue a
> BEGIN, we want it to end up that you're definitely in a transaction at
> that point, and if you issue a COMMIT or ROLLBACK or ABORT, we want
> you to definitely be out of a transaction after that. This is
> important for reasons discussed on Andrew's thread about pre-commit
> triggers just today.
>
> The same considerations don't apply elsewhere; the user has made a
> mistake, and there's no particular reason not to throw an ERROR. We
> could throw a WARNING or NOTICE and pretend like things are OK, but
> there doesn't seem to be much point, certainly not enough to justify
> changing long-established behavior.

OK, what I am hearing you say is that we should change ABORT from NOTICE
to WARNING, leave SAVEPOINT/ROLLBACK TO SAVEPOINT as WARNING (so all
transaction control commands are warnings), and leave the new SET
commands as ERRORs. Works for me.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

On Tue, Nov 19, 2013 at 01:31:55PM -0500, Bruce Momjian wrote:
> On Tue, Nov 19, 2013 at 01:20:47PM -0500, Robert Haas wrote:
> > I think the pattern is and should be different for toplevel
> > transaction control commands than for other things. If you issue a
> > BEGIN, we want it to end up that you're definitely in a transaction at
> > that point, and if you issue a COMMIT or ROLLBACK or ABORT, we want
> > you to definitely be out of a transaction after that. This is
> > important for reasons discussed on Andrew's thread about pre-commit
> > triggers just today.
> >
> > The same considerations don't apply elsewhere; the user has made a
> > mistake, and there's no particular reason not to throw an ERROR. We
> > could throw a WARNING or NOTICE and pretend like things are OK, but
> > there doesn't seem to be much point, certainly not enough to justify
> > changing long-established behavior.
>
> OK, what I am hearing you say is that we should change ABORT from NOTICE
> to WARNING, leave SAVEPOINT/ROLLBACK TO SAVEPOINT as WARNING (so all
> transaction control commands are warnings), and leave the new SET
> commands as ERRORs. Works for me.

Sorry, even I am getting confused. SAVEPOINT/ROLLBACK TO SAVEPOINT stay
as ERROR, so effectively only top-level transaction control commands
BEGIN WORK/ABORT/COMMIT are WARNINGS.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

On Tue, Nov 19, 2013 at 01:37:56PM -0500, Bruce Momjian wrote:
> On Tue, Nov 19, 2013 at 01:31:55PM -0500, Bruce Momjian wrote:
> > On Tue, Nov 19, 2013 at 01:20:47PM -0500, Robert Haas wrote:
> > > I think the pattern is and should be different for toplevel
> > > transaction control commands than for other things. If you issue a
> > > BEGIN, we want it to end up that you're definitely in a transaction at
> > > that point, and if you issue a COMMIT or ROLLBACK or ABORT, we want
> > > you to definitely be out of a transaction after that. This is
> > > important for reasons discussed on Andrew's thread about pre-commit
> > > triggers just today.
> > >
> > > The same considerations don't apply elsewhere; the user has made a
> > > mistake, and there's no particular reason not to throw an ERROR. We
> > > could throw a WARNING or NOTICE and pretend like things are OK, but
> > > there doesn't seem to be much point, certainly not enough to justify
> > > changing long-established behavior.
> >
> > OK, what I am hearing you say is that we should change ABORT from NOTICE
> > to WARNING, leave SAVEPOINT/ROLLBACK TO SAVEPOINT as WARNING (so all
> > transaction control commands are warnings), and leave the new SET
> > commands as ERRORs. Works for me.
>
> Sorry, even I am getting confused. SAVEPOINT/ROLLBACK TO SAVEPOINT stay
> as ERROR, so effectively only top-level transaction control commands
> BEGIN WORK/ABORT/COMMIT are WARNINGS.

Does anyone know if this C comment justifies why ABORT is a NOTICE and
not WARNING?

/*
* The user issued ABORT when not inside a transaction. Issue a
* NOTICE and go to abort state. The upcoming call to
* CommitTransactionCommand() will then put us back into the
* default state.
*/

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> On 2013-11-19 13:09:16 -0500, Bruce Momjian wrote:
>> Because as Tom stated, we already do warnings for other useless
>> transaction commands like BEGIN WORK inside a transaction block:
>
> Which imo is a bad, bad historical accident. I've repeatedly seen this
> hide bugs causing corrupted data in the end.

+1

--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> Does anyone know if this C comment justifies why ABORT is a NOTICE and
> not WARNING?

> /*
> * The user issued ABORT when not inside a transaction. Issue a
> * NOTICE and go to abort state. The upcoming call to
> * CommitTransactionCommand() will then put us back into the
> * default state.
> */

It's just describing the implementation, not defending the design choice.

My personal standpoint is that I don't care much whether these messages
are NOTICE or WARNING. What I'm not happy about is promoting cases that
have been non-error conditions for years into ERRORs.

Now, if we wanted to go the other way (downgrade some ERRORs to WARNINGs),
that would not create an application compatibility problem in my view.

And on the third hand, there's Emerson's excellent advice: "A foolish
consistency is the hobgoblin of little minds". I'm not convinced that
trying to make all these cases have the same message level is actually
a good goal. It seems entirely plausible that some are more dangerous
than others.

regards, tom lane

On Tue, Nov 19, 2013 at 10:21:47PM -0500, Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > Does anyone know if this C comment justifies why ABORT is a NOTICE and
> > not WARNING?
>
> > /*
> > * The user issued ABORT when not inside a transaction. Issue a
> > * NOTICE and go to abort state. The upcoming call to
> > * CommitTransactionCommand() will then put us back into the
> > * default state.
> > */
>
> It's just describing the implementation, not defending the design choice.
>
> My personal standpoint is that I don't care much whether these messages
> are NOTICE or WARNING. What I'm not happy about is promoting cases that
> have been non-error conditions for years into ERRORs.

I don't remember any cases where that was suggested.

> Now, if we wanted to go the other way (downgrade some ERRORs to WARNINGs),
> that would not create an application compatibility problem in my view.

Yes, that was my initial plan, but many didn't like it.

> And on the third hand, there's Emerson's excellent advice: "A foolish
> consistency is the hobgoblin of little minds". I'm not convinced that
> trying to make all these cases have the same message level is actually
> a good goal. It seems entirely plausible that some are more dangerous
> than others.

The attached patch changes ABORT from NOTICE to WARNING, and documents
that all other are errors. This "top-level" logic idea came from Robert
Haas, and it has some level of consistency.

Interesting that ABORT was already documented as returning a warning:

Issuing <command>ABORT</> when not inside a transaction does
no harm, but it will provoke a warning message.
-------

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> On Tue, Nov 19, 2013 at 10:21:47PM -0500, Tom Lane wrote:
>> My personal standpoint is that I don't care much whether these messages
>> are NOTICE or WARNING. What I'm not happy about is promoting cases that
>> have been non-error conditions for years into ERRORs.

> I don't remember any cases where that was suggested.

Apparently you've forgotten the commit that was the subject of this
thread. You took a bunch of SET cases that we've always accepted
without any complaint whatsoever, and made them into ERRORs, thereby
breaking any applications that might've expected such usage to be
harmless. I would be okay if that patch had issued WARNINGs, which
as you can see from the thread title was the original suggestion.

> The attached patch changes ABORT from NOTICE to WARNING, and documents
> that all other are errors. This "top-level" logic idea came from Robert
> Haas, and it has some level of consistency.

This patch utterly fails to address my complaint.

More to the point, I think it's a waste of time to make these sorts of
adjustments. The only thanks you're likely to get for it is complaints
about cross-version behavioral changes. Also, you're totally ignoring
the thought that these different message levels might've been selected
intentionally, back when the code was written. Since there have been
no field complaints about the inconsistency, what's your hurry to
change it? See Emerson.

regards, tom lane

On Wed, Nov 20, 2013 at 10:04:22AM -0500, Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > On Tue, Nov 19, 2013 at 10:21:47PM -0500, Tom Lane wrote:
> >> My personal standpoint is that I don't care much whether these messages
> >> are NOTICE or WARNING. What I'm not happy about is promoting cases that
> >> have been non-error conditions for years into ERRORs.
>
> > I don't remember any cases where that was suggested.
>
> Apparently you've forgotten the commit that was the subject of this
> thread. You took a bunch of SET cases that we've always accepted
> without any complaint whatsoever, and made them into ERRORs, thereby
> breaking any applications that might've expected such usage to be
> harmless. I would be okay if that patch had issued WARNINGs, which
> as you can see from the thread title was the original suggestion.

Oh, those changes. I thought we were just looking at _additional_
changes.

> > The attached patch changes ABORT from NOTICE to WARNING, and documents
> > that all other are errors. This "top-level" logic idea came from Robert
> > Haas, and it has some level of consistency.
>
> This patch utterly fails to address my complaint.
>
> More to the point, I think it's a waste of time to make these sorts of
> adjustments. The only thanks you're likely to get for it is complaints
> about cross-version behavioral changes. Also, you're totally ignoring
> the thought that these different message levels might've been selected
> intentionally, back when the code was written. Since there have been
> no field complaints about the inconsistency, what's your hurry to
> change it? See Emerson.

My problem was that they issued _no_ message at all. I am fine with
them issuing a warning if that's what people prefer. As they are all
SET commands, they will be consistent.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

On Wed, Nov 20, 2013 at 10:16:00AM -0500, Bruce Momjian wrote:
> > > The attached patch changes ABORT from NOTICE to WARNING, and documents
> > > that all other are errors. This "top-level" logic idea came from Robert
> > > Haas, and it has some level of consistency.
> >
> > This patch utterly fails to address my complaint.
> >
> > More to the point, I think it's a waste of time to make these sorts of
> > adjustments. The only thanks you're likely to get for it is complaints
> > about cross-version behavioral changes. Also, you're totally ignoring
> > the thought that these different message levels might've been selected
> > intentionally, back when the code was written. Since there have been
> > no field complaints about the inconsistency, what's your hurry to
> > change it? See Emerson.
>
> My problem was that they issued _no_ message at all. I am fine with
> them issuing a warning if that's what people prefer. As they are all
> SET commands, they will be consistent.

OK, here is a patch which changes ABORT from NOTICE to WARNING, and SET
from ERROR (which is new in 9.4) to WARNING.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

On Wed, Nov 20, 2013 at 4:19 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> On Wed, Nov 20, 2013 at 10:16:00AM -0500, Bruce Momjian wrote:
>> > > The attached patch changes ABORT from NOTICE to WARNING, and documents
>> > > that all other are errors. This "top-level" logic idea came from Robert
>> > > Haas, and it has some level of consistency.
>> >
>> > This patch utterly fails to address my complaint.
>> >
>> > More to the point, I think it's a waste of time to make these sorts of
>> > adjustments. The only thanks you're likely to get for it is complaints
>> > about cross-version behavioral changes. Also, you're totally ignoring
>> > the thought that these different message levels might've been selected
>> > intentionally, back when the code was written. Since there have been
>> > no field complaints about the inconsistency, what's your hurry to
>> > change it? See Emerson.
>>
>> My problem was that they issued _no_ message at all. I am fine with
>> them issuing a warning if that's what people prefer. As they are all
>> SET commands, they will be consistent.
>
> OK, here is a patch which changes ABORT from NOTICE to WARNING, and SET
> from ERROR (which is new in 9.4) to WARNING.

Well, Tom and I are on opposite sides of this, I suppose. I prefer
ERROR for everything other than the top-level transaction commands,
and see no benefit from opting for a wishy-washy warning.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Nov 20, 2013 at 04:31:12PM -0500, Robert Haas wrote:
> On Wed, Nov 20, 2013 at 4:19 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> > On Wed, Nov 20, 2013 at 10:16:00AM -0500, Bruce Momjian wrote:
> >> > > The attached patch changes ABORT from NOTICE to WARNING, and documents
> >> > > that all other are errors. This "top-level" logic idea came from Robert
> >> > > Haas, and it has some level of consistency.
> >> >
> >> > This patch utterly fails to address my complaint.
> >> >
> >> > More to the point, I think it's a waste of time to make these sorts of
> >> > adjustments. The only thanks you're likely to get for it is complaints
> >> > about cross-version behavioral changes. Also, you're totally ignoring
> >> > the thought that these different message levels might've been selected
> >> > intentionally, back when the code was written. Since there have been
> >> > no field complaints about the inconsistency, what's your hurry to
> >> > change it? See Emerson.
> >>
> >> My problem was that they issued _no_ message at all. I am fine with
> >> them issuing a warning if that's what people prefer. As they are all
> >> SET commands, they will be consistent.
> >
> > OK, here is a patch which changes ABORT from NOTICE to WARNING, and SET
> > from ERROR (which is new in 9.4) to WARNING.
>
> Well, Tom and I are on opposite sides of this, I suppose. I prefer
> ERROR for everything other than the top-level transaction commands,
> and see no benefit from opting for a wishy-washy warning.

Well, the only way I can process this is to think of psql with
ON_ERROR_STOP enabled. Would we want a no-op command to abort psql? I
can see logic that top-level transaction commands and SET to not, but
other commands do. I can also see them all aborting psql, or none of
them. :-(

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> Well, Tom and I are on opposite sides of this, I suppose.  I
> prefer ERROR for everything other than the top-level transaction
> commands, and see no benefit from opting for a wishy-washy
> warning.

+1

If the user issued a local command outside of a transaction there
is an extremely high probability that they intended to either set
session state or affect the next transaction.  Either way,
modifying the database with the wrong setting could lead to data
corruption -- at least for some of these settings.  IMO it would be
foolish to insist on consistency with prior releases rather than on
giving people the best shot at preventing, or at least promptly
identifying the cause of, data corruption.

Obviously, changing the level of these is not material for back-
patching.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Bruce Momjian escribió:

> OK, here is a patch which changes ABORT from NOTICE to WARNING, and SET
> from ERROR (which is new in 9.4) to WARNING.

I don't like that this patch changes RequireTransactionChain() from
actually requiring one, to a function that maybe requires a transaction
chain, and maybe it only complains about there not being one. I mean,
it's like you had named the new throwError boolean as "notReally" or
something like that. Also, the new comment paragraph is bad because it
explains who must pass true/false, instead of what's the effect of each
value (and let the callers choose which value to pass).

I would create a separate function to implement this, maybe
WarnUnlessInTransactionBlock() or something like that. That would make
the patch a good deal smaller (because not changing existing callers).

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Fri, Nov 22, 2013 at 10:24:35AM -0300, Alvaro Herrera wrote:
> Bruce Momjian escribió:
>
> > OK, here is a patch which changes ABORT from NOTICE to WARNING, and SET
> > from ERROR (which is new in 9.4) to WARNING.
>
> I don't like that this patch changes RequireTransactionChain() from
> actually requiring one, to a function that maybe requires a transaction
> chain, and maybe it only complains about there not being one. I mean,
> it's like you had named the new throwError boolean as "notReally" or
> something like that. Also, the new comment paragraph is bad because it
> explains who must pass true/false, instead of what's the effect of each
> value (and let the callers choose which value to pass).
>
> I would create a separate function to implement this, maybe
> WarnUnlessInTransactionBlock() or something like that. That would make
> the patch a good deal smaller (because not changing existing callers).

Good points. I have modified the attached patch to do as you suggested.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

On Fri, Nov 22, 2013 at 12:17:41PM -0500, Bruce Momjian wrote:
> Good points. I have modified the attached patch to do as you suggested.

Also, I have read through the thread and summarized the positions of the
posters:

9.3 WARNING ERROR
SET none Tom, DavidJ, AndresF Robert, Kevin
SAVEPOINT error Tom, DavidJ, Robert, AndresF, Kevin
LOCK, DECLARE error Tom, DavidJ, Robert, AndresF, Kevin

Everyone seems to agree that SAVEPOINT, LOCK, and DECLARE should remain
as errors. Everyone also seems to agree that BEGIN and COMMIT should
remain warnings, and ABORT should be changed from notice to warning.

Our only disagreement seems to be how to handle the SET commands, which
used to report nothing. Would anyone else like to correct or express an
opinion? Given the current vote count and backward-compatibility,
warning seems to be the direction we are heading.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

On Fri, Nov 22, 2013 at 01:19:55PM -0500, Bruce Momjian wrote:
> On Fri, Nov 22, 2013 at 12:17:41PM -0500, Bruce Momjian wrote:
> > Good points. I have modified the attached patch to do as you suggested.
>
> Also, I have read through the thread and summarized the positions of the
> posters:
>
> 9.3 WARNING ERROR
> SET none Tom, DavidJ, AndresF Robert, Kevin
> SAVEPOINT error Tom, DavidJ, Robert, AndresF, Kevin
> LOCK, DECLARE error Tom, DavidJ, Robert, AndresF, Kevin
>
> Everyone seems to agree that SAVEPOINT, LOCK, and DECLARE should remain
> as errors. Everyone also seems to agree that BEGIN and COMMIT should
> remain warnings, and ABORT should be changed from notice to warning.
>
> Our only disagreement seems to be how to handle the SET commands, which
> used to report nothing. Would anyone else like to correct or express an
> opinion? Given the current vote count and backward-compatibility,
> warning seems to be the direction we are heading.

Patch applied.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

On Mon, Nov 25, 2013 at 7:19 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> On Fri, Nov 22, 2013 at 01:19:55PM -0500, Bruce Momjian wrote:
>> On Fri, Nov 22, 2013 at 12:17:41PM -0500, Bruce Momjian wrote:
>> > Good points. I have modified the attached patch to do as you suggested.
>>
>> Also, I have read through the thread and summarized the positions of the
>> posters:
>>
>> 9.3 WARNING ERROR
>> SET none Tom, DavidJ, AndresF Robert, Kevin
>> SAVEPOINT error Tom, DavidJ, Robert, AndresF, Kevin
>> LOCK, DECLARE error Tom, DavidJ, Robert, AndresF, Kevin
>>
>> Everyone seems to agree that SAVEPOINT, LOCK, and DECLARE should remain
>> as errors. Everyone also seems to agree that BEGIN and COMMIT should
>> remain warnings, and ABORT should be changed from notice to warning.
>>
>> Our only disagreement seems to be how to handle the SET commands, which
>> used to report nothing. Would anyone else like to correct or express an
>> opinion? Given the current vote count and backward-compatibility,
>> warning seems to be the direction we are heading.
>
> Patch applied.

I must be missing something. The commit message for this patch says:

Also change ABORT outside of a transaction block from notice to
warning.

But the documentation says:

- Issuing <command>ABORT</> when not inside a transaction does
- no harm, but it will provoke a warning message.
+ Issuing <command>ABORT</> outside of a transaction block has no effect.

Those things are not the same.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Nov 25, 2013 at 10:04:19PM -0500, Robert Haas wrote:
> On Mon, Nov 25, 2013 at 7:19 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> > On Fri, Nov 22, 2013 at 01:19:55PM -0500, Bruce Momjian wrote:
> >> On Fri, Nov 22, 2013 at 12:17:41PM -0500, Bruce Momjian wrote:
> >> > Good points. I have modified the attached patch to do as you suggested.
> >>
> >> Also, I have read through the thread and summarized the positions of the
> >> posters:
> >>
> >> 9.3 WARNING ERROR
> >> SET none Tom, DavidJ, AndresF Robert, Kevin
> >> SAVEPOINT error Tom, DavidJ, Robert, AndresF, Kevin
> >> LOCK, DECLARE error Tom, DavidJ, Robert, AndresF, Kevin
> >>
> >> Everyone seems to agree that SAVEPOINT, LOCK, and DECLARE should remain
> >> as errors. Everyone also seems to agree that BEGIN and COMMIT should
> >> remain warnings, and ABORT should be changed from notice to warning.
> >>
> >> Our only disagreement seems to be how to handle the SET commands, which
> >> used to report nothing. Would anyone else like to correct or express an
> >> opinion? Given the current vote count and backward-compatibility,
> >> warning seems to be the direction we are heading.
> >
> > Patch applied.
>
> I must be missing something. The commit message for this patch says:
>
> Also change ABORT outside of a transaction block from notice to
> warning.
>
> But the documentation says:
>
> - Issuing <command>ABORT</> when not inside a transaction does
> - no harm, but it will provoke a warning message.
> + Issuing <command>ABORT</> outside of a transaction block has no effect.
>
> Those things are not the same.

Uh, I ended up mentioning "no effect" to highlight it does nothing,
rather than mention a warning. Would people prefer I say "warning"? Or
should I say "issues a warning because it has no effect" or something?
It is easy to change.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

On Mon, Nov 25, 2013 at 10:12:43PM -0500, Bruce Momjian wrote:
> > Those things are not the same.
>
> Uh, I ended up mentioning "no effect" to highlight it does nothing,
> rather than mention a warning. Would people prefer I say "warning"? Or
> should I say "issues a warning because it has no effect" or something?
> It is easy to change.

I should also point out that in 9.3, ABORT outside of a transaction was
documented as issuing a warning, but issued a notice. git head now
issues a warning. That might be part of the confusion.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> On Mon, Nov 25, 2013 at 10:04:19PM -0500, Robert Haas wrote:
>> But the documentation says:
>>
>> - Issuing <command>ABORT</> when not inside a transaction does
>> - no harm, but it will provoke a warning message.
>> + Issuing <command>ABORT</> outside of a transaction block has no effect.
>>
>> Those things are not the same.

> Uh, I ended up mentioning "no effect" to highlight it does nothing,
> rather than mention a warning. Would people prefer I say "warning"? Or
> should I say "issues a warning because it has no effect" or something?
> It is easy to change.

I'd revert the change Robert highlights above. ISTM you've changed the
code to match the documentation; why would you then change the docs?

regards, tom lane

On Tue, Nov 26, 2013 at 11:22:39AM -0500, Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > On Mon, Nov 25, 2013 at 10:04:19PM -0500, Robert Haas wrote:
> >> But the documentation says:
> >>
> >> - Issuing <command>ABORT</> when not inside a transaction does
> >> - no harm, but it will provoke a warning message.
> >> + Issuing <command>ABORT</> outside of a transaction block has no effect.
> >>
> >> Those things are not the same.
>
> > Uh, I ended up mentioning "no effect" to highlight it does nothing,
> > rather than mention a warning. Would people prefer I say "warning"? Or
> > should I say "issues a warning because it has no effect" or something?
> > It is easy to change.
>
> I'd revert the change Robert highlights above. ISTM you've changed the
> code to match the documentation; why would you then change the docs?

Well, I did it to make it consistent. The question is what to write for
_all_ of the new warnings, including SET. Do we say "warning", do we
say "it has no effect", or do we say both? The ABORT is a just one case
of that.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

Bruce Momjian wrote
>> >> - Issuing
> <command>
> ABORT
> </>
> when not inside a transaction does
>> >> - no harm, but it will provoke a warning message.
>> >> + Issuing
> <command>
> ABORT
> </>
> outside of a transaction block has no effect.
>> >>
>> >> Those things are not the same.
>>
>> > Uh, I ended up mentioning "no effect" to highlight it does nothing,
>> > rather than mention a warning. Would people prefer I say "warning"?
>> Or
>> > should I say "issues a warning because it has no effect" or something?
>> > It is easy to change.
>>
>> I'd revert the change Robert highlights above. ISTM you've changed the
>> code to match the documentation; why would you then change the docs?
>
> Well, I did it to make it consistent. The question is what to write for
> _all_ of the new warnings, including SET. Do we say "warning", do we
> say "it has no effect", or do we say both? The ABORT is a just one case
> of that.

How about:

"Issuing <command> outside of a transaction has no effect and will provoke a
warning."

I dislike "does no harm" because it can if someone thinks the current state
is different than reality.

It is good to indicate that a warning is emitted if this is done in error;
thus reinforcing the fact people should be looking at their warnings.

"when not inside" uses a negative modifier while "outside" is more direct
and thus easier to read, IMO. The phrase "transaction block" seems wordy so
I omitted the word "block".

David J.

--
View this message in context: http://postgresql.1045698.n5.nabble.com/Suggestion-Issue-warning-when-calling-SET-TRANSACTION-outside-transaction-block-tp5743139p5780378.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.

Bruce Momjian escribió:
> On Tue, Nov 26, 2013 at 11:22:39AM -0500, Tom Lane wrote:

> > > Uh, I ended up mentioning "no effect" to highlight it does nothing,
> > > rather than mention a warning. Would people prefer I say "warning"? Or
> > > should I say "issues a warning because it has no effect" or something?
> > > It is easy to change.
> >
> > I'd revert the change Robert highlights above. ISTM you've changed the
> > code to match the documentation; why would you then change the docs?
>
> Well, I did it to make it consistent. The question is what to write for
> _all_ of the new warnings, including SET. Do we say "warning", do we
> say "it has no effect", or do we say both? The ABORT is a just one case
> of that.

Maybe "it emits a warning and otherwise has no effect"? Emitting a
warning is certainly not doing nothing; as has been pointed out in the
SSL renegotiation thread, it might cause the log to fill disk.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Tue, Nov 26, 2013 at 08:54:13AM -0800, David Johnston wrote:
> How about:
>
> "Issuing <command> outside of a transaction has no effect and will provoke a
> warning."
>
> I dislike "does no harm" because it can if someone thinks the current state
> is different than reality.
>
> It is good to indicate that a warning is emitted if this is done in error;
> thus reinforcing the fact people should be looking at their warnings.
>
> "when not inside" uses a negative modifier while "outside" is more direct
> and thus easier to read, IMO. The phrase "transaction block" seems wordy so
> I omitted the word "block".

Every statement runs in a transaction so we can't omit "block".

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

On Tue, Nov 26, 2013 at 01:58:04PM -0300, Alvaro Herrera wrote:
> Bruce Momjian escribió:
> > On Tue, Nov 26, 2013 at 11:22:39AM -0500, Tom Lane wrote:
>
> > > > Uh, I ended up mentioning "no effect" to highlight it does nothing,
> > > > rather than mention a warning. Would people prefer I say "warning"? Or
> > > > should I say "issues a warning because it has no effect" or something?
> > > > It is easy to change.
> > >
> > > I'd revert the change Robert highlights above. ISTM you've changed the
> > > code to match the documentation; why would you then change the docs?
> >
> > Well, I did it to make it consistent. The question is what to write for
> > _all_ of the new warnings, including SET. Do we say "warning", do we
> > say "it has no effect", or do we say both? The ABORT is a just one case
> > of that.
>
> Maybe "it emits a warning and otherwise has no effect"? Emitting a
> warning is certainly not doing nothing; as has been pointed out in the
> SSL renegotiation thread, it might cause the log to fill disk.

OK, doc patch attached.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

On Tue, Nov 26, 2013 at 5:02 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> On Tue, Nov 26, 2013 at 01:58:04PM -0300, Alvaro Herrera wrote:
>> Bruce Momjian escribió:
>> > On Tue, Nov 26, 2013 at 11:22:39AM -0500, Tom Lane wrote:
>>
>> > > > Uh, I ended up mentioning "no effect" to highlight it does nothing,
>> > > > rather than mention a warning. Would people prefer I say "warning"? Or
>> > > > should I say "issues a warning because it has no effect" or something?
>> > > > It is easy to change.
>> > >
>> > > I'd revert the change Robert highlights above. ISTM you've changed the
>> > > code to match the documentation; why would you then change the docs?
>> >
>> > Well, I did it to make it consistent. The question is what to write for
>> > _all_ of the new warnings, including SET. Do we say "warning", do we
>> > say "it has no effect", or do we say both? The ABORT is a just one case
>> > of that.
>>
>> Maybe "it emits a warning and otherwise has no effect"? Emitting a
>> warning is certainly not doing nothing; as has been pointed out in the
>> SSL renegotiation thread, it might cause the log to fill disk.
>
> OK, doc patch attached.

Seems broadly reasonable, but I'd use "no other effect" throughout.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Nov 27, 2013 at 03:59:31PM -0500, Robert Haas wrote:
> On Tue, Nov 26, 2013 at 5:02 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> > On Tue, Nov 26, 2013 at 01:58:04PM -0300, Alvaro Herrera wrote:
> >> Bruce Momjian escribió:
> >> > On Tue, Nov 26, 2013 at 11:22:39AM -0500, Tom Lane wrote:
> >>
> >> > > > Uh, I ended up mentioning "no effect" to highlight it does nothing,
> >> > > > rather than mention a warning. Would people prefer I say "warning"? Or
> >> > > > should I say "issues a warning because it has no effect" or something?
> >> > > > It is easy to change.
> >> > >
> >> > > I'd revert the change Robert highlights above. ISTM you've changed the
> >> > > code to match the documentation; why would you then change the docs?
> >> >
> >> > Well, I did it to make it consistent. The question is what to write for
> >> > _all_ of the new warnings, including SET. Do we say "warning", do we
> >> > say "it has no effect", or do we say both? The ABORT is a just one case
> >> > of that.
> >>
> >> Maybe "it emits a warning and otherwise has no effect"? Emitting a
> >> warning is certainly not doing nothing; as has been pointed out in the
> >> SSL renegotiation thread, it might cause the log to fill disk.
> >
> > OK, doc patch attached.
>
> Seems broadly reasonable, but I'd use "no other effect" throughout.

That sounds awkward, e.g.:

Issuing <command>ROLLBACK</> outside of a transaction
block emits a warning but has no other effect.

I could live with this:

Issuing <command>ROLLBACK</> outside of a transaction
block has no effect except emitting a warning.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

On Wed, Nov 27, 2013 at 04:44:02PM -0500, Bruce Momjian wrote:
> I could live with this:
>
> Issuing <command>ROLLBACK</> outside of a transaction
> block has no effect except emitting a warning.

Proposed doc patch attached.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

On Wed, Nov 27, 2013 at 4:44 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>> Seems broadly reasonable, but I'd use "no other effect" throughout.
>
> That sounds awkward, e.g.:
>
> Issuing <command>ROLLBACK</> outside of a transaction
> block emits a warning but has no other effect.
>
> I could live with this:
>
> Issuing <command>ROLLBACK</> outside of a transaction
> block has no effect except emitting a warning.

I prefer the first version, but that's obviously a judgement call.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Nov 28, 2013 at 04:51:14PM -0500, Robert Haas wrote:
> On Wed, Nov 27, 2013 at 4:44 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> >> Seems broadly reasonable, but I'd use "no other effect" throughout.
> >
> > That sounds awkward, e.g.:
> >
> > Issuing <command>ROLLBACK</> outside of a transaction
> > block emits a warning but has no other effect.
> >
> > I could live with this:
> >
> > Issuing <command>ROLLBACK</> outside of a transaction
> > block has no effect except emitting a warning.
>
> I prefer the first version, but that's obviously a judgement call.

How about:

Issuing <command>ROLLBACK</> outside of a transaction
block has the sole effect of emitting a warning.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

On Nov 28, 2013, at 5:18 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> On Thu, Nov 28, 2013 at 04:51:14PM -0500, Robert Haas wrote:
>> On Wed, Nov 27, 2013 at 4:44 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>>>> Seems broadly reasonable, but I'd use "no other effect" throughout.
>>>
>>> That sounds awkward, e.g.:
>>>
>>> Issuing <command>ROLLBACK</> outside of a transaction
>>> block emits a warning but has no other effect.
>>>
>>> I could live with this:
>>>
>>> Issuing <command>ROLLBACK</> outside of a transaction
>>> block has no effect except emitting a warning.
>>
>> I prefer the first version, but that's obviously a judgement call.
>
> How about:
>
> Issuing <command>ROLLBACK</> outside of a transaction
> block has the sole effect of emitting a warning.

Sure, that sounds OK.

...Robert

Robert Haas wrote
>>
>> Issuing
> <command>
> ROLLBACK
> </>
> outside of a transaction
>> block has the sole effect of emitting a warning.
>
> Sure, that sounds OK.
>
> ...Robert

+1 for:

Issuing <command>ROLLBACK</> outside of a transaction
block has no effect except emitting a warning.

In all of these cases we are assuming that the user understands that
emitting a warning means that something is being logged to disk and thus is
causing a resource drain.

I like explicitly saying that issuing these commands is pointless/"has no
effect"; being indirect and saying that the only thing they do is emit a
warning omits any explicit explicit explanation of why. And while I agree
that logging the warning is an effect; but it is not the primary/direct
effect that the user cares about.

I would maybe change the above to:

*Issuing <command>ROLLBACK</> outside of a transaction block has no effect:
thus it emits a warning [to both user and log file].*

I do like "thus" instead of "except" due to the explicit causality link that
is establishes. We emit a warning because what you just did is pointless.

David J.

--
View this message in context: http://postgresql.1045698.n5.nabble.com/Suggestion-Issue-warning-when-calling-SET-TRANSACTION-outside-transaction-block-tp5743139p5780825.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.

David Johnston wrote:

> In all of these cases we are assuming that the user understands that
> emitting a warning means that something is being logged to disk and thus is
> causing a resource drain.
>
> I like explicitly saying that issuing these commands is pointless/"has no
> effect"; being indirect and saying that the only thing they do is emit a
> warning omits any explicit explicit explanation of why. And while I agree
> that logging the warning is an effect; but it is not the primary/direct
> effect that the user cares about.

Honestly I still prefer what I proposed initially, which AFAICS has all
the properties you deem desirable in the wording:

"issuing ROLLBACK outside a transaction emits a warning and otherwise has no effect".

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Thu, Nov 28, 2013 at 11:04 PM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
> David Johnston wrote:
>
>> In all of these cases we are assuming that the user understands that
>> emitting a warning means that something is being logged to disk and thus is
>> causing a resource drain.
>>
>> I like explicitly saying that issuing these commands is pointless/"has no
>> effect"; being indirect and saying that the only thing they do is emit a
>> warning omits any explicit explicit explanation of why. And while I agree
>> that logging the warning is an effect; but it is not the primary/direct
>> effect that the user cares about.
>
> Honestly I still prefer what I proposed initially, which AFAICS has all
> the properties you deem desirable in the wording:
>
> "issuing ROLLBACK outside a transaction emits a warning and otherwise has no effect".

Yeah, I still like "otherwise has no effect" or "has no other effect"
best. But I can live with Bruce's latest proposal, too.

I wish we'd just left this whole thing well enough alone. It wasn't
broken, and didn't need fixing.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Nov 29, 2013 at 12:27:49AM -0500, Robert Haas wrote:
> I wish we'd just left this whole thing well enough alone. It wasn't
> broken, and didn't need fixing.

Well, this started with a complaint that one SET command outside of a
transaction had no effect, and expanded to other SET commands and the
ABORT/notice inconsistency.

I realize the doc discussion is probably excessive, but we do regularly
get credit for our docs:

https://www.sparkfun.com/news/1316
The PostgreSQL manual is a thing of quiet beauty.

I hope "quiet beauty" matches our discussion goal here. :-)

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

On Fri, Nov 29, 2013 at 12:27:49AM -0500, Robert Haas wrote:
> On Thu, Nov 28, 2013 at 11:04 PM, Alvaro Herrera
> <alvherre(at)2ndquadrant(dot)com> wrote:
> > David Johnston wrote:
> >
> >> In all of these cases we are assuming that the user understands that
> >> emitting a warning means that something is being logged to disk and thus is
> >> causing a resource drain.
> >>
> >> I like explicitly saying that issuing these commands is pointless/"has no
> >> effect"; being indirect and saying that the only thing they do is emit a
> >> warning omits any explicit explicit explanation of why. And while I agree
> >> that logging the warning is an effect; but it is not the primary/direct
> >> effect that the user cares about.
> >
> > Honestly I still prefer what I proposed initially, which AFAICS has all
> > the properties you deem desirable in the wording:
> >
> > "issuing ROLLBACK outside a transaction emits a warning and otherwise has no effect".
>
> Yeah, I still like "otherwise has no effect" or "has no other effect"
> best. But I can live with Bruce's latest proposal, too.

OK, great, I have gone with Alvaro's wording; patch attached.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

On Fri, Nov 29, 2013 at 01:05:20PM -0500, Bruce Momjian wrote:
> On Fri, Nov 29, 2013 at 12:27:49AM -0500, Robert Haas wrote:
> > On Thu, Nov 28, 2013 at 11:04 PM, Alvaro Herrera
> > <alvherre(at)2ndquadrant(dot)com> wrote:
> > > David Johnston wrote:
> > >
> > >> In all of these cases we are assuming that the user understands that
> > >> emitting a warning means that something is being logged to disk and thus is
> > >> causing a resource drain.
> > >>
> > >> I like explicitly saying that issuing these commands is pointless/"has no
> > >> effect"; being indirect and saying that the only thing they do is emit a
> > >> warning omits any explicit explicit explanation of why. And while I agree
> > >> that logging the warning is an effect; but it is not the primary/direct
> > >> effect that the user cares about.
> > >
> > > Honestly I still prefer what I proposed initially, which AFAICS has all
> > > the properties you deem desirable in the wording:
> > >
> > > "issuing ROLLBACK outside a transaction emits a warning and otherwise has no effect".
> >
> > Yeah, I still like "otherwise has no effect" or "has no other effect"
> > best. But I can live with Bruce's latest proposal, too.
>
> OK, great, I have gone with Alvaro's wording; patch attached.

Duh, missing patch. Attached now.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

On Fri, Nov 29, 2013 at 01:19:54PM -0500, Bruce Momjian wrote:
> On Fri, Nov 29, 2013 at 01:05:20PM -0500, Bruce Momjian wrote:
> > On Fri, Nov 29, 2013 at 12:27:49AM -0500, Robert Haas wrote:
> > > On Thu, Nov 28, 2013 at 11:04 PM, Alvaro Herrera
> > > <alvherre(at)2ndquadrant(dot)com> wrote:
> > > > David Johnston wrote:
> > > >
> > > >> In all of these cases we are assuming that the user understands that
> > > >> emitting a warning means that something is being logged to disk and thus is
> > > >> causing a resource drain.
> > > >>
> > > >> I like explicitly saying that issuing these commands is pointless/"has no
> > > >> effect"; being indirect and saying that the only thing they do is emit a
> > > >> warning omits any explicit explicit explanation of why. And while I agree
> > > >> that logging the warning is an effect; but it is not the primary/direct
> > > >> effect that the user cares about.
> > > >
> > > > Honestly I still prefer what I proposed initially, which AFAICS has all
> > > > the properties you deem desirable in the wording:
> > > >
> > > > "issuing ROLLBACK outside a transaction emits a warning and otherwise has no effect".
> > >
> > > Yeah, I still like "otherwise has no effect" or "has no other effect"
> > > best. But I can live with Bruce's latest proposal, too.
> >
> > OK, great, I have gone with Alvaro's wording; patch attached.
>
> Duh, missing patch. Attached now.

Patch applied.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +