Thread: revisiting transaction isolation

revisiting transaction isolation

From
Oliver Jowett
Date:
Currently, this type of code will fail:

>   conn.setAutoCommit(false);
>   if (conn.getTransactionIsolation() != Connection.TRANSACTION_SERIALIZABLE)
>     conn.setTransactionIsolation(Connection.TRANSACTION_SERIALIZABLE);

The problem is that getTransactionIsolation() issues a query and thus
starts a new transaction, and then setTransactionIsolation() complains
you can't change isolation level mid-transaction.

I'm not sure this is reasonable behaviour. One option is to make
getTransactionIsolation (and what other methods too?) not cause a BEGIN
to occur if there is no transaction in progress and autocommit is off.

...

On a related topic I just took a look at the JDBC3 spec and it says:

> The result of invoking the method setTransactionIsolation in the middle
> of a transaction is implementation-defined.
>
> The return value of the method getTransactionIsolation should reflect
> the change in isolation level when it actually occurs. It is recommended
> that drivers implement the setTransactionIsolation method to change the
> isolation level starting with the next transaction. Committing the
> current transaction to make the effect immediate is also a valid
> implementation.

Should we follow the recommendation and have setTransactionIsolation()
defer until the next transaction when called mid-transaction? i.e. store
"current" vs "requested" isolation level; changing isolation level
mid-transaction only changes the requested level, and after
commit/rollback we issue an appropriate SET if current != expected.

-O


Re: revisiting transaction isolation

From
Kris Jurka
Date:

On Sat, 17 Jul 2004, Oliver Jowett wrote:

> Currently, this type of code will fail:
>
> >   conn.setAutoCommit(false);
> >   if (conn.getTransactionIsolation() != Connection.TRANSACTION_SERIALIZABLE)
> >     conn.setTransactionIsolation(Connection.TRANSACTION_SERIALIZABLE);
>
> The problem is that getTransactionIsolation() issues a query and thus
> starts a new transaction, and then setTransactionIsolation() complains
> you can't change isolation level mid-transaction.
>
> I'm not sure this is reasonable behaviour. One option is to make
> getTransactionIsolation (and what other methods too?) not cause a BEGIN
> to occur if there is no transaction in progress and autocommit is off.

I see no reason for getTransactionIsolation or any driver call to start a
transaction, these are only SELECTs and won't be rolled back anyway.

> On a related topic I just took a look at the JDBC3 spec and it says:
>
> > The return value of the method getTransactionIsolation should reflect
> > the change in isolation level when it actually occurs. It is recommended
> > that drivers implement the setTransactionIsolation method to change the
> > isolation level starting with the next transaction. Committing the
> > current transaction to make the effect immediate is also a valid
> > implementation.

This seems confusing and error prone.  I would expect a command I issue to
take effect immediately or throw an Exception, not do nothing now, but
alter later behavior.

Kris Jurka

Re: revisiting transaction isolation

From
Oliver Jowett
Date:
Kris Jurka wrote:
>
> On Sat, 17 Jul 2004, Oliver Jowett wrote:
>
>
>>Currently, this type of code will fail:
>>
>>
>>>  conn.setAutoCommit(false);
>>>  if (conn.getTransactionIsolation() != Connection.TRANSACTION_SERIALIZABLE)
>>>    conn.setTransactionIsolation(Connection.TRANSACTION_SERIALIZABLE);
>>
>>The problem is that getTransactionIsolation() issues a query and thus
>>starts a new transaction, and then setTransactionIsolation() complains
>>you can't change isolation level mid-transaction.
>>
>>I'm not sure this is reasonable behaviour. One option is to make
>>getTransactionIsolation (and what other methods too?) not cause a BEGIN
>>to occur if there is no transaction in progress and autocommit is off.
>
>
> I see no reason for getTransactionIsolation or any driver call to start a
> transaction, these are only SELECTs and won't be rolled back anyway.

It's ok for getTransactionIsolation(), but what about, say, metadata
queries -- we do want transaction isolation to apply there even if it's
just a SELECT, right?

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.

>>>The return value of the method getTransactionIsolation should reflect
>>>the change in isolation level when it actually occurs. [...]
>
> This seems confusing and error prone.

Ok -- I'll leave that alone too.

-O

Re: revisiting transaction isolation

From
"Dario V. Fassi"
Date:

The code bellow throw an exception and I can see any transaction in progress.

Dario.

        String url   = getJdbcUrl();
        try {
            isCon = false;
            con = DriverManager.getConnection( url, usr, pwd );
            if ( con == null ) return false;
            isCon = true;
            con.setAutoCommit( autoCommit );
            int tiso  = con.getTransactionIsolation();
            try {
                    dmd = con.getMetaData();
                    int defTxIsolation  = -1;
                    if ( dmd != null && dmd.supportsTransactionIsolationLevel( tranIsolation ) ) {
                        defTxIsolation = dmd.getDefaultTransactionIsolation();
                    }
                    if ( tranIsolation >= 0 && tranIsolation != defTxIsolation ) {
                        tranIsolation = defTxIsolation;
                    }
                }
                if ( tranIsolation >= 0 ) {
                    con.setTransactionIsolation( tranIsolation );
                    tiso = con.getTransactionIsolation();
                }
            } catch ( SQLException se ) {
                se.printStackTrace();
                lastErrMsg = "\n ERROR: TransactionIsolation No Soportado = " + Tools.InfoException( se );
            }


Oliver Jowett wrote:
Kris Jurka wrote:

On Sat, 17 Jul 2004, Oliver Jowett wrote:


Currently, this type of code will fail:


 conn.setAutoCommit(false);
 if (conn.getTransactionIsolation() != Connection.TRANSACTION_SERIALIZABLE)
   conn.setTransactionIsolation(Connection.TRANSACTION_SERIALIZABLE);

The problem is that getTransactionIsolation() issues a query and thus starts a new transaction, and then setTransactionIsolation() complains you can't change isolation level mid-transaction.

I'm not sure this is reasonable behaviour. One option is to make getTransactionIsolation (and what other methods too?) not cause a BEGIN to occur if there is no transaction in progress and autocommit is off.


I see no reason for getTransactionIsolation or any driver call to start a transaction, these are only SELECTs and won't be rolled back anyway.

It's ok for getTransactionIsolation(), but what about, say, metadata queries -- we do want transaction isolation to apply there even if it's just a SELECT, right?

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.

The return value of the method getTransactionIsolation should reflect
the change in isolation level when it actually occurs. [...]

This seems confusing and error prone.

Ok -- I'll leave that alone too.

-O



Re: revisiting transaction isolation

From
Oliver Jowett
Date:
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);
      }


Re: revisiting transaction isolation

From
Kris Jurka
Date:

On Tue, 20 Jul 2004, Oliver Jowett wrote:

> 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.
>

Applied.