[BACKPORT 2024.1][yugabyte#21361] YSQL: Block query layer retries for…

… multi-statement queries

Summary:
Query layer retries should be blocked for multi-statement queries.
This is because they might have transaction blocks within them that
had committed before a statement faces a kConflict, and a
query layer retry would redo the whole query and hence the
transaction block too, which might not be idempotent. Something
like the below where the second update faces a kConflict:

"begin ... repeatable read; update ...; commit; update ...;"

This diff also does cosmetic changes to log messages related to
query layer retries and error messages that are sent back to the
external client.
Jira: DB-10258

Original commit: b72433e / D33860

Test Plan:
Jenkins

Added yb_query_layer_retries_for_a_multi_statement_query to the yb_pg_isolation_schedule

Reviewers: tfoucher, patnaik.balivada

Reviewed By: patnaik.balivada

Subscribers: yql

Tags: #jenkins-ready

Differential Revision: https://phorge.dev.yugabyte.com/D34633

java/yb-pgsql/src/test/java/org/yb/pgsql/TestPgBatch.java

@@ -294,7 +294,7 @@ protected void testMultipleStatementsPerQueryHelper(boolean extendedProtocol,
       startSignal.await();
       String query = new String();
       if (explicitTransaction) {
-        query = "BEGIN TRANSACTION ISOLATION LEVEL " + isolationLevel.sql + ";";
+        s.execute("BEGIN TRANSACTION ISOLATION LEVEL " + isolationLevel.sql);
       } else {
         s.execute(
             "SET SESSION CHARACTERISTICS AS TRANSACTION ISOLATION LEVEL " + isolationLevel.sql);
@@ -306,10 +306,10 @@ protected void testMultipleStatementsPerQueryHelper(boolean extendedProtocol,
       for (int i = 9; i < 12; i++) {
         query += String.format("UPDATE t SET v=2 WHERE k=%d;", i);
       }
+      s.execute(query);
       if (explicitTransaction) {
-        query += "COMMIT;";
+        s.execute("COMMIT");
       }
-      s.execute(query);
 
       t.join();
       assertFalse(failureDetected.get());
@@ -318,32 +318,36 @@ protected void testMultipleStatementsPerQueryHelper(boolean extendedProtocol,
         expected.add(new Row(i, 2));
       }
       assertRowSet(s, "SELECT * FROM t ORDER BY k", expected);
+      LOG.info("Parameters: extendedProtocol: " + extendedProtocol +
+          ", explicitTransaction: " + explicitTransaction +
+          ", isolationLevel: " + isolationLevel + " success!");
     } catch (PSQLException e) {
-      LOG.info(e.toString());
-      // In RR using the simple query protocol, we hit "Restart isn't possible because statement
-      // isn't one of SELECT/UPDATE/INSERT/DELETE" (due to the "command" being detected as "BEGIN"),
-      // and expect a PSQLException.
-      // In RR and RC using the extended query protocol, it executes as above, a batch statement
-      // after the first statement which can't be retried at the query layer.
+      LOG.info("Parameters: extendedProtocol: " + extendedProtocol +
+          ", explicitTransaction: " + explicitTransaction +
+          ", isolationLevel: " + isolationLevel + " error: " + e);
+
+      // In RR and RC:
+      // (1) If using the extended query protocol, it executes the multi-statement query as a batch
+      // statement for which non-first statements which can't be retried at the query layer.
+      // (2) If using the simple query protocol, retries are blocked due to the error mentioned
+      // below.
       assertNotEquals(isolationLevel, SR);
-      assertTrue(isolationLevel == RR || (isolationLevel == RC && extendedProtocol == true));
-      if (isolationLevel == RR) {
-        assertFalse(explicitTransaction == false && extendedProtocol == false);
+      if (extendedProtocol) {
+        assertTrue(e.toString().contains(
+            "could not serialize access due to concurrent update (query layer retries aren't " +
+            "supported when executing non-first statement in batch, will be unable to replay " +
+            "earlier commands)"));
+      } else {
+        assertTrue(e.toString().contains(
+            "could not serialize access due to concurrent update (query layer retries aren't " +
+            "supported for multi-statement queries issued via the simple query protocol, upvote " +
+            "github issue #21833 if you want this)"));
       }
       return;
     }
     // In SR, the query pauses on the docdb side until the conflicting transaction commits. Then the
     // transaction continues and succeeds.
-    // In RC, in the simple protocol only, in case of a transaction conflict, the entire query
-    // string is restarted, giving us correct results.
-    // In RR, in the simple protocol and where there is no explicit transaction, it's able to retry.
-    if (isolationLevel == RC) {
-      assertFalse(extendedProtocol);
-    }
-    if (isolationLevel == RR) {
-      assertFalse(extendedProtocol);
-      assertFalse(explicitTransaction);
-    }
+    assertEquals(isolationLevel, SR);
   }
 
   @Test
@@ -357,11 +361,8 @@ public void testMultipleStatementsPerQuery() throws Throwable {
     testMultipleStatementsPerQueryHelper(true, true, RR);
     testMultipleStatementsPerQueryHelper(true, true, SR);
 
-    // TODO(Piyush): This test crashes in the debug build due to transaction name being null in the
-    // following line of postgres.c:
-    //  Assert(strcmp(GetCurrentTransactionName(), YB_READ_COMMITTED_INTERNAL_SUB_TXN_NAME) == 0);
-    // testMultipleStatementsPerQueryHelper(false, false, RC);
 
+    testMultipleStatementsPerQueryHelper(false, false, RC);
     testMultipleStatementsPerQueryHelper(false, false, RR);
     testMultipleStatementsPerQueryHelper(false, false, SR);
 

java/yb-pgsql/src/test/java/org/yb/pgsql/TestPgTransparentRestarts.java

@@ -211,20 +211,6 @@ public void selectStarShort() throws Exception {
     ).runTest();
   }
 
-  /**
-   * Doing SELECT * operation on short strings with savepoints enabled and created and expect
-   * restarts to to *not* happen transparently.
-   * <p>
-   * todo
-   */
-  @Test
-  public void selectStarShortWithSavepoints() throws Exception {
-    new SavepointStatementTester(
-        getConnectionBuilder(),
-        getShortString()
-    ).runTest();
-  }
-
   /**
    * Same as the previous test but uses PreparedStatements (with no parameters)
    */
@@ -1206,75 +1192,6 @@ public ResultSet executeQuery(PreparedStatement stmt) throws Exception {
     }
   }
 
-  /** SavepointStatementTester that uses regular Statement and savepoints in various places */
-  private class SavepointStatementTester extends ConcurrentInsertQueryTester<Statement> {
-    public SavepointStatementTester(
-        ConnectionBuilder cb,
-        String valueToInsert) {
-      super(cb, valueToInsert, 50 /* numInserts */);
-    }
-
-    private ThrowingRunnable getRunnableThread(
-        ConnectionBuilder cb, BooleanSupplier isExecutionDone, String secondSavepointOpString) {
-      return () -> {
-        int selectsAttempted = 0;
-        int selectsRestartRequired = 0;
-        int selectsSucceeded = 0;
-        try (Connection selectTxnConn =
-                 cb.withIsolationLevel(IsolationLevel.REPEATABLE_READ).connect();
-             Statement stmt = selectTxnConn.createStatement()) {
-          selectTxnConn.setAutoCommit(false);
-          for (/* No setup */; !isExecutionDone.getAsBoolean(); ++selectsAttempted) {
-            try {
-              stmt.execute("SAVEPOINT a");
-              if (!secondSavepointOpString.isEmpty()) {
-                stmt.execute(secondSavepointOpString);
-              }
-              stmt.executeQuery("SELECT count(*) from test_rr");
-              selectTxnConn.commit();
-              ++selectsSucceeded;
-            } catch (Exception ex) {
-              try {
-                selectTxnConn.rollback();
-              } catch (SQLException ex1) {
-                fail("Rollback failed: " + ex1.getMessage());
-              }
-              if (isRestartReadError(ex)) {
-                ++selectsRestartRequired;
-              }
-              if (!isTxnError(ex)) {
-                throw ex;
-              }
-            }
-          }
-        } catch (Exception ex) {
-          fail("SELECT in savepoint thread failed: " + ex.getMessage());
-        }
-        LOG.info(String.format(
-            "SELECT in savepoint thread with second savepoint op: \"%s\": selectsSucceeded=%d" +
-            " selectsAttempted=%d selectsRestartRequired=%d", secondSavepointOpString,
-            selectsSucceeded, selectsAttempted, selectsRestartRequired));
-        assertTrue(
-            "No SELECTs after second savepoint statement: " + secondSavepointOpString
-                + " resulted in 'restart read required' on second operation"
-                + " - but we expected them to!"
-                + " " + selectsAttempted + " attempted, " + selectsSucceeded + " succeeded",
-                selectsRestartRequired > 0);
-      };
-    }
-
-    @Override
-    public List<ThrowingRunnable> getRunnableThreads(
-        ConnectionBuilder cb, BooleanSupplier isExecutionDone) {
-      List<ThrowingRunnable> runnables = new ArrayList<>();
-      runnables.add(getRunnableThread(cb, isExecutionDone, ""));
-      runnables.add(getRunnableThread(cb, isExecutionDone, "SAVEPOINT b"));
-      runnables.add(getRunnableThread(cb, isExecutionDone, "ROLLBACK TO a"));
-      runnables.add(getRunnableThread(cb, isExecutionDone, "RELEASE a"));
-      return runnables;
-    }
-  }
-
   /* DmlTester tests read restarts for DML statements. */
   private abstract class DmlTester <Stmt extends AutoCloseable> extends
       ConcurrentInsertQueryTester<Stmt> {

src/postgres/src/backend/access/transam/xact.c

@@ -3646,10 +3646,6 @@ IsInTransactionBlock(bool isTopLevel)
 	if (!isTopLevel)
 		return true;
 
-	if (CurrentTransactionState->blockState != TBLOCK_DEFAULT &&
-		CurrentTransactionState->blockState != TBLOCK_STARTED)
-		return true;
-
 	return false;
 }