Re: Surprising behaviour of \set AUTOCOMMIT ON

From: Rushabh Lathia <rushabh(dot)lathia(at)gmail(dot)com>
To: Rahila Syed <rahilasyed90(at)gmail(dot)com>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Surprising behaviour of \set AUTOCOMMIT ON
Date: 2016-09-02 06:16:25
Message-ID: CAGPqQf1XS4KTNNSvZ7B3RMjBENFOSf=SFpdv2rV8X8MQ3_RLOA@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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(at)gmail(dot)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 Syed
>
> On Wed, Aug 17, 2016 at 6:31 PM, Robert Haas <robertmhaas(at)gmail(dot)com>
> wrote:
>
>> On Tue, Aug 16, 2016 at 5:25 PM, Rahila Syed <rahilasyed90(at)gmail(dot)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(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>

--
Rushabh Lathia

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Ashutosh Bapat 2016-09-02 06:22:39 Re: Declarative partitioning - another take
Previous Message Amit Langote 2016-09-02 06:08:18 Re: Declarative partitioning - another take