Re: revisiting transaction isolation

Oliver Jowett wrote:

> A quick skim of AbstractJdbc2Connection turns up these methods as
> candidates for ignoring autocommit:
>
>  - getTransactionIsolation()
>  - getPGType (both versions) on a cache miss
>
> It seems reasonable to suppress BEGINs for both of those cases. I can
> put together a patch to do that. I'll leave the metadata queries alone
> for the moment.

And here is the patch.

I reworked execSQLQuery / execSQLUpdate to always ignore autocommit,
since all places they're called from want this behaviour. Also tweaked
them to detect unexpected cases (no resultset from query, resultset from
update).

-O
Index: org/postgresql/core/BaseConnection.java
===================================================================
RCS file: /usr/local/cvsroot/pgjdbc/pgjdbc/org/postgresql/core/BaseConnection.java,v
retrieving revision 1.8
diff -u -c -r1.8 BaseConnection.java
*** org/postgresql/core/BaseConnection.java    29 Jun 2004 06:43:24 -0000    1.8
--- org/postgresql/core/BaseConnection.java    20 Jul 2004 08:35:05 -0000
***************
*** 31,36 ****
--- 31,37 ----

      /**
       * Execute a SQL query that returns a single resultset.
+      * Never causes a new transaction to be started regardless of the autocommit setting.
       *
       * @param s the query to execute
       * @return the (non-null) returned resultset
***************
*** 40,45 ****
--- 41,47 ----

      /**
       * Execute a SQL query that does not return results.
+      * Never causes a new transaction to be started regardless of the autocommit setting.
       *
       * @param s the query to execute
       * @throws SQLException if something goes wrong.
Index: org/postgresql/core/BaseStatement.java
===================================================================
RCS file: /usr/local/cvsroot/pgjdbc/pgjdbc/org/postgresql/core/BaseStatement.java,v
retrieving revision 1.13
diff -u -c -r1.13 BaseStatement.java
*** org/postgresql/core/BaseStatement.java    16 Jul 2004 09:07:53 -0000    1.13
--- org/postgresql/core/BaseStatement.java    20 Jul 2004 08:35:05 -0000
***************
*** 53,56 ****
--- 53,65 ----
       * @throws SQLException if something goes wrong.
       */
      public boolean executeWithFlags(String p_sql, int flags) throws SQLException;
+
+     /**
+      * Execute a prepared query, passing additional query flags.
+      *
+      * @param flags additional {@link QueryExecutor} flags for execution; these
+      *  are bitwise-ORed into the default flags.
+      * @throws SQLException if something goes wrong.
+      */
+     public boolean executeWithFlags(int flags) throws SQLException;
  }
Index: org/postgresql/jdbc2/AbstractJdbc2Connection.java
===================================================================
RCS file: /usr/local/cvsroot/pgjdbc/pgjdbc/org/postgresql/jdbc2/AbstractJdbc2Connection.java,v
retrieving revision 1.13
diff -u -c -r1.13 AbstractJdbc2Connection.java
*** org/postgresql/jdbc2/AbstractJdbc2Connection.java    16 Jul 2004 09:07:57 -0000    1.13
--- org/postgresql/jdbc2/AbstractJdbc2Connection.java    20 Jul 2004 08:35:06 -0000
***************
*** 175,184 ****
      /**
       * Simple query execution.
       */
