Revert "fix: convert silent rollbacks into exception if application s…

…ends commit command (pgjdbc#1729)" This reverts commit adcb194. we still want to do this but it is a breaking change and we will introduce this change in 42.3.0
davecramer · Mar 30, 2020 · 732b744 · 732b744
1 parent 49d66e4
commit 732b744
Show file tree

Hide file tree

Showing 16 changed files with 9 additions and 463 deletions.
diff --git a/docs/documentation/head/connect.md b/docs/documentation/head/connect.md
@@ -220,15 +220,6 @@ Connection conn = DriverManager.getConnection(url);
 
     The default is 'false'
 
-* **raiseExceptionOnSilentRollback** = boolean
-
-    since 42.2.11
-
-    Certain database versions perform a silent rollback instead of commit in case the transaction was in a failed state.
-    See https://www.postgresql.org/message-id/b9fb50dc-0f6e-15fb-6555-8ddb86f4aa71%40postgresfriends.org
-
-    The default is 'true'
-
 * **binaryTransfer** = boolean
 
 	Use binary format for sending and receiving data if possible.

diff --git a/pgjdbc/src/main/java/org/postgresql/PGProperty.java b/pgjdbc/src/main/java/org/postgresql/PGProperty.java
@@ -407,14 +407,6 @@ public enum PGProperty {
     false,
     new String[] {"3"}),
 
-  /**
-   * Certain database versions perform a silent rollback instead of commit in case the transaction was in a failed state.
-   */
-  RAISE_EXCEPTION_ON_SILENT_ROLLBACK(
-    "raiseExceptionOnSilentRollback",
-    "true",
-    "Certain database versions perform a silent rollback instead of commit in case the transaction was in a failed state"),
-
   /**
    * Puts this connection in read-only mode.
    */

diff --git a/pgjdbc/src/main/java/org/postgresql/core/QueryExecutor.java b/pgjdbc/src/main/java/org/postgresql/core/QueryExecutor.java
@@ -444,10 +444,6 @@ Object createQueryKey(String sql, boolean escapeProcessing, boolean isParameteri
 
   boolean willHealOnRetry(SQLException e);
 
-  boolean isRaiseExceptionOnSilentRollback();
-
-  void setRaiseExceptionOnSilentRollback(boolean raiseExceptionOnSilentRollback);
-
   /**
    * By default, the connection resets statement cache in case deallocate all/discard all
    * message is observed.

diff --git a/pgjdbc/src/main/java/org/postgresql/core/QueryExecutorBase.java b/pgjdbc/src/main/java/org/postgresql/core/QueryExecutorBase.java
@@ -48,7 +48,6 @@ public abstract class QueryExecutorBase implements QueryExecutor {
   private AutoSave autoSave;
   private boolean flushCacheOnDeallocate = true;
   protected final boolean logServerErrorDetail;
-  private boolean raiseExceptionOnSilentRollback;
 
   // default value for server versions that don't report standard_conforming_strings
   private boolean standardConformingStrings = false;
@@ -409,16 +408,6 @@ public void setFlushCacheOnDeallocate(boolean flushCacheOnDeallocate) {
     this.flushCacheOnDeallocate = flushCacheOnDeallocate;
   }
 
-  @Override
-  public boolean isRaiseExceptionOnSilentRollback() {
-    return raiseExceptionOnSilentRollback;
-  }
-
-  @Override
-  public void setRaiseExceptionOnSilentRollback(boolean raiseExceptionOnSilentRollback) {
-    this.raiseExceptionOnSilentRollback = raiseExceptionOnSilentRollback;
-  }
-
   protected boolean hasNotifications() {
     return notifications.size() > 0;
   }

diff --git a/pgjdbc/src/main/java/org/postgresql/core/v3/QueryExecutorImpl.java b/pgjdbc/src/main/java/org/postgresql/core/v3/QueryExecutorImpl.java
@@ -67,7 +67,6 @@
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.logging.Level;
 import java.util.logging.Logger;
-import java.util.regex.Pattern;
 
 /**
  * QueryExecutor implementation for the V3 protocol.
@@ -76,23 +75,6 @@ public class QueryExecutorImpl extends QueryExecutorBase {
 
   private static final Logger LOGGER = Logger.getLogger(QueryExecutorImpl.class.getName());
   private static final String COPY_ERROR_MESSAGE = "COPY commands are only supported using the CopyManager API.";
-  private static final Pattern ROLLBACK_PATTERN = Pattern.compile("\\brollback\\b", Pattern.CASE_INSENSITIVE);
-  private static final Pattern COMMIT_PATTERN = Pattern.compile("\\bcommit\\b", Pattern.CASE_INSENSITIVE);
-  private static final Pattern PREPARE_PATTERN = Pattern.compile("\\bprepare ++transaction\\b", Pattern.CASE_INSENSITIVE);
-
-  private static boolean looksLikeCommit(String sql) {
-    if ("COMMIT".equalsIgnoreCase(sql)) {
-      return true;
-    }
-    if ("ROLLBACK".equalsIgnoreCase(sql)) {
-      return false;
-    }
-    return COMMIT_PATTERN.matcher(sql).find() && !ROLLBACK_PATTERN.matcher(sql).find();
-  }
-
-  private static boolean looksLikePrepare(String sql) {
-    return sql.startsWith("PREPARE TRANSACTION") || PREPARE_PATTERN.matcher(sql).find();
-  }
 
   /**
    * TimeZone of the current connection (TimeZone backend parameter).
@@ -373,6 +355,7 @@ private boolean sendAutomaticSavepoint(Query query, int flags) throws IOExceptio
     if (((flags & QueryExecutor.QUERY_SUPPRESS_BEGIN) == 0
         || getTransactionState() == TransactionState.OPEN)
         && query != restoreToAutoSave
+        && !query.getNativeSql().equalsIgnoreCase("COMMIT")
         && getAutoSave() != AutoSave.NEVER
         // If query has no resulting fields, it cannot fail with 'cached plan must not change result type'
         // thus no need to set a savepoint before such query
@@ -2183,36 +2166,8 @@ protected void processResults(ResultHandler handler, int flags) throws IOExcepti
           SimpleQuery currentQuery = executeData.query;
           Portal currentPortal = executeData.portal;
 
-          String nativeSql = currentQuery.getNativeQuery().nativeSql;
-          // Certain backend versions (e.g. 12.2, 11.7, 10.12, 9.6.17, 9.5.21, etc)
-          // silently rollback the transaction in the response to COMMIT statement
-          // in case the transaction has failed.
-          // See discussion in pgsql-hackers: https://www.postgresql.org/message-id/b9fb50dc-0f6e-15fb-6555-8ddb86f4aa71%40postgresfriends.org
-          if (isRaiseExceptionOnSilentRollback()
-              && handler.getException() == null
-              && status.startsWith("ROLLBACK")) {
-            String message = null;
-            if (looksLikeCommit(nativeSql)) {
-              if (transactionFailCause == null) {
-                message = GT.tr("The database returned ROLLBACK, so the transaction cannot be committed. Transaction failure is not known (check server logs?)");
-              } else {
-                message = GT.tr("The database returned ROLLBACK, so the transaction cannot be committed. Transaction failure cause is <<{0}>>", transactionFailCause.getMessage());
-              }
-            } else if (looksLikePrepare(nativeSql)) {
-              if (transactionFailCause == null) {
-                message = GT.tr("The database returned ROLLBACK, so the transaction cannot be prepared. Transaction failure is not known (check server logs?)");
-              } else {
-                message = GT.tr("The database returned ROLLBACK, so the transaction cannot be prepared. Transaction failure cause is <<{0}>>", transactionFailCause.getMessage());
-              }
-            }
-            if (message != null) {
-              handler.handleError(
-                  new PSQLException(
-                      message, PSQLState.IN_FAILED_SQL_TRANSACTION, transactionFailCause));
-            }
-          }
-
           if (status.startsWith("SET")) {
+            String nativeSql = currentQuery.getNativeQuery().nativeSql;
             // Scan only the first 1024 characters to
             // avoid big overhead for long queries.
             if (nativeSql.lastIndexOf("search_path", 1024) != -1

diff --git a/pgjdbc/src/main/java/org/postgresql/ds/common/BaseDataSource.java b/pgjdbc/src/main/java/org/postgresql/ds/common/BaseDataSource.java
@@ -1461,22 +1461,6 @@ public void setAutosave(AutoSave autoSave) {
     PGProperty.AUTOSAVE.set(properties, autoSave.value());
   }
 
-  /**
-   * @return connection configuration regarding throwing exception from commit if database rolls back the transaction
-   * @see PGProperty#RAISE_EXCEPTION_ON_SILENT_ROLLBACK
-   */
-  public boolean isRaiseExceptionOnSilentRollback() {
-    return PGProperty.RAISE_EXCEPTION_ON_SILENT_ROLLBACK.getBoolean(properties);
-  }
-
-  /**
-   * @param raiseExceptionOnSilentRollback if the database should throw exception if commit silently rolls back
-   * @see PGProperty#RAISE_EXCEPTION_ON_SILENT_ROLLBACK
-   */
-  public void setRaiseExceptionOnSilentRollback(boolean raiseExceptionOnSilentRollback) {
-    PGProperty.RAISE_EXCEPTION_ON_SILENT_ROLLBACK.set(properties, raiseExceptionOnSilentRollback);
-  }
-
   /**
    * see PGProperty#CLEANUP_SAVEPOINTS
    *

diff --git a/pgjdbc/src/main/java/org/postgresql/jdbc/PgConnection.java b/pgjdbc/src/main/java/org/postgresql/jdbc/PgConnection.java
@@ -247,10 +247,6 @@ public PgConnection(HostSpec[] hostSpecs,
       LOGGER.log(Level.FINEST, "    integer date/time = {0}", queryExecutor.getIntegerDateTimes());
     }
 
-    queryExecutor.setRaiseExceptionOnSilentRollback(
-        PGProperty.RAISE_EXCEPTION_ON_SILENT_ROLLBACK.getBoolean(info)
-    );
-
     //
     // String -> text or unknown?
     //

diff --git a/pgjdbc/src/main/java/org/postgresql/xa/PGXAConnection.java b/pgjdbc/src/main/java/org/postgresql/xa/PGXAConnection.java
@@ -647,9 +647,6 @@ private int mapSQLStateToXAErrorCode(SQLException sqlException) {
     if (isPostgreSQLIntegrityConstraintViolation(sqlException)) {
       return XAException.XA_RBINTEGRITY;
     }
-    if (PSQLState.IN_FAILED_SQL_TRANSACTION.getState().equals(sqlException.getSQLState())) {
-      return XAException.XA_RBOTHER;
-    }
 
     return XAException.XAER_RMFAIL;
   }

diff --git a/pgjdbc/src/test/java/org/postgresql/test/jdbc2/AutoRollbackTestSuite.java b/pgjdbc/src/test/java/org/postgresql/test/jdbc2/AutoRollbackTestSuite.java
@@ -162,9 +162,6 @@ public void setUp() throws Exception {
   public void tearDown() throws SQLException {
     try {
       con.setAutoCommit(true);
-    } catch (Exception ignored) {
-    }
-    try {
       TestUtil.dropTable(con, "rollbacktest");
     } catch (Exception e) {
       e.printStackTrace();
@@ -295,46 +292,15 @@ public void run() throws SQLException {
     switch (continueMode) {
       case COMMIT:
         try {
-          TransactionState transactionState = pgConnection.getTransactionState();
           doCommit();
-          Assert.assertNotEquals(
-              ".commit() should throw exception since the transaction was in failed state",
-                TransactionState.FAILED, transactionState);
-          Assert.assertEquals("Transaction should be IDLE after .commit()",
-              TransactionState.IDLE, pgConnection.getTransactionState());
-          // Commit might fail in case the transaction is in aborted state
+          // No assert here: commit should always succeed with exception of well known failure cases in catch
         } catch (SQLException e) {
-          if (!PSQLState.INVALID_SQL_STATEMENT_NAME.getState().equals(e.getSQLState())) {
-            // In preferQueryMode=extendedCacheEverything mode it might be that "commit"
-            // statement is using server-prepared mode, and it might result in
-            // ERROR: prepared statement "S_4" does not exist
-            try {
-              Assert.assertEquals("Transaction should be IDLE after .commit()",
-                  TransactionState.IDLE, pgConnection.getTransactionState());
-            } catch (AssertionError ae) {
-              ae.initCause(e);
-              throw ae;
-            }
-          }
-          if (PSQLState.IN_FAILED_SQL_TRANSACTION.getState().equals(e.getSQLState())
-              && (!flushCacheOnDeallocate && DEALLOCATES.contains(failMode)
-              || autoCommit == AutoCommit.NO)) {
-            // We have two cases here:
-            // 1) autocommit=false, so transaction was in progress, so it is expected that commit
-            //    fails with IN_FAILED_SQL_TRANSACTION
-            // 2) commit might fail due to <<ERROR: prepared statement "S_4" does not exist>>
-            //    However, if flushCacheOnDeallocate is false, then autosave can't really work, so
-            //    it is expected that commit fails
-            // Commit terminates the transaction, so "autosave" can't really "rollback"
-            return;
-          }
           if (!flushCacheOnDeallocate && DEALLOCATES.contains(failMode)
               && autoSave == AutoSave.NEVER) {
             Assert.assertEquals(
                 "flushCacheOnDeallocate is disabled, thus " + failMode + " should cause 'prepared statement \"...\" does not exist'"
                     + " error message is " + e.getMessage(),
                 PSQLState.INVALID_SQL_STATEMENT_NAME.getState(), e.getSQLState());
-            // Commit terminates the transaction, so "autosave" can't really "rollback"
             return;
           }
           throw e;

diff --git a/pgjdbc/src/test/java/org/postgresql/test/jdbc2/BatchExecuteTest.java b/pgjdbc/src/test/java/org/postgresql/test/jdbc2/BatchExecuteTest.java
@@ -8,7 +8,6 @@
 import org.postgresql.PGProperty;
 import org.postgresql.PGStatement;
 import org.postgresql.test.TestUtil;
-import org.postgresql.util.PSQLState;
 
 import org.junit.Assert;
 import org.junit.Test;
@@ -239,13 +238,13 @@ public void testSelectInBatch() throws Exception {
   }
 
   @Test
-  public void testSelectInBatchThrowsAutoCommit() throws Throwable {
+  public void testSelectInBatchThrowsAutoCommit() throws Exception {
     con.setAutoCommit(true);
     testSelectInBatchThrows();
   }
 
   @Test
-  public void testSelectInBatchThrows() throws Throwable {
+  public void testSelectInBatchThrows() throws Exception {
     Statement stmt = con.createStatement();
 
     int oldValue = getCol1Value();
@@ -262,13 +261,7 @@ public void testSelectInBatchThrows() throws Throwable {
     }
 
     if (!con.getAutoCommit()) {
-      try {
-        con.commit();
-      } catch (SQLException e) {
-        if (!PSQLState.IN_FAILED_SQL_TRANSACTION.getState().equals(e.getSQLState())) {
-          throw e;
-        }
-      }
+      con.commit();
     }
 
     int newValue = getCol1Value();

diff --git a/pgjdbc/src/test/java/org/postgresql/test/jdbc2/BatchFailureTest.java b/pgjdbc/src/test/java/org/postgresql/test/jdbc2/BatchFailureTest.java
@@ -7,7 +7,6 @@
 
 import org.postgresql.PGProperty;
 import org.postgresql.test.TestUtil;
-import org.postgresql.util.PSQLState;
 
 import org.junit.Assert;
 import org.junit.Test;
@@ -243,14 +242,7 @@ public void run() throws SQLException {
     }
 
     if (!con.getAutoCommit()) {
-      try {
-        // Commit might fail if the transaction is in aborted state
-        con.commit();
-      } catch (SQLException e) {
-        if (!PSQLState.IN_FAILED_SQL_TRANSACTION.getState().equals(e.getSQLState())) {
-          throw e;
-        }
-      }
+      con.commit();
     }
 
     int finalCount = getBatchUpdCount();

diff --git a/pgjdbc/src/test/java/org/postgresql/test/jdbc2/Jdbc2TestSuite.java b/pgjdbc/src/test/java/org/postgresql/test/jdbc2/Jdbc2TestSuite.java
@@ -118,7 +118,6 @@
     TimeTest.class,
     TimezoneCachingTest.class,
     TimezoneTest.class,
-    TransactionTest.class,
     TypeCacheDLLStressTest.class,
     UpdateableResultTest.class,
     UpsertTest.class,

diff --git a/pgjdbc/src/test/java/org/postgresql/test/jdbc2/MiscTest.java b/pgjdbc/src/test/java/org/postgresql/test/jdbc2/MiscTest.java
@@ -9,9 +9,7 @@
 import static org.junit.Assert.fail;
 
 import org.postgresql.test.TestUtil;
-import org.postgresql.util.PSQLState;
 
-import org.junit.Assert;
 import org.junit.Ignore;
 import org.junit.Test;
 
@@ -90,15 +88,7 @@ public void testError() throws Exception {
       oos.close();
     }
 
-    try {
-      con.commit();
-    } catch (SQLException e) {
-      Assert.assertEquals(
-          "Transaction is in failed state, so .commit() should throw",
-          PSQLState.IN_FAILED_SQL_TRANSACTION.getState(),
-          e.getSQLState()
-      );
-    }
+    con.commit();
     con.close();
   }