Re: Surprising behaviour of \set AUTOCOMMIT ON

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 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 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 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

www.EnterpriseDB.com