Re: Slightly bogus regression test for contrib/dblink - Mailing list pgsql-hackers

Tom Lane wrote:
> Lines 509-512 of contrib/dblink/expected/dblink.out read:
>
> -- this should fail because there is no open transaction
> SELECT dblink_exec('myconn','DECLARE xact_test CURSOR FOR SELECT * FROM foo');
> ERROR:  sql error
> DETAIL:  ERROR:  cursor "xact_test" already exists
>
> The error message is not consistent with what the comment claims.
> Looking at the test case in total, I think the code is responding

Actually the comment was correct, but there was a bug which caused the
automatically opened transaction to not get closed.

I was really expecting a "DECLARE CURSOR may only be used in transaction
blocks" error there, but didn't notice that I was actually getting a
different error message :-(.

The bug can be reproduced by using dblink_open to automatically open a
transaction, and then using dblink_exec to manually ABORT it:

-- open a test connection
SELECT dblink_connect('myconn','dbname=contrib_regression');
  dblink_connect
----------------
  OK
(1 row)

-- open a cursor incorrectly; this bumps up the refcount
contrib_regression=# SELECT
dblink_open('myconn','rmt_foo_cursor','SELECT * FROM foobar',false);
NOTICE:  sql error
DETAIL:  ERROR:  relation "foobar" does not exist

  dblink_open
-------------
  ERROR
(1 row)

-- manually abort remote transaction; does not maintain refcount
SELECT dblink_exec('myconn','ABORT');
  dblink_exec
-------------
  ROLLBACK
(1 row)

-- test automatic transaction; this bumps up the refcount
SELECT dblink_open('myconn','rmt_foo_cursor','SELECT * FROM foo');
  dblink_open
-------------
  OK
(1 row)

-- this should commit the automatically opened transaction
-- but it doesn't because the refcount is wrong
SELECT dblink_close('myconn','rmt_foo_cursor');
  dblink_close
--------------
  OK
(1 row)

-- this should fail because there is no open transaction
-- but it doesn't because dblink_close did not commit
SELECT dblink_exec('myconn','DECLARE xact_test2 CURSOR FOR SELECT * FROM
foo');
   dblink_exec
----------------
  DECLARE CURSOR
(1 row)

I think the attached patch does the trick in a minimally invasive way.
If there are no objections I'll commit to head and 8.1 stable branches.
The problem doesn't exist before 8.1.

> BTW, I was led to notice this while examining the current buildfarm
> failure report from osprey,
> http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=osprey&dt=2006-06-17%2004:00:16
> It looks to me like the diffs are consistent with the idea that the test
> is using a copy of dblink that predates this patch ... do you agree?
> If so, anyone have an idea how that could happen?  I thought we'd fixed
> all the rpath problems, and anyway osprey wasn't failing like this
> before today.

I haven't really looked at the buildfarm before -- I might be blind, but
I couldn't figure out how to see the regression.diff file.

Joe


? .deps
? .regression.diffs.swp
? current.diff
? dblink.sql
? libdblink.so.0.0
? results
Index: dblink.c
===================================================================
RCS file: /cvsroot/pgsql/contrib/dblink/dblink.c,v
retrieving revision 1.55
diff -c -r1.55 dblink.c
*** dblink.c    30 May 2006 22:12:12 -0000    1.55
--- dblink.c    20 Jun 2006 16:28:01 -0000
***************
*** 361,366 ****
--- 361,373 ----
              DBLINK_RES_INTERNALERROR("begin error");
          PQclear(res);
          rconn->newXactForCursor = TRUE;
+         /*
+          * Since transaction state was IDLE, we force cursor count to
+          * initially be 0. This is needed as a previous ABORT might
+          * have wiped out our transaction without maintaining the
+          * cursor count for us.
+          */
+         rconn->openCursorCount = 0;
      }

      /* if we started a transaction, increment cursor count */
Index: expected/dblink.out
===================================================================
RCS file: /cvsroot/pgsql/contrib/dblink/expected/dblink.out,v
retrieving revision 1.16
diff -c -r1.16 dblink.out
*** expected/dblink.out    18 Oct 2005 02:55:49 -0000    1.16
--- expected/dblink.out    20 Jun 2006 16:28:01 -0000
***************
*** 509,515 ****
  -- this should fail because there is no open transaction
  SELECT dblink_exec('myconn','DECLARE xact_test CURSOR FOR SELECT * FROM foo');
  ERROR:  sql error
! DETAIL:  ERROR:  cursor "xact_test" already exists

  -- reset remote transaction state
  SELECT dblink_exec('myconn','ABORT');
--- 509,515 ----
  -- this should fail because there is no open transaction
  SELECT dblink_exec('myconn','DECLARE xact_test CURSOR FOR SELECT * FROM foo');
  ERROR:  sql error
! DETAIL:  ERROR:  DECLARE CURSOR may only be used in transaction blocks

  -- reset remote transaction state
  SELECT dblink_exec('myconn','ABORT');