XA transactions and autocommit

Hi there,

I've been playing around with JTA using Apache Geronimo and the
postgresql-8.1-407.jdbc3.jar driver. I've noticed a minor problem.
The driver's XA code disables autocommit and assumes it will remain
so indefinitely. tranql-connection (used by Geronimo for pooling)
enables autocommit whenever the connection is returned to the pool.
(Specifically in its ManagedXAConnection.cleanup method.) According
to the J2EE Connector spec, this is apparently the correct thing to do.

I didn't notice the problem until I tried rolling back a transaction,
where I discovered it was being committed instead. Eventually I
traced it to the fact that autocommit was on.

Arguably, if you follow what the connector spec has to say about
autocommit and XA transactions, autocommit should be on by default
and should only be explicitly disabled during the duration of a
transaction.

I've made a minor patch against 407 that does just this.
XAResource.start disables autocommit, and autocommit is then re-
enabled at the appropriate place within the prepare, commit, and
rollback methods.

When searching the mailing list, I found this, which might be related:

     http://archives.postgresql.org/pgsql-jdbc/2006-05/msg00104.php

I don't think there was any resolution.

The patch follows (though my mailer seems to be munging the empty
lines) and can also be found here:

     http://www.saddi.com/software/tranql-postgresql/postgresql-
PGXAConnection-autocommit.diff

Regards,
- Allan

--- org/postgresql/test/xa/XADataSourceTest.java.orig    2006-04-26
11:59:31.000000000 -0700
+++ org/postgresql/test/xa/XADataSourceTest.java    2006-10-07
08:35:08.000000000 -0700
@@ -217,6 +217,37 @@
          xaRes.rollback(xid);
      }

+    public void testAutoCommit() throws Exception {
+        Xid xid = new CustomXid(6);
+
+        assertTrue(conn.getAutoCommit());
+
+        xaRes.start(xid, XAResource.TMNOFLAGS);
+        assertFalse(conn.getAutoCommit());
+        xaRes.end(xid, XAResource.TMSUCCESS);
+        assertFalse(conn.getAutoCommit());
+        xaRes.commit(xid, true);
+        assertTrue(conn.getAutoCommit());
+
+        xaRes.start(xid, XAResource.TMNOFLAGS);
+        xaRes.end(xid, XAResource.TMSUCCESS);
+        xaRes.prepare(xid);
+        assertTrue(conn.getAutoCommit());
+        xaRes.commit(xid, false);
+        assertTrue(conn.getAutoCommit());
+
+        xaRes.start(xid, XAResource.TMNOFLAGS);
+        xaRes.end(xid, XAResource.TMSUCCESS);
+        xaRes.rollback(xid);
+        assertTrue(conn.getAutoCommit());
+
+        xaRes.start(xid, XAResource.TMNOFLAGS);
+        xaRes.end(xid, XAResource.TMSUCCESS);
+        xaRes.prepare(xid);
+        xaRes.rollback(xid);
+        assertTrue(conn.getAutoCommit());
+    }
+
      /* We don't support transaction interleaving.
      public void testInterleaving1() throws Exception {
       Xid xid1 = new CustomXid(1);
--- org/postgresql/xa/PGXAConnection.java.orig    2006-04-26
11:59:31.000000000 -0700
+++ org/postgresql/xa/PGXAConnection.java    2006-10-07
08:32:01.000000000 -0700
@@ -48,9 +48,8 @@

      PGXAConnection(BaseConnection conn) throws SQLException
      {
-        super(conn, false, true);
+        super(conn, true, true);
          this.conn = conn;
-        this.conn.setAutoCommit(false);
          this.state = STATE_IDLE;
      }

@@ -104,6 +103,15 @@
          if (state == STATE_ENDED)
              throw new PGXAException(GT.tr("Transaction interleaving
not implemented"), XAException.XAER_RMERR);

+        try
+        {
+            conn.setAutoCommit(false);
+        }
+        catch (SQLException ex)
+        {
+            throw new PGXAException(GT.tr("Error disabling
autocommit"), ex, XAException.XAER_RMERR);
+        }
+
          // Preconditions are met, Associate connection with the
transaction
          state = STATE_ACTIVE;
          currentXid = xid;
@@ -190,6 +198,7 @@
              {
                  stmt.close();
              }
+            conn.setAutoCommit(true);

              return XA_OK;
          }
@@ -278,27 +287,21 @@
                  state = STATE_IDLE;
                  currentXid = null;
                  conn.rollback();
+                conn.setAutoCommit(true);
              }
              else
              {
                  String s = RecoveredXid.xidToString(xid);

                  conn.setAutoCommit(true);
+                Statement stmt = conn.createStatement();
                  try
                  {
-                    Statement stmt = conn.createStatement();
-                    try
-                    {
-                        stmt.executeUpdate("ROLLBACK PREPARED '" + s
+ "'");
-                    }
-                    finally
-                    {
-                        stmt.close();
-                    }
+                    stmt.executeUpdate("ROLLBACK PREPARED '" + s +
"'");
                  }
                  finally
                  {
-                    conn.setAutoCommit(false);
+                    stmt.close();
                  }
              }
          }
@@ -350,6 +353,7 @@
              currentXid = null;

              conn.commit();
+            conn.setAutoCommit(true);
          }
          catch (SQLException ex)
          {
@@ -378,21 +382,14 @@
              String s = RecoveredXid.xidToString(xid);

              conn.setAutoCommit(true);
+            Statement stmt = conn.createStatement();
              try
              {
-                Statement stmt = conn.createStatement();
-                try
-                {
-                    stmt.executeUpdate("COMMIT PREPARED '" + s + "'");
-                }
-                finally
-                {
-                    stmt.close();
-                }
+                stmt.executeUpdate("COMMIT PREPARED '" + s + "'");
              }
              finally
              {
-                conn.setAutoCommit(false);
+                stmt.close();
              }
          }
          catch (SQLException ex)