Surprising behaviour of \set AUTOCOMMIT ON
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?
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
Show quoted text
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
Show quoted text
--
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
Show quoted text
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
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.
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".
I feel that raising an error is a sane move here -- it is reasonable to
insist that the user make their intention unambiguous.
Cheers,
BJ
On Sat, 6 Aug 2016 at 15:30 Amit Kapila <amit.kapila16@gmail.com> wrote:
Show quoted text
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
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.
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:
Show quoted text
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. WarningAs 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, 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. WarningAs 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 myopinion.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 subsequentcommands 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. WarningAs 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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. WarningAs 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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:
Show quoted text
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. WarningAs 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 subsequentcommands 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--
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. WarningAs 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 myopinion.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 subsequentcommands 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
On Thu, Aug 11, 2016 at 2:11 AM, Venkata Balaji N <nag1010@gmail.com> wrote:
[...]
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Aug 12, 2016 at 3:03 PM, Robert Haas <robertmhaas@gmail.com> wrote:
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 thecorrect
solution - or to respond in a way conditional to the present usage
(interactive vs. script). I disagree with Robert's unsubstantiatedbelief
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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. This can be
user unfriendly making user rerun all the commands just because of
autocommit switch. So probably behaviour of 'ON_ERROR_ROLLBACK on' needs to
be implemented along with the error display. This will rollback just the
autocommit switch command.
Also, psql error instead of a simple commit will lead to script
terminations. Hence issuing a COMMIT seems more viable here. However,
script termination can be avoided by default behaviour of ON_ERROR_STOP
which will execute subsequent commands successfully.(However subsequent
commands won't be executed in autocommit mode which I think should be OK as
it will be notified via ERROR).
So summarizing my view of the discussion on this thread, issuing a psql
error seems to be the best option. I will post a patch regarding this if
there is no objection.
Thank you,
Rahila Syed
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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 Syed
On Wed, Aug 17, 2016 at 6:31 PM, Robert Haas <robertmhaas@gmail.com> wrote:
Show quoted text
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
Attachments:
psql_error_on_autocommit.patchapplication/x-download; name=psql_error_on_autocommit.patchDownload
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 8a66ce7..573f61d 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -3089,6 +3089,14 @@ bar
without committing, your work will be lost.
</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>
<note>
<para>
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index a9a2fdb..914298c 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -1294,7 +1294,9 @@ exec_command(const char *cmd,
*/
char *newval;
char *opt;
+ PGTransactionStatusType transaction_status;
+ transaction_status = PQtransactionStatus(pset.db);
opt = psql_scan_slash_option(scan_state,
OT_NORMAL, NULL, false);
newval = pg_strdup(opt ? opt : "");
@@ -1308,7 +1310,14 @@ exec_command(const char *cmd,
free(opt);
}
- 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;
+ }
+ else if (!SetVariable(pset.vars, opt0, newval))
{
psql_error("\\%s: error while setting variable\n", cmd);
success = false;
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?
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.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
--
Rushabh Lathia
Hello,
Thank you for comments.
Above test not throwing psql error, as you used strcmp(newval,"ON"). You
should use pg_strcasecmp.
Corrected in the attached.
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.
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?
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.
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.
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=#
Thank you,
Rahila Syed
Attachments:
psql_error_on_autocommit_v1.patchapplication/x-download; name=psql_error_on_autocommit_v1.patchDownload
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 8a66ce7..573f61d 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -3089,6 +3089,14 @@ bar
without committing, your work will be lost.
</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>
<note>
<para>
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index a9a2fdb..e045bb4 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -1294,7 +1294,9 @@ exec_command(const char *cmd,
*/
char *newval;
char *opt;
+ PGTransactionStatusType transaction_status;
+ transaction_status = PQtransactionStatus(pset.db);
opt = psql_scan_slash_option(scan_state,
OT_NORMAL, NULL, false);
newval = pg_strdup(opt ? opt : "");
@@ -1308,7 +1310,13 @@ exec_command(const char *cmd,
free(opt);
}
- if (!SetVariable(pset.vars, opt0, newval))
+ if (transaction_status == PQTRANS_INTRANS && !pset.autocommit &&
+ !strcasecmp(newval,"ON") && !strcmp(opt0,"AUTOCOMMIT"))
+ {
+ psql_error("\\%s: Cannot set %s to %s inside a transaction, either COMMIT or ROLLBACK and retry\n", cmd, opt0, newval);
+ success = false;
+ }
+ else if (!SetVariable(pset.vars, opt0, newval))
{
psql_error("\\%s: error while setting variable\n", cmd);
success = false;
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Sep 2, 2016 at 1:12 PM, Rahila Syed <rahilasyed90@gmail.com> wrote:
Hello,
Thank you for comments.
Above test not throwing psql error, as you used strcmp(newval,"ON"). You
should use pg_strcasecmp.Corrected in the attached.
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.
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?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.
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 orROLLBACK 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 up
calling 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
someonetries 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
www.EnterpriseDB.com
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:
Show quoted text
On Fri, Sep 2, 2016 at 1:12 PM, Rahila Syed <rahilasyed90@gmail.com>
wrote:Hello,
Thank you for comments.
Above test not throwing psql error, as you used strcmp(newval,"ON"). You
should use pg_strcasecmp.Corrected in the attached.
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.
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?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.
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 orROLLBACK 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
up
calling 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
someonetries 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
www.EnterpriseDB.com
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
--
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 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
"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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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,
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:
Show quoted text
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" \gsetBut 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
Attachments:
psql_error_on_autocommit_v2.patchapplication/x-download; name=psql_error_on_autocommit_v2.patchDownload
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 8a66ce7..d99945d 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -3089,6 +3089,14 @@ bar
without committing, your work will be lost.
</para>
</note>
+
+ <note>
+ <para>
+ The autocommit cannot be set to on inside a transaction, the ongoing
+ transaction has to be ended by entering <command>COMMIT</> or
+ <command>ROLLBACK</> before setting autocommit to on.
+ </para>
+ </note>
<note>
<para>
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index a9a2fdb..64b0cd5 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -1308,7 +1308,14 @@ exec_command(const char *cmd,
free(opt);
}
- if (!SetVariable(pset.vars, opt0, newval))
+ if (!strcmp(opt0,"AUTOCOMMIT") && ParseVariableBool(newval,
+ "AUTOCOMMIT") && PQtransactionStatus(pset.db) == PQTRANS_INTRANS)
+ {
+ psql_error("\\%s: Cannot set %s to %s inside a transaction, either COMMIT or ROLLBACK and retry\n",
+ cmd, opt0, newval);
+ success = false;
+ }
+ else if (!SetVariable(pset.vars, opt0, newval))
{
psql_error("\\%s: error while setting variable\n", cmd);
success = false;
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
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");
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:
Show quoted text
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Have you considered expanding
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.
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:
Show quoted text
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Sep 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
"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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers