Thread: revisiting transaction isolation
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
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
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
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
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);
}
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.