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) {
---------------------------(end of broadcast)--------------------------- TIP 2: Don't 'kill -9' the postmaster