fix: convert silent rollbacks into exception if application sends com…

…mit command (pgjdbc#1729)

The feature is enabled by default.
It can be disabled by setting raiseExceptionOnSilentRollback=false connection property.

See discussion in pgsql-hackers: https://www.postgresql.org/message-id/b9fb50dc-0f6e-15fb-6555-8ddb86f4aa71%40postgresfriends.org

fixes pgjdbc#1697

docs/documentation/head/connect.md

@@ -220,6 +220,15 @@ 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.

pgjdbc/src/main/java/org/postgresql/PGProperty.java

@@ -408,6 +408,14 @@ 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.
    */

pgjdbc/src/main/java/org/postgresql/core/QueryExecutor.java

@@ -444,6 +444,10 @@ 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.

pgjdbc/src/main/java/org/postgresql/core/QueryExecutorBase.java

@@ -48,6 +48,7 @@ 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;
@@ -408,6 +409,16 @@ 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;
   }

pgjdbc/src/main/java/org/postgresql/core/v3/QueryExecutorImpl.java

@@ -67,6 +67,7 @@
 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.
@@ -75,6 +76,23 @@ 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).
@@ -355,7 +373,6 @@ 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
@@ -2166,8 +2183,36 @@ 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

pgjdbc/src/main/java/org/postgresql/ds/common/BaseDataSource.java

@@ -1461,6 +1461,22 @@ 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
    *

pgjdbc/src/main/java/org/postgresql/jdbc/PgConnection.java

@@ -247,6 +247,10 @@ 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?
     //

pgjdbc/src/main/java/org/postgresql/xa/PGXAConnection.java

@@ -647,6 +647,9 @@ 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;
   }

pgjdbc/src/test/java/org/postgresql/test/jdbc2/AutoRollbackTestSuite.java

@@ -162,6 +162,9 @@ 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();
@@ -292,15 +295,46 @@ public void run() throws SQLException {
     switch (continueMode) {
       case COMMIT:
         try {
+          TransactionState transactionState = pgConnection.getTransactionState();
           doCommit();
-          // No assert here: commit should always succeed with exception of well known failure cases in catch
+          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
         } 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;

pgjdbc/src/test/java/org/postgresql/test/jdbc2/BatchExecuteTest.java

@@ -8,6 +8,7 @@
 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;
@@ -238,13 +239,13 @@ public void testSelectInBatch() throws Exception {
   }
 
   @Test
-  public void testSelectInBatchThrowsAutoCommit() throws Exception {
+  public void testSelectInBatchThrowsAutoCommit() throws Throwable {
     con.setAutoCommit(true);
     testSelectInBatchThrows();
   }
 
   @Test
-  public void testSelectInBatchThrows() throws Exception {
+  public void testSelectInBatchThrows() throws Throwable {
     Statement stmt = con.createStatement();
 
     int oldValue = getCol1Value();
@@ -261,7 +262,13 @@ public void testSelectInBatchThrows() throws Exception {
     }
 
     if (!con.getAutoCommit()) {
-      con.commit();
+      try {
+        con.commit();
+      } catch (SQLException e) {
+        if (!PSQLState.IN_FAILED_SQL_TRANSACTION.getState().equals(e.getSQLState())) {
+          throw e;
+        }
+      }
     }
 
     int newValue = getCol1Value();

pgjdbc/src/test/java/org/postgresql/test/jdbc2/BatchFailureTest.java

@@ -7,6 +7,7 @@
 
 import org.postgresql.PGProperty;
 import org.postgresql.test.TestUtil;
+import org.postgresql.util.PSQLState;
 
 import org.junit.Assert;
 import org.junit.Test;
@@ -242,7 +243,14 @@ public void run() throws SQLException {
     }
 
     if (!con.getAutoCommit()) {
-      con.commit();
+      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;
+        }
+      }
     }
 
     int finalCount = getBatchUpdCount();

pgjdbc/src/test/java/org/postgresql/test/jdbc2/Jdbc2TestSuite.java

@@ -118,6 +118,7 @@
     TimeTest.class,
     TimezoneCachingTest.class,
     TimezoneTest.class,
+    TransactionTest.class,
     TypeCacheDLLStressTest.class,
     UpdateableResultTest.class,
     UpsertTest.class,

pgjdbc/src/test/java/org/postgresql/test/jdbc2/MiscTest.java

@@ -9,7 +9,9 @@
 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;
 
@@ -88,7 +90,15 @@ public void testError() throws Exception {
       oos.close();
     }
 
-    con.commit();
+    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.close();
   }