!     public ResultSet execSQLQuery(String s) throws SQLException
!     {
!         Statement stat = (Statement) createStatement();
!         boolean hasResultSet = stat.execute(s);

          // Transfer warnings to the connection, since the user never
          // has a chance to see the statement itself.
--- 175,189 ----
      /**
       * Simple query execution.
       */
!     public ResultSet execSQLQuery(String s) throws SQLException {
!         BaseStatement stat = (BaseStatement) createStatement();
!         boolean hasResultSet = stat.executeWithFlags(s, QueryExecutor.QUERY_SUPPRESS_BEGIN);
!
!         while (!hasResultSet && stat.getUpdateCount() != -1)
!             hasResultSet = stat.getMoreResults();
!
!         if (!hasResultSet)
!             throw new PSQLException("postgresql.stat.noresult", PSQLState.NO_DATA);

          // Transfer warnings to the connection, since the user never
          // has a chance to see the statement itself.
***************
*** 186,206 ****
          if (warnings != null)
              addWarning(warnings);

-         while (!hasResultSet && stat.getUpdateCount() != -1)
-             hasResultSet = stat.getMoreResults();
-
          return stat.getResultSet();
      }

!     public void execSQLUpdate(String s) throws SQLException
!     {
!         execSQLUpdate(s, 0);
!     }
!
!     private void execSQLUpdate(String s, int flags) throws SQLException
!     {
          BaseStatement stmt = (BaseStatement) createStatement();
!         stmt.executeWithFlags(s, QueryExecutor.QUERY_NO_METADATA | QueryExecutor.QUERY_NO_RESULTS | flags);

          // Transfer warnings to the connection, since the user never
          // has a chance to see the statement itself.
--- 191,203 ----
          if (warnings != null)
              addWarning(warnings);

          return stat.getResultSet();
      }

!     public void execSQLUpdate(String s) throws SQLException {
          BaseStatement stmt = (BaseStatement) createStatement();
!         if (stmt.executeWithFlags(s, QueryExecutor.QUERY_NO_METADATA | QueryExecutor.QUERY_NO_RESULTS |
QueryExecutor.QUERY_SUPPRESS_BEGIN))
!             throw new PSQLException("postgresql.stat.result");

          // Transfer warnings to the connection, since the user never
          // has a chance to see the statement itself.
***************
*** 526,532 ****

          if (haveMinimumServerVersion("7.4") && readOnly != this.readOnly) {
              String readOnlySql = "SET SESSION CHARACTERISTICS AS TRANSACTION " + (readOnly ? "READ ONLY" : "READ
WRITE");
!             execSQLUpdate(readOnlySql, QueryExecutor.QUERY_SUPPRESS_BEGIN); // No BEGIN regardles of our autocommit
setting
          }

          this.readOnly = readOnly;
--- 523,529 ----

          if (haveMinimumServerVersion("7.4") && readOnly != this.readOnly) {
              String readOnlySql = "SET SESSION CHARACTERISTICS AS TRANSACTION " + (readOnly ? "READ ONLY" : "READ
WRITE");
!             execSQLUpdate(readOnlySql); // nb: no BEGIN triggered.
          }

          this.readOnly = readOnly;
***************
*** 633,658 ****
      public int getTransactionIsolation() throws SQLException
      {
          String level = null;
-         Statement stmt = createStatement();

!         if (haveMinimumServerVersion("7.3")) {
!             ResultSet rs = stmt.executeQuery("SHOW TRANSACTION ISOLATION LEVEL");
              if (rs.next())
                  level = rs.getString(1);
!
!             // Transfer warnings to the connection, since the user never
!             // has a chance to see the statement itself.
!             SQLWarning warnings = stmt.getWarnings();
!             if (warnings != null)
!                 addWarning(warnings);
          } else {
!             stmt.executeUpdate("SHOW TRANSACTION ISOLATION LEVEL");
!             SQLWarning warning = stmt.getWarnings();
              if (warning != null)
                  level = warning.getMessage();
-         }

!         stmt.close();

          // XXX revisit: throw exception instead of silently eating the error in unkwon cases?
          if (level == null)
--- 630,661 ----
      public int getTransactionIsolation() throws SQLException
      {
          String level = null;

!         if (haveMinimumServerVersion("7.3")) {
!             // 7.3+ returns the level as a query result.
!             ResultSet rs = execSQLQuery("SHOW TRANSACTION ISOLATION LEVEL"); // nb: no BEGIN triggered
              if (rs.next())
                  level = rs.getString(1);
!             rs.close();
          } else {
!             // 7.2 returns the level as an INFO message. Ew.
!             // We juggle the warning chains a bit here.
!
!             // Swap out current warnings.
!             SQLWarning saveWarnings = getWarnings();
!             clearWarnings();
!
!             // Run the query any examine any resulting warnings.
!             execSQLUpdate("SHOW TRANSACTION ISOLATION LEVEL"); // nb: no BEGIN triggered
!             SQLWarning warning = getWarnings();
              if (warning != null)
                  level = warning.getMessage();

!             // Swap original warnings back.
!             clearWarnings();
!             if (saveWarnings != null)
!                 addWarning(saveWarnings);
!         }

          // XXX revisit: throw exception instead of silently eating the error in unkwon cases?
          if (level == null)
***************
*** 694,700 ****
              throw new PSQLException("postgresql.con.isolevel", PSQLState.TRANSACTION_STATE_INVALID, new
Integer(level));

          String isolationLevelSQL = "SET SESSION CHARACTERISTICS AS TRANSACTION ISOLATION LEVEL " +
isolationLevelName;
!         execSQLUpdate(isolationLevelSQL, QueryExecutor.QUERY_SUPPRESS_BEGIN); // No BEGIN regardles of our autocommit
setting
      }

      protected String getIsolationLevelName(int level)
--- 697,703 ----
              throw new PSQLException("postgresql.con.isolevel", PSQLState.TRANSACTION_STATE_INVALID, new
Integer(level));

          String isolationLevelSQL = "SET SESSION CHARACTERISTICS AS TRANSACTION ISOLATION LEVEL " +
isolationLevelName;
!         execSQLUpdate(isolationLevelSQL); // nb: no BEGIN triggered
      }

      protected String getIsolationLevelName(int level)
***************
*** 887,893 ****

              query.setString(1, typeName);

!             BaseResultSet result = (BaseResultSet)query.executeQuery();
              if (result.next()) {
                  oid = result.getInt(1);
                  oidTypeCache.put(new Integer(oid), typeName);
--- 890,899 ----

              query.setString(1, typeName);

!             if (! ((BaseStatement)query).executeWithFlags(QueryExecutor.QUERY_SUPPRESS_BEGIN) )
!                 throw new PSQLException("postgresql.stat.noresult", PSQLState.NO_DATA);
!
!             ResultSet result = query.getResultSet();
              if (result.next()) {
                  oid = result.getInt(1);
                  oidTypeCache.put(new Integer(oid), typeName);
***************
*** 926,932 ****

              query.setInt(1, oid);

!             ResultSet result = query.executeQuery();
              if (result.next()) {
                  typeName = result.getString(1);
                  typeOidCache.put(typeName, new Integer(oid));
--- 932,941 ----

              query.setInt(1, oid);

!             if (! ((BaseStatement)query).executeWithFlags(QueryExecutor.QUERY_SUPPRESS_BEGIN) )
!                 throw new PSQLException("postgresql.stat.noresult", PSQLState.NO_DATA);
!
!             ResultSet result = query.getResultSet();
              if (result.next()) {
                  typeName = result.getString(1);
                  typeOidCache.put(typeName, new Integer(oid));
Index: org/postgresql/jdbc2/AbstractJdbc2ResultSet.java
===================================================================
RCS file: /usr/local/cvsroot/pgjdbc/pgjdbc/org/postgresql/jdbc2/AbstractJdbc2ResultSet.java,v
retrieving revision 1.43
diff -u -c -r1.43 AbstractJdbc2ResultSet.java
*** org/postgresql/jdbc2/AbstractJdbc2ResultSet.java    16 Jul 2004 09:07:59 -0000    1.43
--- org/postgresql/jdbc2/AbstractJdbc2ResultSet.java    20 Jul 2004 08:35:06 -0000
***************
*** 158,163 ****
--- 158,171 ----
                      // Fetch all results.
                      String cursorName = getString(columnIndex);
                      String fetchSql = "FETCH ALL IN \"" + cursorName + "\"";
+
+                     // nb: no BEGIN triggered here. This is fine. If someone
+                     // committed, and the cursor was not holdable (closing the
+                     // cursor), we avoid starting a new xact and promptly causing
+                     // it to fail. If the cursor *was* holdable, we don't want a
+                     // new xact anyway since holdable cursor state isn't affected
+                     // by xact boundaries. If our caller didn't commit at all, or
+                     // autocommit was on, then we wouldn't issue a BEGIN anyway.
                      ResultSet rs = connection.execSQLQuery(fetchSql);
                      ((AbstractJdbc2ResultSet)rs).setRefCursor(cursorName);
                      return rs;
Index: org/postgresql/jdbc2/AbstractJdbc2Statement.java
===================================================================
RCS file: /usr/local/cvsroot/pgjdbc/pgjdbc/org/postgresql/jdbc2/AbstractJdbc2Statement.java,v
retrieving revision 1.27
diff -u -c -r1.27 AbstractJdbc2Statement.java
*** org/postgresql/jdbc2/AbstractJdbc2Statement.java    16 Jul 2004 09:08:00 -0000    1.27
--- org/postgresql/jdbc2/AbstractJdbc2Statement.java    20 Jul 2004 08:35:06 -0000
***************
*** 282,288 ****
          return executeWithFlags(0);
      }

!     private boolean executeWithFlags(int flags) throws SQLException
      {
          checkClosed();
          if (isFunction && !returnTypeSet)
--- 282,288 ----
          return executeWithFlags(0);
      }

!     public boolean executeWithFlags(int flags) throws SQLException
      {
          checkClosed();
          if (isFunction && !returnTypeSet)
Index: org/postgresql/test/jdbc2/ConnectionTest.java
===================================================================
RCS file: /usr/local/cvsroot/pgjdbc/pgjdbc/org/postgresql/test/jdbc2/ConnectionTest.java,v
retrieving revision 1.14
diff -u -c -r1.14 ConnectionTest.java
*** org/postgresql/test/jdbc2/ConnectionTest.java    10 Apr 2004 13:53:11 -0000    1.14
--- org/postgresql/test/jdbc2/ConnectionTest.java    20 Jul 2004 08:35:06 -0000
***************
*** 242,249 ****
              assertEquals(Connection.TRANSACTION_READ_COMMITTED,
                           con.getTransactionIsolation());

-
-             Statement stmt = con.createStatement();
              // Now run some tests with autocommit enabled.
              con.setAutoCommit(true);

--- 242,247 ----
***************
*** 274,280 ****
--- 272,297 ----
              con.setAutoCommit(false);
              assertEquals(Connection.TRANSACTION_READ_COMMITTED,
                           con.getTransactionIsolation());
+             con.commit();
+
+             // Test that getTransactionIsolation() does not actually start a new txn.
+             con.getTransactionIsolation(); // Shouldn't start a new transaction.
+             con.setTransactionIsolation(Connection.TRANSACTION_SERIALIZABLE); // Should be ok -- we're not in a
transaction.
+             con.setTransactionIsolation(Connection.TRANSACTION_READ_COMMITTED); // Should still be ok.
+
+             // Test that we can't change isolation mid-transaction
+             Statement stmt = con.createStatement();
+             stmt.executeQuery("SELECT 1");          // Start transaction.
+             stmt.close();
+
+             try {
+                 con.setTransactionIsolation(Connection.TRANSACTION_SERIALIZABLE);
+                 fail("Expected an exception when changing transaction isolation mid-transaction");
+             } catch (SQLException e) {
+                 // Ok.
+             }

+             con.rollback();
              TestUtil.closeDB(con);
      }