Re: Bug in psql (on_error_rollback) - Mailing list pgsql-patches

Good patch. You are right that the original code did not consider that
AUTOCOMMIT would change the transaction state seen by the
ON_ERROR_ROLLBACK check.

Fixed in CVS.  ON_ERROR_ROLLBACK was not in 8.0.X so no need to backpatch.

---------------------------------------------------------------------------

Michael Paesold wrote:
> There is a bug in psql for the new ON_ERROR_ROLLBACK feature. In AUTOCOMMIT
> off mode it does not work correctly for the first statement.
>
> This is how it works usually:
> ====================================
> postgres=# \set AUTOCOMMIT off
> postgres=# \set ON_ERROR_ROLLBACK interactive
> postgres=# SELECT 1;
>  ?column?
> ----------
>         1
> (1 row)
>
> postgres=# SELECT a;
> ERROR:  column "a" does not exist
> postgres=# SELECT 1;
>  ?column?
> ----------
>         1
> (1 row)
>
> postgres=# BEGIN;
> WARNING:  there is already a transaction in progress
> BEGIN
> postgres=# ROLLBACK;
> ROLLBACK
> ====================================
>
> For the first statement in a transaction after the implicit BEGIN it does
> not work:
> ====================================
> postgres=# ROLLBACK;
> ROLLBACK
> postgres=#
> postgres=# SELECT a;
> ERROR:  column "a" does not exist
> postgres=# SELECT 1;
> ERROR:  current transaction is aborted, commands ignored until end of
> transaction block
> ====================================
>
> With the attaced patch it works correctly even for the first statement.
> ====================================
> postgres=# \set AUTOCOMMIT off
> postgres=# \set ON_ERROR_ROLLBACK interactive
> postgres=# SELECT a;
> ERROR:  column "a" does not exist
> postgres=# SELECT 1;
>  ?column?
> ----------
>         1
> (1 row)
>
> postgres=# BEGIN;
> WARNING:  there is already a transaction in progress
> BEGIN
> postgres=# ABORT;
> ROLLBACK
> ====================================
>
> Please check the patch and apply to CVS tip.
> I think it would be good to add regression tests for AUTOCOMMIT and
> ON_ERROR_ROLLBACK and possibly others. There are currently no regression
> tests specifically for psql features, but since the regression tests are
> executed via psql, there would be no problem in creating a set of such
> tests, right?. I could write some.
>
> Best Regards,
> Michael Paesold

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 2: Don't 'kill -9' the postmaster

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073
Index: src/bin/psql/common.c
===================================================================
RCS file: /cvsroot/pgsql/src/bin/psql/common.c,v
retrieving revision 1.104
diff -c -c -r1.104 common.c
*** src/bin/psql/common.c    22 Jun 2005 21:14:30 -0000    1.104
--- src/bin/psql/common.c    20 Sep 2005 21:41:45 -0000
***************
*** 1010,1022 ****
              return false;
          }
          PQclear(results);
      }
!     else if (transaction_status == PQTRANS_INTRANS &&
!              (rollback_str = GetVariable(pset.vars, "ON_ERROR_ROLLBACK")) != NULL &&
!              /* !off and !interactive is 'on' */
!              pg_strcasecmp(rollback_str, "off") != 0 &&
!              (pset.cur_cmd_interactive ||
!               pg_strcasecmp(rollback_str, "interactive") != 0))
      {
          if (on_error_rollback_warning == false && pset.sversion < 80000)
          {
--- 1010,1024 ----
              return false;
          }
          PQclear(results);
+         transaction_status = PQtransactionStatus(pset.db);
      }
!
!     if (transaction_status == PQTRANS_INTRANS &&
!         (rollback_str = GetVariable(pset.vars, "ON_ERROR_ROLLBACK")) != NULL &&
!         /* !off and !interactive is 'on' */
!         pg_strcasecmp(rollback_str, "off") != 0 &&
!         (pset.cur_cmd_interactive ||
!          pg_strcasecmp(rollback_str, "interactive") != 0))
      {
          if (on_error_rollback_warning == false && pset.sversion < 80000)
          {