Thread: Surprising behaviour of \set AUTOCOMMIT ON
Hello,
Need community's opinion on following behaviour of \set AUTOCOMMITIf \set AUTOCOMMIT ON is issued after \set AUTOCOMMIT OFF the commands which follow after AUTOCOMMIT is set ON are not committed until an explicit COMMIT is issued.
Its can be surprising to the user to not see results of the commands fired after AUTOCOMMIT is set to ON.
bash-4.2$ psql -d postgres -U rahila
psql (9.6beta3)
Type "help" for help.
postgres=# \set AUTOCOMMIT OFF
postgres=# create table test1(i int);
CREATE TABLE
postgres=# \set AUTOCOMMIT ON
postgres=# create table test2(j int);
CREATE TABLE
postgres=# \c postgres rahila
You are now connected to database "postgres" as user "rahila".
postgres=# \dt;
No relations found.
The ongoing transaction is left running when there is this change in mode from AUTOCOMMIT OFF to AUTOCOMMIT ON.
This happens because \set AUTOCOMMIT ON is fired within a transaction block started when first command after \set AUTOCOMMIT OFF is executed. Hence it requires an explicit COMMIT to be effective.
Should changing the value from OFF to ON automatically either commit or rollback transaction in progress?
FWIW, running set autocommit through ecpg commits the ongoing transaction when autocommit is set to ON from OFF. Should such behaviour be implemented for \set AUTOCOMMIT ON as well?
Thank you,
Rahila Syed
2016-08-03 12:16 GMT+02:00 Rahila Syed <rahilasyed90@gmail.com>:
Hello,Need community's opinion on following behaviour of \set AUTOCOMMIT
If \set AUTOCOMMIT ON is issued after \set AUTOCOMMIT OFF the commands which follow after AUTOCOMMIT is set ON are not committed until an explicit COMMIT is issued.
Its can be surprising to the user to not see results of the commands fired after AUTOCOMMIT is set to ON.
bash-4.2$ psql -d postgres -U rahila
psql (9.6beta3)
Type "help" for help.
postgres=# \set AUTOCOMMIT OFF
postgres=# create table test1(i int);
CREATE TABLE
postgres=# \set AUTOCOMMIT ON
postgres=# create table test2(j int);
CREATE TABLE
postgres=# \c postgres rahila
You are now connected to database "postgres" as user "rahila".
postgres=# \dt;
No relations found.
The ongoing transaction is left running when there is this change in mode from AUTOCOMMIT OFF to AUTOCOMMIT ON.
This happens because \set AUTOCOMMIT ON is fired within a transaction block started when first command after \set AUTOCOMMIT OFF is executed. Hence it requires an explicit COMMIT to be effective.
Should changing the value from OFF to ON automatically either commit or rollback transaction in progress?
FWIW, running set autocommit through ecpg commits the ongoing transaction when autocommit is set to ON from OFF. Should such behaviour be implemented for \set AUTOCOMMIT ON as well?
I dislike automatic commit or rollback here. What about raising warning if some transaction is open?
Regards
Pavel
Thank you,
Rahila Syed
On Wed, Aug 3, 2016 at 5:09 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote: > > > 2016-08-03 12:16 GMT+02:00 Rahila Syed <rahilasyed90@gmail.com>: >> >> Should changing the value from OFF to ON automatically either commit or >> rollback transaction in progress? >> >> >> FWIW, running set autocommit through ecpg commits the ongoing transaction >> when autocommit is set to ON from OFF. Should such behaviour be implemented >> for \set AUTOCOMMIT ON as well? > > > I dislike automatic commit or rollback here. > What problem you see with it, if we do so and may be mention the same in docs as well. Anyway, I think we should make the behaviour of both ecpg and psql same. > What about raising warning if > some transaction is open? > Not sure what benefit we will get by raising warning. I think it is better to choose one behaviour (automatic commit or leave the transaction open as is currently being done in psql) and make it consistent across all clients. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
2016-08-04 15:37 GMT+02:00 Amit Kapila <amit.kapila16@gmail.com>:
On Wed, Aug 3, 2016 at 5:09 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
>
> 2016-08-03 12:16 GMT+02:00 Rahila Syed <rahilasyed90@gmail.com>:
>>
>> Should changing the value from OFF to ON automatically either commit or
>> rollback transaction in progress?
>>
>>
>> FWIW, running set autocommit through ecpg commits the ongoing transaction
>> when autocommit is set to ON from OFF. Should such behaviour be implemented
>> for \set AUTOCOMMIT ON as well?
>
>
> I dislike automatic commit or rollback here.
>
What problem you see with it, if we do so and may be mention the same
in docs as well. Anyway, I think we should make the behaviour of both
ecpg and psql same.
Implicit COMMIT can be dangerous - ROLLBACK can be unfriendly surprising.
> What about raising warning if
> some transaction is open?
>
Not sure what benefit we will get by raising warning. I think it is
better to choose one behaviour (automatic commit or leave the
transaction open as is currently being done in psql) and make it
consistent across all clients.
I am not sure about value of ecpg for this case. It is used by 0.0001% users. Probably nobody in Czech Republic knows this client.
Warnings enforce the user do some decision - I don't think so we can do this decision well.
Regards
Pavel
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
On Thu, Aug 4, 2016 at 7:46 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote: > 2016-08-04 15:37 GMT+02:00 Amit Kapila <amit.kapila16@gmail.com>: >> >> > I dislike automatic commit or rollback here. >> > >> >> What problem you see with it, if we do so and may be mention the same >> in docs as well. Anyway, I think we should make the behaviour of both >> ecpg and psql same. > > > Implicit COMMIT can be dangerous > Not, when user has specifically requested for autocommit mode as 'on'. I think here what would be more meaningful is that after "Set AutoCommit On", when the first command is committed, it should commit previous non-pending committed commands as well. >> >> Not sure what benefit we will get by raising warning. I think it is >> better to choose one behaviour (automatic commit or leave the >> transaction open as is currently being done in psql) and make it >> consistent across all clients. > > > I am not sure about value of ecpg for this case. It is used by 0.0001% > users. Probably nobody in Czech Republic knows this client. > Sure, but that doesn't give us the license for being inconsistent in behaviour across different clients. > Warnings enforce the user do some decision > They could be annoying as well, especially if that happens in scripts. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
2016-08-06 7:29 GMT+02:00 Amit Kapila <amit.kapila16@gmail.com>:
On Thu, Aug 4, 2016 at 7:46 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> 2016-08-04 15:37 GMT+02:00 Amit Kapila <amit.kapila16@gmail.com>:
>>
>> > I dislike automatic commit or rollback here.
>> >
>>
>> What problem you see with it, if we do so and may be mention the same
>> in docs as well. Anyway, I think we should make the behaviour of both
>> ecpg and psql same.
>
>
> Implicit COMMIT can be dangerous
>
Not, when user has specifically requested for autocommit mode as 'on'.
I think here what would be more meaningful is that after "Set
AutoCommit On", when the first command is committed, it should commit
previous non-pending committed commands as well.
This is place when safety and and user friendly interface going against - the most safe behave is raising rollback there. But it can be in contrast with user's expectation.
>>
>> Not sure what benefit we will get by raising warning. I think it is
>> better to choose one behaviour (automatic commit or leave the
>> transaction open as is currently being done in psql) and make it
>> consistent across all clients.
>
>
> I am not sure about value of ecpg for this case. It is used by 0.0001%
> users. Probably nobody in Czech Republic knows this client.
>
Sure, but that doesn't give us the license for being inconsistent in
behaviour across different clients.
This is question. ecpg was designed years ago - and some details can be designed wrong.
Next question is design for interactive and non interactive usage.
> Warnings enforce the user do some decision
>
They could be annoying as well, especially if that happens in scripts.
in script - probably rollback is correct - script can be executed more time and user can fix it.
I am not sure if we can solve this issue as isolated problem. The first question should be - who, why and when does switching from autocommit off to on? How often this operation is? And we should be safe or we should not?
Regards
Pavel
As an extra data point, if you try this in Python (psycopg2) you get an exception:
psycopg2.ProgrammingError: autocommit cannot be used inside a transaction
I think this exception is a legitimate response. If the user switches on autocommit mode inside a transaction, it was most likely not on purpose. Chances are, they didn't realise autocommit was off in the first place.psycopg2.ProgrammingError: autocommit cannot be used inside a transaction
Even if we assume that it was done deliberately, it's difficult to know exactly what the user intended. It seems to hinge on a subtlety of what the user understands autocommit mode to mean -- either "issue an implicit COMMIT after each statement", or "ensure there is never an open transaction".
On Sat, 6 Aug 2016 at 15:30 Amit Kapila <amit.kapila16@gmail.com> wrote:
On Thu, Aug 4, 2016 at 7:46 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> 2016-08-04 15:37 GMT+02:00 Amit Kapila <amit.kapila16@gmail.com>:
>>
>> > I dislike automatic commit or rollback here.
>> >
>>
>> What problem you see with it, if we do so and may be mention the same
>> in docs as well. Anyway, I think we should make the behaviour of both
>> ecpg and psql same.
>
>
> Implicit COMMIT can be dangerous
>
Not, when user has specifically requested for autocommit mode as 'on'.
I think here what would be more meaningful is that after "Set
AutoCommit On", when the first command is committed, it should commit
previous non-pending committed commands as well.
>>
>> Not sure what benefit we will get by raising warning. I think it is
>> better to choose one behaviour (automatic commit or leave the
>> transaction open as is currently being done in psql) and make it
>> consistent across all clients.
>
>
> I am not sure about value of ecpg for this case. It is used by 0.0001%
> users. Probably nobody in Czech Republic knows this client.
>
Sure, but that doesn't give us the license for being inconsistent in
behaviour across different clients.
> Warnings enforce the user do some decision
>
They could be annoying as well, especially if that happens in scripts.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
<div dir="ltr"><div class="gmail_extra">Its worth noting that the JDBC's behavior when you switch back to autocommit is toimmediately commit the open transaction.</div><div class="gmail_extra"><br /></div><div class="gmail_extra">Personally,I think committing immediately or erroring are unsurprising behaviors. The current behavioris surprising and obviously wrong. Rolling back without an error would be very surprising (no other database APII know of does that) and would take forever to debug issues around the behavior. And committing after the next statementis definitely the most surprising behavior suggested.</div><div class="gmail_extra"><br /></div><div class="gmail_extra">IMHO,I think committing immediately and erroring are both valid. I think I'd prefer the error in principle,and per the law of bad code I'm sure, although no one has ever intended to use this behavior, there is probablysome code out there that is relying on this behavior for "correctness". I think a hard failure and making the devadd an explicit commit is least likely to cause people serious issues. As for the other options, consider me opposed.</div><divclass="gmail_extra"><br /></div><div class="gmail_extra">- Matt K.</div></div>
Thank you for inputs everyone.
The opinions on this thread can be classified into following
1. Commit
2. Rollback
3. Error
4. Warning
As per opinion upthread, issuing implicit commit immediately after switching autocommit to ON, can be unsafe if it was not desired. While I agree that its difficult to judge users intention here, but if we were to base it on some assumption, the closest would be implicit COMMIT in my opinion.There is higher likelihood of a user being happy with issuing a commit when setting autocommit ON than a transaction being rolled back. Also there are quite some interfaces which provide this.
As mentioned upthread, issuing a warning on switching back to autocommit will not be effective inside a script. It won't allow subsequent commands to be committed as set autocommit to ON is not committed. Scripts will have to be rerun with changes which will impact user friendliness.
While I agree that issuing an ERROR and rolling back the transaction ranks higher in safe behaviour, it is not as common (according to instances stated upthread) as immediately committing any open transaction when switching back to autocommit.
Thank you,
Rahila Syed
On Sun, Aug 7, 2016 at 4:42 AM, Matt Kelly <mkellycs@gmail.com> wrote:
Its worth noting that the JDBC's behavior when you switch back to autocommit is to immediately commit the open transaction.Personally, I think committing immediately or erroring are unsurprising behaviors. The current behavior is surprising and obviously wrong. Rolling back without an error would be very surprising (no other database API I know of does that) and would take forever to debug issues around the behavior. And committing after the next statement is definitely the most surprising behavior suggested.IMHO, I think committing immediately and erroring are both valid. I think I'd prefer the error in principle, and per the law of bad code I'm sure, although no one has ever intended to use this behavior, there is probably some code out there that is relying on this behavior for "correctness". I think a hard failure and making the dev add an explicit commit is least likely to cause people serious issues. As for the other options, consider me opposed.- Matt K.
On Mon, Aug 8, 2016 at 10:10 AM, Rahila Syed <rahilasyed90@gmail.com> wrote: > Thank you for inputs everyone. > > The opinions on this thread can be classified into following > 1. Commit > 2. Rollback > 3. Error > 4. Warning > > As per opinion upthread, issuing implicit commit immediately after switching > autocommit to ON, can be unsafe if it was not desired. While I agree that > its difficult to judge users intention here, but if we were to base it on > some assumption, the closest would be implicit COMMIT in my opinion.There is > higher likelihood of a user being happy with issuing a commit when setting > autocommit ON than a transaction being rolled back. Also there are quite > some interfaces which provide this. > > As mentioned upthread, issuing a warning on switching back to autocommit > will not be effective inside a script. It won't allow subsequent commands to > be committed as set autocommit to ON is not committed. Scripts will have to > be rerun with changes which will impact user friendliness. > > While I agree that issuing an ERROR and rolling back the transaction ranks > higher in safe behaviour, it is not as common (according to instances stated > upthread) as immediately committing any open transaction when switching back > to autocommit. I think I like the option of having psql issue an error. On the server side, the transaction would still be open, but the user would receive a psql error message and the autocommit setting would not be changed. So the user could type COMMIT or ROLLBACK manually and then retry changing the value of the setting. Alternatively, I also think it would be sensible to issue an immediate COMMIT when the autocommit setting is changed from off to on. That was my first reaction. Aborting the server-side transaction - with or without notice - doesn't seem very reasonable. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mon, Aug 8, 2016 at 10:10 AM, Rahila Syed <rahilasyed90@gmail.com> wrote:
> Thank you for inputs everyone.
>
> The opinions on this thread can be classified into following
> 1. Commit
> 2. Rollback
> 3. Error
> 4. Warning
>
> As per opinion upthread, issuing implicit commit immediately after switching
> autocommit to ON, can be unsafe if it was not desired. While I agree that
> its difficult to judge users intention here, but if we were to base it on
> some assumption, the closest would be implicit COMMIT in my opinion.There is
> higher likelihood of a user being happy with issuing a commit when setting
> autocommit ON than a transaction being rolled back. Also there are quite
> some interfaces which provide this.
>
> As mentioned upthread, issuing a warning on switching back to autocommit
> will not be effective inside a script. It won't allow subsequent commands to
> be committed as set autocommit to ON is not committed. Scripts will have to
> be rerun with changes which will impact user friendliness.
>
> While I agree that issuing an ERROR and rolling back the transaction ranks
> higher in safe behaviour, it is not as common (according to instances stated
> upthread) as immediately committing any open transaction when switching back
> to autocommit.
I think I like the option of having psql issue an error. On the
server side, the transaction would still be open, but the user would
receive a psql error message and the autocommit setting would not be
changed. So the user could type COMMIT or ROLLBACK manually and then
retry changing the value of the setting.
Alternatively, I also think it would be sensible to issue an immediate
COMMIT when the autocommit setting is changed from off to on. That
was my first reaction.
Aborting the server-side transaction - with or without notice -
doesn't seem very reasonable.
Best of both worlds?
IF (ON_ERROR_STOP == 1)
THEN (treat this like any other error)
ELSE (send COMMIT; to server)
David J.
On Mon, Aug 8, 2016 at 11:17 AM, David G. Johnston <david.g.johnston@gmail.com> wrote: > On Mon, Aug 8, 2016 at 11:02 AM, Robert Haas <robertmhaas@gmail.com> wrote: >> >> On Mon, Aug 8, 2016 at 10:10 AM, Rahila Syed <rahilasyed90@gmail.com> >> wrote: >> > Thank you for inputs everyone. >> > >> > The opinions on this thread can be classified into following >> > 1. Commit >> > 2. Rollback >> > 3. Error >> > 4. Warning >> > >> > As per opinion upthread, issuing implicit commit immediately after >> > switching >> > autocommit to ON, can be unsafe if it was not desired. While I agree >> > that >> > its difficult to judge users intention here, but if we were to base it >> > on >> > some assumption, the closest would be implicit COMMIT in my >> > opinion.There is >> > higher likelihood of a user being happy with issuing a commit when >> > setting >> > autocommit ON than a transaction being rolled back. Also there are >> > quite >> > some interfaces which provide this. >> > >> > As mentioned upthread, issuing a warning on switching back to autocommit >> > will not be effective inside a script. It won't allow subsequent >> > commands to >> > be committed as set autocommit to ON is not committed. Scripts will have >> > to >> > be rerun with changes which will impact user friendliness. >> > >> > While I agree that issuing an ERROR and rolling back the transaction >> > ranks >> > higher in safe behaviour, it is not as common (according to instances >> > stated >> > upthread) as immediately committing any open transaction when switching >> > back >> > to autocommit. >> >> I think I like the option of having psql issue an error. On the >> server side, the transaction would still be open, but the user would >> receive a psql error message and the autocommit setting would not be >> changed. So the user could type COMMIT or ROLLBACK manually and then >> retry changing the value of the setting. >> >> Alternatively, I also think it would be sensible to issue an immediate >> COMMIT when the autocommit setting is changed from off to on. That >> was my first reaction. >> >> Aborting the server-side transaction - with or without notice - >> doesn't seem very reasonable. >> > > Best of both worlds? > > IF (ON_ERROR_STOP == 1) > THEN (treat this like any other error) > ELSE (send COMMIT; to server) No, that's not a good idea. The purpose of ON_ERROR_STOP is something else entirely, and we shouldn't reuse it for an unrelated purpose. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 08/08/16 17:02, Robert Haas wrote: > On Mon, Aug 8, 2016 at 10:10 AM, Rahila Syed <rahilasyed90@gmail.com> wrote: >> Thank you for inputs everyone. >> >> The opinions on this thread can be classified into following >> 1. Commit >> 2. Rollback >> 3. Error >> 4. Warning >> >> As per opinion upthread, issuing implicit commit immediately after switching >> autocommit to ON, can be unsafe if it was not desired. While I agree that >> its difficult to judge users intention here, but if we were to base it on >> some assumption, the closest would be implicit COMMIT in my opinion.There is >> higher likelihood of a user being happy with issuing a commit when setting >> autocommit ON than a transaction being rolled back. Also there are quite >> some interfaces which provide this. >> >> As mentioned upthread, issuing a warning on switching back to autocommit >> will not be effective inside a script. It won't allow subsequent commands to >> be committed as set autocommit to ON is not committed. Scripts will have to >> be rerun with changes which will impact user friendliness. >> >> While I agree that issuing an ERROR and rolling back the transaction ranks >> higher in safe behaviour, it is not as common (according to instances stated >> upthread) as immediately committing any open transaction when switching back >> to autocommit. > > I think I like the option of having psql issue an error. On the > server side, the transaction would still be open, but the user would > receive a psql error message and the autocommit setting would not be > changed. So the user could type COMMIT or ROLLBACK manually and then > retry changing the value of the setting. This is my preferred action. > Alternatively, I also think it would be sensible to issue an immediate > COMMIT when the autocommit setting is changed from off to on. That > was my first reaction. I don't care for this very much. > Aborting the server-side transaction - with or without notice - > doesn't seem very reasonable. Agreed. -- Vik Fearing +33 6 46 75 15 36 http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
Just for information,
PG current behavior,
"\set AUTOCOMMIT OFF" implicitly does/open "BEGIN;" block
So, "\set AUTOCOMMIT ON" has no effect once "\set AUTOCOMMIT OFF" is issued until "END;" or "COMMIT;" or "ROLLBACK;"
however, I think if exit session release the transactions then change session should also release the transactions
Thanks
Sridhar
On Mon, Aug 8, 2016 at 10:34 PM, Vik Fearing <vik@2ndquadrant.fr> wrote:
This is my preferred action.On 08/08/16 17:02, Robert Haas wrote:
> On Mon, Aug 8, 2016 at 10:10 AM, Rahila Syed <rahilasyed90@gmail.com> wrote:
>> Thank you for inputs everyone.
>>
>> The opinions on this thread can be classified into following
>> 1. Commit
>> 2. Rollback
>> 3. Error
>> 4. Warning
>>
>> As per opinion upthread, issuing implicit commit immediately after switching
>> autocommit to ON, can be unsafe if it was not desired. While I agree that
>> its difficult to judge users intention here, but if we were to base it on
>> some assumption, the closest would be implicit COMMIT in my opinion.There is
>> higher likelihood of a user being happy with issuing a commit when setting
>> autocommit ON than a transaction being rolled back. Also there are quite
>> some interfaces which provide this.
>>
>> As mentioned upthread, issuing a warning on switching back to autocommit
>> will not be effective inside a script. It won't allow subsequent commands to
>> be committed as set autocommit to ON is not committed. Scripts will have to
>> be rerun with changes which will impact user friendliness.
>>
>> While I agree that issuing an ERROR and rolling back the transaction ranks
>> higher in safe behaviour, it is not as common (according to instances stated
>> upthread) as immediately committing any open transaction when switching back
>> to autocommit.
>
> I think I like the option of having psql issue an error. On the
> server side, the transaction would still be open, but the user would
> receive a psql error message and the autocommit setting would not be
> changed. So the user could type COMMIT or ROLLBACK manually and then
> retry changing the value of the setting.
> Alternatively, I also think it would be sensible to issue an immediate
> COMMIT when the autocommit setting is changed from off to on. That
> was my first reaction.
I don't care for this very much.
> Aborting the server-side transaction - with or without notice -
> doesn't seem very reasonable.
Agreed.
--
Vik Fearing +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Aug 9, 2016 at 1:02 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Mon, Aug 8, 2016 at 10:10 AM, Rahila Syed <rahilasyed90@gmail.com> wrote:
> Thank you for inputs everyone.
>
> The opinions on this thread can be classified into following
> 1. Commit
> 2. Rollback
> 3. Error
> 4. Warning
>
> As per opinion upthread, issuing implicit commit immediately after switching
> autocommit to ON, can be unsafe if it was not desired. While I agree that
> its difficult to judge users intention here, but if we were to base it on
> some assumption, the closest would be implicit COMMIT in my opinion.There is
> higher likelihood of a user being happy with issuing a commit when setting
> autocommit ON than a transaction being rolled back. Also there are quite
> some interfaces which provide this.
>
> As mentioned upthread, issuing a warning on switching back to autocommit
> will not be effective inside a script. It won't allow subsequent commands to
> be committed as set autocommit to ON is not committed. Scripts will have to
> be rerun with changes which will impact user friendliness.
>
> While I agree that issuing an ERROR and rolling back the transaction ranks
> higher in safe behaviour, it is not as common (according to instances stated
> upthread) as immediately committing any open transaction when switching back
> to autocommit.
I think I like the option of having psql issue an error. On the
server side, the transaction would still be open, but the user would
receive a psql error message and the autocommit setting would not be
changed. So the user could type COMMIT or ROLLBACK manually and then
retry changing the value of the setting.
This makes more sense as the user who is doing it would realise that the transaction has been left open.
Alternatively, I also think it would be sensible to issue an immediate
COMMIT when the autocommit setting is changed from off to on. That
was my first reaction.
Issuing commit would indicate that, open transactions will be committed which is not a good idea in my opinion. If the user is issuing AUTOCOMMIT = ON, then it means all the transactions initiated after issuing this must be committed, whereas it is committing the previously pending transactions as well.
Aborting the server-side transaction - with or without notice -
doesn't seem very reasonable.
Agreed. Traditionally, open transactions in the database must be left open until user issues a COMMIT or ROLLBACK. If the session is changed or killed, then, the transaction must be rolled back.
Regards,
Venkata B N
Fujitsu Australia
On Thu, Aug 11, 2016 at 2:58 PM, Venkata Balaji N <nag1010@gmail.com> wrote:
On Tue, Aug 9, 2016 at 1:02 AM, Robert Haas <robertmhaas@gmail.com> wrote:On Mon, Aug 8, 2016 at 10:10 AM, Rahila Syed <rahilasyed90@gmail.com> wrote:
> Thank you for inputs everyone.
>
> The opinions on this thread can be classified into following
> 1. CommitThis makes more sense as the user who is doing it would realise that the transaction has been left open.Alternatively, I also think it would be sensible to issue an immediate
COMMIT when the autocommit setting is changed from off to on. That
was my first reaction.Issuing commit would indicate that, open transactions will be committed which is not a good idea in my opinion. If the user is issuing AUTOCOMMIT = ON, then it means all the transactions initiated after issuing this must be committed, whereas it is committing the previously pending transactions as well.
My apologies for confusing statement, correction - i meant, by setting autocommit=on, committing all the previously open transactions ( transactions opened when autocommit=off) may not be a good idea. What user meant by autocommit=on is that all the subsequent transactions must be committed.
Regards,
Venkata B N
Fujitsu Australia
[...]committing all the previously open transactions[...]
"All"? There can only ever be at most one open transaction at any given time...
I don't have a fundamental issue with saying "when turning auto-commit on you are also requesting that the open transaction, if there is one, is committed immediately." I'm more inclined to think an error is the correct solution - or to respond in a way conditional to the present usage (interactive vs. script). I disagree with Robert's unsubstantiated belief regarding ON_ERROR_STOP and think that it captures the relevant user-intent for this behavior as well.
David J.
On Thu, Aug 11, 2016 at 8:34 AM, David G. Johnston <david.g.johnston@gmail.com> wrote: > I don't have a fundamental issue with saying "when turning auto-commit on > you are also requesting that the open transaction, if there is one, is > committed immediately." I'm more inclined to think an error is the correct > solution - or to respond in a way conditional to the present usage > (interactive vs. script). I disagree with Robert's unsubstantiated belief > regarding ON_ERROR_STOP and think that it captures the relevant user-intent > for this behavior as well. I'll substantiate my belief by referring to you for the documentation for ON_ERROR_STOP, which says: "By default, command processing continues after an error. When this variable is set to on, processing will instead stop immediately. In interactive mode, psql will return to the command prompt; otherwise, psql will exit, returning error code 3 to distinguish this case from fatal error conditions, which are reported using error code 1. In either case, any currently running scripts (the top-level script, if any, and any other scripts which it may have in invoked) will be terminated immediately. If the top-level command string contained multiple SQL commands, processing will stop with the current command." In every existing case, ON_ERROR_STOP affects whether we continue to process further commands after an error has already occurred. Your proposal would involve changing things so that the value ON_ERROR_STOP affects not only *how errors are handled* but *whether they happen in the first place* -- but only in this one really specific case, and no others. This isn't really an arguable point, even if you want to try to pretend otherwise. ON_ERROR_STOP should affect whether we stop on error, not whether generate an error in the first place. The clue is in the name. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Thu, Aug 11, 2016 at 8:34 AM, David G. Johnston
<david.g.johnston@gmail.com> wrote:
> I don't have a fundamental issue with saying "when turning auto-commit on
> you are also requesting that the open transaction, if there is one, is
> committed immediately." I'm more inclined to think an error is the correct
> solution - or to respond in a way conditional to the present usage
> (interactive vs. script). I disagree with Robert's unsubstantiated belief
> regarding ON_ERROR_STOP and think that it captures the relevant user-intent
> for this behavior as well.
I'll substantiate my belief by referring to you for the documentation
for ON_ERROR_STOP, which says:
"By default, command processing continues after an error. When this
variable is set to on, processing will instead stop immediately. In
interactive mode, psql will return to the command prompt; otherwise,
psql will exit, returning error code 3 to distinguish this case from
fatal error conditions, which are reported using error code 1. In
either case, any currently running scripts (the top-level script, if
any, and any other scripts which it may have in invoked) will be
terminated immediately. If the top-level command string contained
multiple SQL commands, processing will stop with the current command."
In every existing case, ON_ERROR_STOP affects whether we continue to
process further commands after an error has already occurred. Your
proposal would involve changing things so that the value ON_ERROR_STOP
affects not only *how errors are handled* but *whether they happen in
the first place* -- but only in this one really specific case, and no
others.
This isn't really an arguable point, even if you want to try to
pretend otherwise. ON_ERROR_STOP should affect whether we stop on
error, not whether generate an error in the first place. The clue is
in the name.
Changing AUTOCOMMIT to ON while in a transaction is a psql error - period.
If ON_ERROR_STOP is on we stop. This meets the current semantics for ON_ERROR_STOP.
With ON_ERROR_STOP off psql is going to continue on with the next command. I'd suggest changing things so that psql can, depending upon the error, invoke additional commands to bring the system into a known good state before the next user command is executed. In the case of "\set AUTOCOMMIT on" this additional command would be COMMIT. We can still report the error before continuing on - so there is no affecting the "generating [of] an error in the first place.".
Allowing command-specific responses to errors when faced with script continuation would be a change. I do not think it would be a bad one. Am I stretching a bit here? Sure. Is it worth stretching to avoid adding more knobs to the system? Maybe.
I'll admit I haven't tried to find fault with the idea (or discover better alternatives) nor how it would look in implementation. As a user, though, it would make sense if the system behaved in this way. That only AUTOCOMMIT needs this capability at the moment doesn't bother me. I'm also fine with making it an error and moving on - but if you want to accommodate both possibilities this seems like a cleaner solution than yet another environment variable that a user would need to consider.
David J.
On Sat, Aug 13, 2016 at 1:05 AM, David G. Johnston <david.g.johnston@gmail.com> wrote: > > I'll admit I haven't tried to find fault with the idea (or discover better > alternatives) nor how it would look in implementation. As a user, though, > it would make sense if the system behaved in this way. That only AUTOCOMMIT > needs this capability at the moment doesn't bother me. I'm also fine with > making it an error and moving on - but if you want to accommodate both > possibilities this seems like a cleaner solution than yet another > environment variable that a user would need to consider. > I agree on your broad point that we should consider user convenience, but in this case I am not sure if it will be convenient for a user to make the behaviour dependent on value of ON_ERROR_STOP. I have checked this behaviour in one of the top commercial database and it just commits the previous transaction after the user turns the Autocommit to on and the first command is complete. I am not saying that we should blindly follow that behaviour, but just to indicate that it should be okay for users if we don't try to define multiple behaviours here based on value of ON_ERROR_STOP. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
<div dir="ltr"><p dir="ltr">>I think I like the option of having psql issue an error. On the<br /> >server side, thetransaction would still be open, but the user would<br /> >receive a psql error message and the autocommit settingwould not be<br /> >changed. So the user could type COMMIT or ROLLBACK manually and then<br /> >retry changingthe value of the setting.<p dir="ltr">Throwing psql error comes out to be most accepted outcome on this thread. Iagree it is safer than guessing user intention. <p>Although according to the default behaviour of psql, error will abortthe current transaction and roll back all the previous commands. This can be user unfriendly making user rerun all thecommands just because of autocommit switch. So probably behaviour of 'ON_ERROR_ROLLBACK on' needs to be implemented alongwith the error display. This will rollback just the autocommit switch command.<p>Also, psql error instead of a simplecommit will lead to script terminations. Hence issuing a COMMIT seems more viable here. However, script terminationcan be avoided by default behaviour of ON_ERROR_STOP which will execute subsequent commands successfully.(Howeversubsequent commands won't be executed in autocommit mode which I think should be OK as it will be notifiedvia ERROR).<p>So summarizing my view of the discussion on this thread, issuing a psql error seems to be the bestoption. I will post a patch regarding this if there is no objection. <p><br /><p>Thank you,<p>Rahila Syed<p><br /><p><br/></div>
On Tue, Aug 16, 2016 at 5:25 PM, Rahila Syed <rahilasyed90@gmail.com> wrote: >>I think I like the option of having psql issue an error. On the >>server side, the transaction would still be open, but the user would >>receive a psql error message and the autocommit setting would not be >>changed. So the user could type COMMIT or ROLLBACK manually and then >>retry changing the value of the setting. > > Throwing psql error comes out to be most accepted outcome on this thread. I > agree it is safer than guessing user intention. > > Although according to the default behaviour of psql, error will abort the > current transaction and roll back all the previous commands. A server error would do that, but a psql errror won't. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Ok. Please find attached a patch which introduces psql error when autocommit is turned on inside a transaction. It also adds relevant documentation in psql-ref.sgml. Following is the output.
bash-4.2$ psql -d postgres
psql (10devel)
Type "help" for help.
postgres=# \set AUTOCOMMIT OFF
postgres=# create table test(i int);
CREATE TABLE
postgres=# \set AUTOCOMMIT ON
\set: Cannot set AUTOCOMMIT to ON inside a transaction, either COMMIT or ROLLBACK and retry
postgres=#
bash-4.2$ psql -d postgres
psql (10devel)
Type "help" for help.
postgres=# \set AUTOCOMMIT OFF
postgres=# create table test(i int);
CREATE TABLE
postgres=# \set AUTOCOMMIT ON
\set: Cannot set AUTOCOMMIT to ON inside a transaction, either COMMIT or ROLLBACK and retry
postgres=#
Thank you,
Rahila Syed
On Wed, Aug 17, 2016 at 6:31 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Tue, Aug 16, 2016 at 5:25 PM, Rahila Syed <rahilasyed90@gmail.com> wrote:
>>I think I like the option of having psql issue an error. On the
>>server side, the transaction would still be open, but the user would
>>receive a psql error message and the autocommit setting would not be
>>changed. So the user could type COMMIT or ROLLBACK manually and then
>>retry changing the value of the setting.
>
> Throwing psql error comes out to be most accepted outcome on this thread. I
> agree it is safer than guessing user intention.
>
> Although according to the default behaviour of psql, error will abort the
> current transaction and roll back all the previous commands.
A server error would do that, but a psql errror won't.
Attachment
I don't think patch is doing correct things:
1)
postgres=# \set AUTOCOMMIT off
postgres=# create table foo (a int );
CREATE TABLE
postgres=# \set AUTOCOMMIT on
Above test not throwing psql error, as you used strcmp(newval,"ON"). You
should use pg_strcasecmp.
2)
postgres=# \set AUTOCOMMIT OFF
postgres=# create table foo ( a int );
CREATE TABLE
postgres=# \set VERBOSITY ON
\set: Cannot set VERBOSITY to ON inside a transaction, either COMMIT or ROLLBACK and retry
Above error coming because in below code block change, you don't have check for
command (you should check opt0 for AUTOCOMMIT command)
- if (!SetVariable(pset.vars, opt0, newval))
+ if (transaction_status == PQTRANS_INTRANS && !pset.autocommit &&
+ !strcmp(newval,"ON"))
+ {
+ psql_error("\\%s: Cannot set %s to %s inside a transaction, either COMMIT or ROLLBACK and retry\n",
+ cmd, opt0, newval);
+ success = false;
+ }
3)
postgres=# BEGIN;
BEGIN
postgres=# create table foo ( a int );
CREATE TABLE
postgres=# \set AUTOCOMMIT ON
Don't you think, in above case also we should throw a psql error?
4)
postgres=# \set AUTOCOMMIT off
postgres=# create table foo ( a int );
CREATE TABLE
postgres=# \set XYZ ON
\set: Cannot set XYZ to ON inside a transaction, either COMMIT or ROLLBACK and retry
May be you would like to move the new code block inside SetVariable(). So that
don't need to worry about the validity of the variable names.
Basically if I understand correctly, if we are within transaction and someone
tries the set the AUTOCOMMIT ON, it should throw a psql error. Correct me
if I am missing anything?
1)
postgres=# \set AUTOCOMMIT off
postgres=# create table foo (a int );
CREATE TABLE
postgres=# \set AUTOCOMMIT on
Above test not throwing psql error, as you used strcmp(newval,"ON"). You
should use pg_strcasecmp.
2)
postgres=# \set AUTOCOMMIT OFF
postgres=# create table foo ( a int );
CREATE TABLE
postgres=# \set VERBOSITY ON
\set: Cannot set VERBOSITY to ON inside a transaction, either COMMIT or ROLLBACK and retry
Above error coming because in below code block change, you don't have check for
command (you should check opt0 for AUTOCOMMIT command)
- if (!SetVariable(pset.vars, opt0, newval))
+ if (transaction_status == PQTRANS_INTRANS && !pset.autocommit &&
+ !strcmp(newval,"ON"))
+ {
+ psql_error("\\%s: Cannot set %s to %s inside a transaction, either COMMIT or ROLLBACK and retry\n",
+ cmd, opt0, newval);
+ success = false;
+ }
3)
postgres=# BEGIN;
BEGIN
postgres=# create table foo ( a int );
CREATE TABLE
postgres=# \set AUTOCOMMIT ON
Don't you think, in above case also we should throw a psql error?
4)
postgres=# \set AUTOCOMMIT off
postgres=# create table foo ( a int );
CREATE TABLE
postgres=# \set XYZ ON
\set: Cannot set XYZ to ON inside a transaction, either COMMIT or ROLLBACK and retry
May be you would like to move the new code block inside SetVariable(). So that
don't need to worry about the validity of the variable names.
Basically if I understand correctly, if we are within transaction and someone
tries the set the AUTOCOMMIT ON, it should throw a psql error. Correct me
if I am missing anything?
On Thu, Sep 1, 2016 at 4:23 PM, Rahila Syed <rahilasyed90@gmail.com> wrote:
Ok. Please find attached a patch which introduces psql error when autocommit is turned on inside a transaction. It also adds relevant documentation in psql-ref.sgml. Following is the output.
bash-4.2$ psql -d postgres
psql (10devel)
Type "help" for help.
postgres=# \set AUTOCOMMIT OFF
postgres=# create table test(i int);
CREATE TABLE
postgres=# \set AUTOCOMMIT ON
\set: Cannot set AUTOCOMMIT to ON inside a transaction, either COMMIT or ROLLBACK and retry
postgres=#Thank you,Rahila SyedOn Wed, Aug 17, 2016 at 6:31 PM, Robert Haas <robertmhaas@gmail.com> wrote:On Tue, Aug 16, 2016 at 5:25 PM, Rahila Syed <rahilasyed90@gmail.com> wrote:
>>I think I like the option of having psql issue an error. On the
>>server side, the transaction would still be open, but the user would
>>receive a psql error message and the autocommit setting would not be
>>changed. So the user could type COMMIT or ROLLBACK manually and then
>>retry changing the value of the setting.
>
> Throwing psql error comes out to be most accepted outcome on this thread. I
> agree it is safer than guessing user intention.
>
> Although according to the default behaviour of psql, error will abort the
> current transaction and roll back all the previous commands.
A server error would do that, but a psql errror won't.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
--
Rushabh Lathia
Hello,
Thank you for comments.>Above test not throwing psql error, as you used strcmp(newval,"ON"). You
>should use pg_strcasecmp.
>Above error coming because in below code block change, you don't have check for
>command (you should check opt0 for AUTOCOMMIT command)
>postgres=# BEGIN;
>BEGIN
>postgres=# create table foo ( a int );
>CREATE TABLE
>postgres=# \set AUTOCOMMIT ON
>Don't you think, in above case also we should throw a psql error?
Hence I did not consider this case.
>postgres=# \set AUTOCOMMIT off
>postgres=# create table foo ( a int );
>CREATE TABLE
>postgres=# \set XYZ ON
>\set: Cannot set XYZ to ON inside a transaction, either COMMIT or ROLLBACK and retry
>May be you would like to move the new code block inside SetVariable(). So that
>don't need to worry about the validity of the variable names.
>postgres=# \set AUTOCOMMIT off
>postgres=# create table foo ( a int );
>CREATE TABLE
>postgres=# \set XYZ ON
>\set: Cannot set XYZ to ON inside a transaction, either COMMIT or ROLLBACK and retry
>May be you would like to move the new code block inside SetVariable(). So that
>don't need to worry about the validity of the variable names.
I think validating variable names wont be required if we throw error only if command is \set AUTOCOMMIT.
Validation can happen later as in the existing code.
>Basically if I understand correctly, if we are within transaction and someone
>tries the set the AUTOCOMMIT ON, it should throw a psql error. Correct me
>if I am missing anything?
Yes the psql_error is thrown when AUTOCOMMIT is turned on inside a transaction. But only when there is an implicit BEGIN as in following case,
postgres=# \set AUTOCOMMIT OFF
postgres=# create table test(i int);
CREATE TABLE
postgres=# \set AUTOCOMMIT ON
\set: Cannot set AUTOCOMMIT to ON inside a transaction, either COMMIT or ROLLBACK and retry
postgres=#
postgres=# \set AUTOCOMMIT OFF
postgres=# create table test(i int);
CREATE TABLE
postgres=# \set AUTOCOMMIT ON
\set: Cannot set AUTOCOMMIT to ON inside a transaction, either COMMIT or ROLLBACK and retry
postgres=#
Thank you,
Rahila Syed
Attachment
Hi,
Some minor comments.
+ <note>
+ <para>
+ Autocommit cannot be set on inside a transaction, the ongoing
+ transaction has to be ended by entering <command>COMMIT</> or
+ <command>ROLLBACK</> before setting autocommit on.
+ </para>
+ </note>
I guess: "cannot be set *to* on" and likewise for "setting autocommit on"
The error message further in the patch spells it correctly:
+ psql_error("\\%s: Cannot set %s to %s inside a transaction, ...
Also (maybe): s/Autocommit/AUTOCOMMIT/g
Thanks,
Amit
On Fri, Sep 2, 2016 at 1:12 PM, Rahila Syed <rahilasyed90@gmail.com> wrote:
IMO, in this case BEGIN is explicitly specified by user, so I think it is understood that a commit is required for changes to be effective.Corrected in the attached.Corrected in the attached.Hello,Thank you for comments.
>Above test not throwing psql error, as you used strcmp(newval,"ON"). You
>should use pg_strcasecmp.
>Above error coming because in below code block change, you don't have check for
>command (you should check opt0 for AUTOCOMMIT command)
>postgres=# BEGIN;
>BEGIN
>postgres=# create table foo ( a int );
>CREATE TABLE
>postgres=# \set AUTOCOMMIT ON
>Don't you think, in above case also we should throw a psql error?Hence I did not consider this case.
Document doesn't say this changes are only for implicit BEGIN..
+ <note>
+ <para>
+ Autocommit cannot be set on inside a transaction, the ongoing
+ transaction has to be ended by entering <command>COMMIT</> or
+ <command>ROLLBACK</> before setting autocommit on.
+ </para>
+ </note>
+ <note>
+ <para>
+ Autocommit cannot be set on inside a transaction, the ongoing
+ transaction has to be ended by entering <command>COMMIT</> or
+ <command>ROLLBACK</> before setting autocommit on.
+ </para>
+ </note>
In my opinion, its good to be consistent, whether its implicit or explicitly specified BEGIN.
>postgres=# \set AUTOCOMMIT off
>postgres=# create table foo ( a int );
>CREATE TABLE
>postgres=# \set XYZ ON
>\set: Cannot set XYZ to ON inside a transaction, either COMMIT or ROLLBACK and retry
>May be you would like to move the new code block inside SetVariable(). So that
>don't need to worry about the validity of the variable names.I think validating variable names wont be required if we throw error only if command is \set AUTOCOMMIT.Validation can happen later as in the existing code.
It might happen that SetVariable() can be called from other places in future,
so its good to have all the set variable related checks inside SetVariable().
so its good to have all the set variable related checks inside SetVariable().
Also you can modify the condition in such way, so that we don't often end up
calling PQtransactionStatus() even though its not require.
Example:
if (!strcmp(opt0,"AUTOCOMMIT") &&
!strcasecmp(newval,"ON") &&
!pset.autocommit &&
PQtransactionStatus(pset.db) == PQTRANS_INTRANS)
if (!strcmp(opt0,"AUTOCOMMIT") &&
!strcasecmp(newval,"ON") &&
!pset.autocommit &&
PQtransactionStatus(pset.db) == PQTRANS_INTRANS)
>Basically if I understand correctly, if we are within transaction and someone
>tries the set the AUTOCOMMIT ON, it should throw a psql error. Correct me
>if I am missing anything?Yes the psql_error is thrown when AUTOCOMMIT is turned on inside a transaction. But only when there is an implicit BEGIN as in following case,
postgres=# \set AUTOCOMMIT OFF
postgres=# create table test(i int);
CREATE TABLE
postgres=# \set AUTOCOMMIT ON
\set: Cannot set AUTOCOMMIT to ON inside a transaction, either COMMIT or ROLLBACK and retry
postgres=#Thank you,Rahila Syed
Regards,
Rushabh Lathia
>Document doesn't say this changes are only for implicit BEGIN..
Correcting myself here. Not just implicit BEGIN, it will throw an error on \set AUTOCOMMIT inside any any transaction provided earlier value of AUTOCOMMIT was OFF. Probably the test in which you tried it was already ON.
Thank you,
Rahila Syed
On Fri, Sep 2, 2016 at 3:17 PM, Rushabh Lathia <rushabh.lathia@gmail.com> wrote:
On Fri, Sep 2, 2016 at 1:12 PM, Rahila Syed <rahilasyed90@gmail.com> wrote:IMO, in this case BEGIN is explicitly specified by user, so I think it is understood that a commit is required for changes to be effective.Corrected in the attached.Corrected in the attached.Hello,Thank you for comments.
>Above test not throwing psql error, as you used strcmp(newval,"ON"). You
>should use pg_strcasecmp.
>Above error coming because in below code block change, you don't have check for
>command (you should check opt0 for AUTOCOMMIT command)
>postgres=# BEGIN;
>BEGIN
>postgres=# create table foo ( a int );
>CREATE TABLE
>postgres=# \set AUTOCOMMIT ON
>Don't you think, in above case also we should throw a psql error?Hence I did not consider this case.Document doesn't say this changes are only for implicit BEGIN..
+ <note>
+ <para>
+ Autocommit cannot be set on inside a transaction, the ongoing
+ transaction has to be ended by entering <command>COMMIT</> or
+ <command>ROLLBACK</> before setting autocommit on.
+ </para>
+ </note>In my opinion, its good to be consistent, whether its implicit or explicitly specified BEGIN.>postgres=# \set AUTOCOMMIT off
>postgres=# create table foo ( a int );
>CREATE TABLE
>postgres=# \set XYZ ON
>\set: Cannot set XYZ to ON inside a transaction, either COMMIT or ROLLBACK and retry
>May be you would like to move the new code block inside SetVariable(). So that
>don't need to worry about the validity of the variable names.I think validating variable names wont be required if we throw error only if command is \set AUTOCOMMIT.Validation can happen later as in the existing code.It might happen that SetVariable() can be called from other places in future,
so its good to have all the set variable related checks inside SetVariable().Also you can modify the condition in such way, so that we don't often end upcalling PQtransactionStatus() even though its not require.Example:
if (!strcmp(opt0,"AUTOCOMMIT") &&
!strcasecmp(newval,"ON") &&
!pset.autocommit &&
PQtransactionStatus(pset.db) == PQTRANS_INTRANS)>Basically if I understand correctly, if we are within transaction and someone
>tries the set the AUTOCOMMIT ON, it should throw a psql error. Correct me
>if I am missing anything?Yes the psql_error is thrown when AUTOCOMMIT is turned on inside a transaction. But only when there is an implicit BEGIN as in following case,
postgres=# \set AUTOCOMMIT OFF
postgres=# create table test(i int);
CREATE TABLE
postgres=# \set AUTOCOMMIT ON
\set: Cannot set AUTOCOMMIT to ON inside a transaction, either COMMIT or ROLLBACK and retry
postgres=#Thank you,Rahila SyedRegards,Rushabh Lathia
Rahila Syed wrote: > >Above error coming because in below code block change, you don't have > check for > >command (you should check opt0 for AUTOCOMMIT command) > Corrected in the attached. There are other values than ON: true/yes and theirs abbreviations, the value 1, and anything that doesn't resolve to OFF is taken as ON. ParseVariableBool() in commands.c already does the job of converting these to bool, the new code should probably just call that function instead of parsing the value itself. Best regards, -- Daniel Vérité PostgreSQL-powered mailer: http://www.manitou-mail.org Twitter: @DanielVerite
Rushabh Lathia wrote:
> It might happen that SetVariable() can be called from other places in
> future,
> so its good to have all the set variable related checks inside
> SetVariable().
There's already another way to skip the \set check: select 'on' as "AUTOCOMMIT" \gset
But there's a function in startup.c which might be the ideal location
for the check, as it looks like the one and only place where the
autocommit flag actually changes:
static void
autocommit_hook(const char *newval)
{ pset.autocommit = ParseVariableBool(newval, "AUTOCOMMIT");
}
Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite
"Daniel Verite" <daniel@manitou-mail.org> writes:
> Rushabh Lathia wrote:
>> so its good to have all the set variable related checks inside
>> SetVariable().
> There's already another way to skip the \set check:
> select 'on' as "AUTOCOMMIT" \gset
Hmm, that might be a bug.
regards, tom lane
Thank you for comments.
>But there's a function in startup.c which might be the ideal location
>or the check, as it looks like the one and only place where the
>autocommit flag actually changes:
>static void
>autocommit_hook(const char *newval)
Thank you for pointing out this. This indeed is the only place where autocommit setting changes in psql. However, >But there's a function in startup.c which might be the ideal location
>or the check, as it looks like the one and only place where the
>autocommit flag actually changes:
>static void
>autocommit_hook(const char *newval)
including the check here will require returning the status of command from this hook. i.e if we throw error inside this
hook we will need to return false indicating the value has not changed. This will change the existing definition of the hook
which returns void. This will also lead to changes in other implementations of this hook apart from autocommit_hook.
>There are other values than ON: true/yes and theirs abbreviations,
>the value 1, and anything that doesn't resolve to OFF is taken as ON.
>ParseVariableBool() in commands.c already does the job of converting
>these to bool, the new code should probably just call that function
>instead of parsing the value itself.
I have included this in the attached patch.
Also I have included prior review comments by Rushabh Lathia and Amit Langote in the attached patch
Regarding suggestion to move the code inside SetVariable(), I think it is not a good idea because
SetVariable() and variable.c file in which it is present is about handling of psql variables, checks for
correct variable names, values etc. This check is about correctness of the command so it is better placed
in exec_command().
Thank you,
Rahila Syed
On Sat, Sep 3, 2016 at 4:39 PM, Daniel Verite <daniel@manitou-mail.org> wrote:
Rushabh Lathia wrote:
> It might happen that SetVariable() can be called from other places in
> future,
> so its good to have all the set variable related checks inside
> SetVariable().
There's already another way to skip the \set check:
select 'on' as "AUTOCOMMIT" \gset
But there's a function in startup.c which might be the ideal location
for the check, as it looks like the one and only place where the
autocommit flag actually changes:
static void
autocommit_hook(const char *newval)
{
pset.autocommit = ParseVariableBool(newval, "AUTOCOMMIT");}
Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite
Attachment
Rahila Syed wrote:
> However, including the check here will require returning the status
> of command from this hook. i.e if we throw error inside this
> hook we will need to return false indicating the value has not changed.
Looking at the other variables hooks, they already emit errors and can
deny the effect of a change corresponding to a new value, without
informing the caller. Why would autocommit be different?
For example echo_hook does this:
/* ...if the value is in (queries,errors,all,none) then assign pset.echo accordingly ... */
else
{ psql_error("unrecognized value \"%s\" for \"%s\"; assuming \"%s\"\n", newval, "ECHO", "none"); pset.echo =
PSQL_ECHO_NONE;
}
If the user issues \set ECHO FOOBAR, then it produces the above error
message and makes the same internal change as if \set ECHO none
had been issued.
But, the value of the variable as seen by the user is still FOOBAR:
\set
[...other variables...]
ECHO = 'FOOBAR'
The proposed patch doesn't follow that behavior, as it denies
a new value for AUTOCOMMIT. You might argue that it's better,
but on the other side, there are two reasons against it:
- it's not consistent with the other uses of \set that affect psql,
such as ECHO, ECHO_HIDDEN, ON_ERROR_ROLLBACK,COMP_KEYWORD_CASE... and even AUTOCOMMIT as\set AUTOCOMMIT FOOBAR is not
denied,just reinterpreted.
- it doesn't address the case of another method than \set being usedto set the variable, as in : SELECT 'on' as
"AUTOCOMMIT"\gsetwhereas the hook would work in that case.
Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite
Hello,
>Looking at the other variables hooks, they already emit errors and can
>deny the effect of a change corresponding to a new value, without
>deny the effect of a change corresponding to a new value, without
>informing the caller. Why would autocommit be different?
The only type of psql_error inside hooks is as follows,
psql_error("unrecognized value \"%s\" for \"%s\"; assuming \"%s\"\n",
newval, "ECHO", "none");
newval, "ECHO", "none");
These instances where psql_error occurs inside hooks the command is
successful and the value supplied by user is reinterpreted to some other
value as user had supplied an unrecognisable value.
With psql_error_on_autocommit patch what was intended was to make
the command unsuccessful and keep the previous setting of autocommit.
Hence having it inside autocommit_hook did not seem appropriate to me.
But as pointed out by you, the other way of setting autocommit i,e
SELECT 'on' as "AUTOCOMMIT" \gset will not be handled by the patch.
So I will change the patch to have the check in the autocommit_hook instead.
This will mean if \set AUTOCOMMIT ON or SELECT 'on' as "AUTOCOMMIT" \gset
is run inside a transaction, it will be effective after current transaction has
ended. Appropriate message will be displayed notifying this to the user and user need not
rerun the set AUTOCOMMIT command.
Thank you,
Rahila Syed
On Thu, Sep 8, 2016 at 9:55 PM, Daniel Verite <daniel@manitou-mail.org> wrote:
Rahila Syed wrote:
> However, including the check here will require returning the status
> of command from this hook. i.e if we throw error inside this
> hook we will need to return false indicating the value has not changed.
Looking at the other variables hooks, they already emit errors and can
deny the effect of a change corresponding to a new value, without
informing the caller. Why would autocommit be different?
For example echo_hook does this:
/* ...if the value is in (queries,errors,all,none) then
assign pset.echo accordingly ... */
else
{
psql_error("unrecognized value \"%s\" for \"%s\"; assuming \"%s\"\n",
newval, "ECHO", "none");
pset.echo = PSQL_ECHO_NONE;
}
If the user issues \set ECHO FOOBAR, then it produces the above error
message and makes the same internal change as if \set ECHO none
had been issued.
But, the value of the variable as seen by the user is still FOOBAR:
\set
[...other variables...]
ECHO = 'FOOBAR'
The proposed patch doesn't follow that behavior, as it denies
a new value for AUTOCOMMIT. You might argue that it's better,
but on the other side, there are two reasons against it:
- it's not consistent with the other uses of \set that affect psql,
such as ECHO, ECHO_HIDDEN, ON_ERROR_ROLLBACK,
COMP_KEYWORD_CASE... and even AUTOCOMMIT as
\set AUTOCOMMIT FOOBAR is not denied, just reinterpreted.
- it doesn't address the case of another method than \set being used
to set the variable, as in : SELECT 'on' as "AUTOCOMMIT" \gset
whereas the hook would work in that case.
Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite
Rahila Syed <rahilasyed90@gmail.com> writes:
>> Looking at the other variables hooks, they already emit errors and can
>> deny the effect of a change corresponding to a new value, without
>> informing the caller. Why would autocommit be different?
> These instances where psql_error occurs inside hooks the command is
> successful and the value supplied by user is reinterpreted to some other
> value as user had supplied an unrecognisable value.
> With psql_error_on_autocommit patch what was intended was to make
> the command unsuccessful and keep the previous setting of autocommit.
> Hence having it inside autocommit_hook did not seem appropriate to me.
Nonetheless, asking all callers of SetVariable to deal with such cases
is entirely unmaintainable/unacceptable. Have you considered expanding
the API for hook functions? I'm not really sure why we didn't provide a
way for the hooks to reject a setting to begin with.
Actually, it would make a lot more sense UI-wise if attempting to assign a
non-boolean value to a boolean variable resulted in an error and no change
to the variable, instead of what happens now.
Anyway, I'm not very thrilled with the idea that AUTOCOMMIT is so special
that it should have a different behavior than any other built-in psql
variable. If we make them all throw errors and refuse to change to bad
values, that would be consistent and defensible IMO. But having
AUTOCOMMIT alone act that way is not a feature, it's a wart.
regards, tom lane
>Have you considered expanding
>the API for hook functions?
>the API for hook functions?
Changing the hooks API to allow rejecting a setting and return false is certainly useful
to other psql variables wanting to report an error and reject a value.
I did not consider expanding hook APIs because there was no requirement in sight for other
variables to reject a setting. As far as autocommit is concerned something in line with the current design can be implemented.
In the current design, any unrecognisable/bad value is reinterpreted and the execution inside hook is always
successful.
In keeping with current design of hooks instead of rejecting autocommit 'ON' setting inside
a transaction,the value can be set to 'ON' with a psql_error displaying that the value
will be effective when the current transaction has ended.
>Actually, it would make a lot more sense UI-wise if attempting to assign a
>non-boolean value to a boolean variable resulted in an error and no change
>to the variable, instead of what happens now.
>Actually, it would make a lot more sense UI-wise if attempting to assign a
>non-boolean value to a boolean variable resulted in an error and no change
>to the variable, instead of what happens now.
Hooks API can be expanded to implement this.
The proposed feature is mainly to reduce the ambiguity for the user when
\set AUTOCOMMIT on is run within a transaction. According to current behaviour,
the variable is set immediately but it is effective only when the current transaction
has ended. It is good to notify this to the user.
This ambiguity in the behaviour was highlighted because in AUTOCOMMIT off mode Postgres
implicitly starts a transaction and behaviour of \set AUTOCOMMIT ON in such scenario can
be confusing.
Thank you,
Rahila Syed
On Wed, Sep 14, 2016 at 8:06 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Rahila Syed <rahilasyed90@gmail.com> writes:
>> Looking at the other variables hooks, they already emit errors and can
>> deny the effect of a change corresponding to a new value, without
>> informing the caller. Why would autocommit be different?
> These instances where psql_error occurs inside hooks the command is
> successful and the value supplied by user is reinterpreted to some other
> value as user had supplied an unrecognisable value.
> With psql_error_on_autocommit patch what was intended was to make
> the command unsuccessful and keep the previous setting of autocommit.
> Hence having it inside autocommit_hook did not seem appropriate to me.
Nonetheless, asking all callers of SetVariable to deal with such cases
is entirely unmaintainable/unacceptable. Have you considered expanding
the API for hook functions? I'm not really sure why we didn't provide a
way for the hooks to reject a setting to begin with.
Actually, it would make a lot more sense UI-wise if attempting to assign a
non-boolean value to a boolean variable resulted in an error and no change
to the variable, instead of what happens now.
Anyway, I'm not very thrilled with the idea that AUTOCOMMIT is so special
that it should have a different behavior than any other built-in psql
variable. If we make them all throw errors and refuse to change to bad
values, that would be consistent and defensible IMO. But having
AUTOCOMMIT alone act that way is not a feature, it's a wart.
regards, tom lane
On Thu, Sep 15, 2016 at 7:29 AM, Rahila Syed <rahilasyed90@gmail.com> wrote: > In keeping with current design of hooks instead of rejecting autocommit 'ON' > setting inside > a transaction,the value can be set to 'ON' with a psql_error displaying that > the value > will be effective when the current transaction has ended. Hmm, that seems like a reasonable compromise. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, Sep 15, 2016 at 7:29 AM, Rahila Syed <rahilasyed90@gmail.com> wrote:
>> In keeping with current design of hooks instead of rejecting autocommit 'ON'
>> setting inside
>> a transaction,the value can be set to 'ON' with a psql_error displaying that
>> the value
>> will be effective when the current transaction has ended.
> Hmm, that seems like a reasonable compromise.
I dunno, implementing that seems like it will require some very fragile
behavior in the autocommit code, ie that even though the variable is "on"
we don't do anything until after reaching an out-of-transaction state.
It might work today but I'm afraid we'd break it in future.
I think changing the hook API is a pretty reasonable thing to do here
(though I'd make it its own patch and then add the autocommit change
on top). When was the last time you actually wanted to set VERBOSITY
to "fubar"?
regards, tom lane
On Thu, Sep 15, 2016 at 11:10 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Thu, Sep 15, 2016 at 7:29 AM, Rahila Syed <rahilasyed90@gmail.com> wrote: >>> In keeping with current design of hooks instead of rejecting autocommit 'ON' >>> setting inside >>> a transaction,the value can be set to 'ON' with a psql_error displaying that >>> the value >>> will be effective when the current transaction has ended. > >> Hmm, that seems like a reasonable compromise. > > I dunno, implementing that seems like it will require some very fragile > behavior in the autocommit code, ie that even though the variable is "on" > we don't do anything until after reaching an out-of-transaction state. > It might work today but I'm afraid we'd break it in future. Hmm, I don't think any logic change is being proposed, just a warning that it may not work the way you think. So I don't think it would be any more fragile than now. Am I missing something? > I think changing the hook API is a pretty reasonable thing to do here > (though I'd make it its own patch and then add the autocommit change > on top). When was the last time you actually wanted to set VERBOSITY > to "fubar"? I agree that'd be better but I don't know that we should expect Rahila to do that as a condition of getting a usability warning accepted. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, Sep 15, 2016 at 11:10 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Robert Haas <robertmhaas@gmail.com> writes:
>>> On Thu, Sep 15, 2016 at 7:29 AM, Rahila Syed <rahilasyed90@gmail.com> wrote:
>>>> In keeping with current design of hooks instead of rejecting
>>>> autocommit 'ON' setting inside a transaction,the value can be set to
>>>> 'ON' with a psql_error displaying that the value will be effective
>>>> when the current transaction has ended.
>>> Hmm, that seems like a reasonable compromise.
>> I dunno, implementing that seems like it will require some very fragile
>> behavior in the autocommit code, ie that even though the variable is "on"
>> we don't do anything until after reaching an out-of-transaction state.
>> It might work today but I'm afraid we'd break it in future.
> Hmm, I don't think any logic change is being proposed, just a warning
> that it may not work the way you think. So I don't think it would be
> any more fragile than now. Am I missing something?
As I see it, up to now we never considered what would happen if you
changed the variable's setting mid-transaction. The fact that it works
like this is an implementation artifact. Now that we are considering it,
we should ask ourselves if we like that artifact and are willing to commit
to keeping it working like that forevermore. I'm not sure that the answer
to either one is "yes". I think throwing an actual error would be
preferable.
>> I think changing the hook API is a pretty reasonable thing to do here
>> (though I'd make it its own patch and then add the autocommit change
>> on top). When was the last time you actually wanted to set VERBOSITY
>> to "fubar"?
> I agree that'd be better but I don't know that we should expect Rahila
> to do that as a condition of getting a usability warning accepted.
If it's something that would end up getting thrown away after someone
does the API change, accepting a warning-only patch doesn't seem all
that useful. But I just work here.
regards, tom lane
Tom Lane wrote: > I think changing the hook API is a pretty reasonable thing to do here > (though I'd make it its own patch and then add the autocommit change > on top). When was the last time you actually wanted to set VERBOSITY > to "fubar"? It looks easy to make the hooks return a bool, and when returning false, cancel the assignment of the variable. I volunteer to write that patch. It would close the hazard that exists today of an internal psql flag getting decorrelated from the corresponding variable, due to feeding it with a wrong value, or in the case of autocommit, in the wrong context. Also with that, the behavior of ParseVariableBool() assuming "on" when it's being fed with junk could be changed. Instead it could just reject the assignment rather than emit a warning, and both the internal flag and the variable would keep the values they had at the point of the bogus assignment. Best regards, -- Daniel Vérité PostgreSQL-powered mailer: http://www.manitou-mail.org Twitter: @DanielVerite
"Daniel Verite" <daniel@manitou-mail.org> writes:
> It looks easy to make the hooks return a bool, and when returning false,
> cancel the assignment of the variable.
Yeah, that's about how I was envisioning it too.
> I volunteer to write that patch.
Thanks.
> It would close the hazard that exists today of an internal psql flag
> getting decorrelated from the corresponding variable, due to feeding
> it with a wrong value, or in the case of autocommit, in the wrong
> context.
If we want to get to the situation that the variable's value always
reflects the internal state, then we should reject deleting the variable
too. Currently that's allowed; I was unsure whether we should still
permit it, but this line of thought says no.
Another issue is what to do in SetVariableAssignHook(). Currently,
that's only invoked immediately after creating the variable space
so it will always go through the hook(NULL) call, which the hook
will think means deletion and then fail for, given the above change.
What might be cleanest is to modify SetVariableAssignHook so that it
is passed both the initial value and the hook (deleting the separate
initial-value-setting calls that currently exist near startup.c:40).
If the hook rejects the initial value, that would be grounds for
fatal exit.
> Also with that, the behavior of ParseVariableBool() assuming "on"
> when it's being fed with junk could be changed.
Right.
regards, tom lane