Обсуждение: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Morten Hustveit
Дата:
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.
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Amit Kapila
Дата:
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.
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Robert Haas
Дата:
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
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Amit kapila
Дата:
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, hemight 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.
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Bruce Momjian
Дата:
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. +
Вложения
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Amit Kapila
Дата:
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
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Bruce Momjian
Дата:
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. +
Вложения
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Amit Kapila
Дата:
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
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Bruce Momjian
Дата:
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. +
Вложения
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Amit Kapila
Дата:
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
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Bruce Momjian
Дата:
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. +
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Andres Freund
Дата:
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
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Amit Kapila
Дата:
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
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Amit Kapila
Дата:
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
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Bruce Momjian
Дата:
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. +
[ 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
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Robert Haas
Дата:
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
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Bruce Momjian
Дата:
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. +
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
David Johnston
Дата:
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.
Re: Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Bruce Momjian
Дата:
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. +
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
David Johnston
Дата:
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.
Re: Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Bruce Momjian
Дата:
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@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
David Johnston
Дата:
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@momjian.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
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
David Johnston
Дата:
Robert Haas wrote > On Mon, Nov 18, 2013 at 9:07 PM, Bruce Momjian < > bruce@ > > 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@yahoo.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@yahoo.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@momjian.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 outsidea 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
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Bruce Momjian
Дата:
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. 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@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Вложения
Re: Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Bruce Momjian
Дата:
On Tue, Nov 19, 2013 at 12:24:50PM -0500, Tom Lane wrote: > David Johnston <polobo@yahoo.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@momjian.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@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Andres Freund
Дата:
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
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Bruce Momjian
Дата:
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;BEGINtest=> begin work; --> WARNING: there is already a transaction in progressBEGINtest=> commit;COMMITtest=> -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Robert Haas
Дата:
On Tue, Nov 19, 2013 at 1:05 PM, Bruce Momjian <bruce@momjian.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
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Andres Freund
Дата:
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
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Bruce Momjian
Дата:
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 progressROLLBACKtest=> 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@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Andres Freund
Дата:
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
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
David Johnston
Дата:
Tom Lane-2 wrote > David Johnston < > polobo@ > > 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@ > > > 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.
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Robert Haas
Дата:
On Tue, Nov 19, 2013 at 1:14 PM, Bruce Momjian <bruce@momjian.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
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Bruce Momjian
Дата:
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@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Bruce Momjian
Дата:
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@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Bruce Momjian
Дата:
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 abortstate. The upcoming call to * CommitTransactionCommand() will then put us back into the * defaultstate. */ -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Dimitri Fontaine
Дата:
Andres Freund <andres@2ndquadrant.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@momjian.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
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Bruce Momjian
Дата:
On Tue, Nov 19, 2013 at 10:21:47PM -0500, Tom Lane wrote: > Bruce Momjian <bruce@momjian.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@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Вложения
Bruce Momjian <bruce@momjian.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
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Bruce Momjian
Дата:
On Wed, Nov 20, 2013 at 10:04:22AM -0500, Tom Lane wrote: > Bruce Momjian <bruce@momjian.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@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Bruce Momjian
Дата:
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@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Вложения
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Robert Haas
Дата:
On Wed, Nov 20, 2013 at 4:19 PM, Bruce Momjian <bruce@momjian.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
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Bruce Momjian
Дата:
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@momjian.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@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Kevin Grittner
Дата:
Robert Haas <robertmhaas@gmail.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
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Alvaro Herrera
Дата:
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
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Bruce Momjian
Дата:
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@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Вложения
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Bruce Momjian
Дата:
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@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Bruce Momjian
Дата:
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@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Robert Haas
Дата:
On Mon, Nov 25, 2013 at 7:19 PM, Bruce Momjian <bruce@momjian.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
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Bruce Momjian
Дата:
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@momjian.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@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Bruce Momjian
Дата:
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@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Bruce Momjian <bruce@momjian.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
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Bruce Momjian
Дата:
On Tue, Nov 26, 2013 at 11:22:39AM -0500, Tom Lane wrote: > Bruce Momjian <bruce@momjian.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@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
David Johnston
Дата:
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.
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Alvaro Herrera
Дата:
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
Re: Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Bruce Momjian
Дата:
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@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Bruce Momjian
Дата:
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@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Вложения
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Robert Haas
Дата:
On Tue, Nov 26, 2013 at 5:02 PM, Bruce Momjian <bruce@momjian.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
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Bruce Momjian
Дата:
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@momjian.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@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Bruce Momjian
Дата:
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@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Вложения
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Robert Haas
Дата:
On Wed, Nov 27, 2013 at 4:44 PM, Bruce Momjian <bruce@momjian.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
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Bruce Momjian
Дата:
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@momjian.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@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Robert Haas
Дата:
On Nov 28, 2013, at 5:18 PM, Bruce Momjian <bruce@momjian.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@momjian.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
Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
David Johnston
Дата:
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.
Re: Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Alvaro Herrera
Дата:
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@2ndquadrant.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
Re: Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Bruce Momjian
Дата:
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/1316The PostgreSQL manual is a thing of quiet beauty. I hope "quiet beauty" matches our discussion goal here. :-) -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Re: Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Bruce Momjian
Дата:
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@2ndquadrant.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@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Re: Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Bruce Momjian
Дата:
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@2ndquadrant.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@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Вложения
Re: Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
От
Bruce Momjian
Дата:
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@2ndquadrant.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@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +