Suggestion: Issue warning when calling SET TRANSACTION outside transaction block

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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Feb 1, 2013 at 12:04 AM, Amit Kapila <amit.kapila@huawei.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Saturday, February 02, 2013 9:08 PM Robert Haas wrote:
On Fri, Feb 1, 2013 at 12:04 AM, Amit Kapila <amit.kapila@huawei.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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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.kapila@huawei.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@momjian.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@momjian.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.kapila@huawei.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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@momjian.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@momjian.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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@momjian.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@momjian.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Oct 3, 2013 at 8:35 PM, Andres Freund <andres@2ndquadrant.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Oct 3, 2013 at 8:32 PM, Bruce Momjian <bruce@momjian.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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@2ndquadrant.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@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[ I'm so far behind ... ]

Bruce Momjian <bruce@momjian.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Nov 8, 2013 at 5:36 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

[ I'm so far behind ... ]

Bruce Momjian <bruce@momjian.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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@sss.pgh.pa.us> wrote:

[ I'm so far behind ... ]

Bruce Momjian <bruce@momjian.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@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers