[#18286][#14114][#18435] YSQL: Fix regression caused by error code se…

…t in reported deadlocks

Summary:
Commit d6d0cf6 caused a regression as we were setting `YB_PG_T_R_DEADLOCK_DETECTED` error code for deadlocks.

For READ COMMITTED isolation, Pg retries the whole statement with a new read point when it faces a kConflict or kReadRestart error. In case of SERIALIZABLE and REPEATABLE READ isolation levels, Pg retries the whole transaction with a new read point on kConflict or kReadRestart errors, only when  no data has been sent yet to the external client as part of the transaction (i.e., no prior statement has been executed and ysql_output_buffer_size hasn't yet been used by the first statement).

The above commit changed the error code, which led to increased failures in a conflicting workload as transactions weren't being retired. This diff addresses the issue by modifying the retry logic to also check for `kDeadlock` and act accordingly.

- For READ COMMITTED, we have the ability to retry a statement in case of `kConflict` or `kReadRestart`, and we do so indefinitely to honor read committed semantics unless a filled buffer of response data has already been sent to the client for the statement (see `YBIsDataSentForCurrQuery()`). But for deadlock errors, there is no point in retrying the statement because the deadlock cycle might involve granted locks from previous statement(s). But we can still retry if it is the first statement in the transaction and no response data has been sent back to the client (retrying in this case will result in releasing the granted locks as well and hence allows progress). We check for this condition using `YBIsDataSent()`.
- For other isolation levels (SERIALIZABLE & REPEATABLE READ), we retry the transaction for at most `ysql_max_write_restart_attempts` only when no data has been sent so far (i.e., `YBIsDataSent()` is false).

Here's the log when a transaction encounters a deadlock (with the changes)
```
ERROR:  deadlock detected
DETAIL:  Transaction ebf3d40f-0d39-4f26-b969-4d515043b3c5 aborted due to a deadlock.
ebf3d40f-0d39-4f26-b969-4d515043b3c5 -> d8beba29-3ca5-4292-bd2b-24d878d381d4 -> ebf3d40f-0d39-4f26-b969-4d515043b3c5 : kDeadlock
```

Note: the test change is required as we previously used to set 40001, but now since we also return 40P01, we need to check for both. Without this change, the test fails, which further asserts that we return 40P01 now.
Jira: DB-7281, DB-3597, DB-7420

Test Plan:
./yb_build.sh asan --java-test 'org.yb.pgsql.TestPgTransactions#testSerializableReadDelayWrite'
./yb_build.sh asan --cxx-test pgwrapper_pg_libpq-test --gtest_filter PgLibPqTest.TestLockedConcurrentCounterSerializable
./yb_build.sh --cxx-test pgwrapper_pg_libpq-test --gtest_filter PgLibPqTest.TestConcurrentCounterReadCommitted
./yb_build.sh --cxx-test  pgwrapper_pg_wait_on_conflict-test --gtest_filter PgWaitQueuesReadCommittedTest.TestDeadlockSimple

Reviewers: rsami, pjain, smishra, mihnea, tvesely

Reviewed By: pjain

Subscribers: myang, yql, ybase

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

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

@@ -53,7 +53,12 @@ public class TestPgTransactions extends BasePgSQLTest {
   private static final Logger LOG = LoggerFactory.getLogger(TestPgTransactions.class);
 
   private static boolean isYBTransactionError(PSQLException ex) {
-    return ex.getSQLState().equals("40001");
+    // TODO: Refactor the function to check for specific error codes instead of checking multiple
+    // errors as few tests that shouldn't encounter a 40P01 would also get through on usage of a
+    // generic check. Refer https://github.com/yugabyte/yugabyte-db/issues/18477 for details.
+    //
+    // Return true on exceptions of kind SERIALIZATION_FAILURE or DEADLOCK_DETECTED.
+    return ex.getSQLState().equals("40001") || ex.getSQLState().equals("40P01");
   }
 
   private static boolean isTransactionAbortedError(PSQLException ex) {

src/postgres/src/backend/tcop/postgres.c

@@ -3837,14 +3837,14 @@ static void YBPrepareCacheRefreshIfNeeded(ErrorData *edata,
 	*/
 	bool is_read_restart_error = YBCIsRestartReadError(edata->yb_txn_errcode);
 	bool is_conflict_error     = YBCIsTxnConflictError(edata->yb_txn_errcode);
-
+	bool is_deadlock_error	   = YBCIsTxnDeadlockError(edata->yb_txn_errcode);
 	/*
 	 * Note that 'is_dml' could be set for a Select operation on a pg_catalog
 	 * table. Even if it fails due to conflict, a retry is expected to succeed
 	 * without refreshing the cache (as the schema of a PG catalog table cannot
 	 * change).
 	 */
-	if (is_dml && (is_read_restart_error || is_conflict_error))
+	if (is_dml && (is_read_restart_error || is_conflict_error || is_deadlock_error))
 	{
 		return;
 	}
@@ -4163,10 +4163,11 @@ yb_is_restart_possible(const ErrorData* edata,
 			 edata->message, edata->filename, edata->lineno);
 	bool is_read_restart_error = YBCIsRestartReadError(edata->yb_txn_errcode);
 	bool is_conflict_error     = YBCIsTxnConflictError(edata->yb_txn_errcode);
-	if (!is_read_restart_error && !is_conflict_error)
+	bool is_deadlock_error	   = YBCIsTxnDeadlockError(edata->yb_txn_errcode);
+	if (!is_read_restart_error && !is_conflict_error && !is_deadlock_error)
 	{
 		if (yb_debug_log_internal_restarts)
-			elog(LOG, "Restart isn't possible, code %d isn't a read restart/conflict error",
+			elog(LOG, "Restart isn't possible, code %d isn't a read restart/conflict/deadlock error",
 			          edata->yb_txn_errcode);
 		return false;
 	}
@@ -4199,6 +4200,41 @@ yb_is_restart_possible(const ErrorData* edata,
 		return false;
 	}
 
+	/*
+	 * For isolation levels other than READ COMMITTED, retries on deadlock are capped at
+	 * ysql_max_write_restart_attempts, given that no data has been sent as part of the transaction.
+	 */
+	if (!IsYBReadCommitted() && is_deadlock_error &&
+		attempt >= *YBCGetGFlags()->ysql_max_write_restart_attempts)
+	{
+		if (yb_debug_log_internal_restarts)
+			elog(LOG, "Restart isn't possible, we're out of read/write restart attempts (%d) on deadlock",
+			          attempt);
+		*retries_exhausted = true;
+		return false;
+	}
+
+	/*
+	 * In case of READ COMMITTED, retries involve restarting the current statement and not the whole
+	 * transaction.
+	 *
+	 * We can't differentiate if locks acquired by this transaction that are part of the deadlock
+	 * cycle were acquired in a previous statement or the current statement. If acquired in a previous
+	 * statement, restarting the current statement is of no use. If acquired in the current statement,
+	 * a restart could help in resolving the deadlock since the acquired locks would be released in an
+	 * attempt to reacquire.
+	 *
+	 * Hence retries on deadlock in READ COMMITTED isolation are performed indefinitely until statement
+	 * timeout only when no data has been sent as part of the transaction (which also subsumes ensuring
+	 * that this was the first statement in the transaction).
+	 */
+	if (IsYBReadCommitted() && is_deadlock_error && YBIsDataSent()) {
+		if (yb_debug_log_internal_restarts)
+			elog(LOG, "Restart isn't possible, encountered deadlock in READ COMMITTED isolation");
+		*retries_exhausted = true;
+		return false;
+	}
+
 	// We can perform kReadRestart retries in READ COMMITTED isolation level even if data has been
 	// sent as part of the txn, but not as part of the current query. This is because we just have to
 	// retry the query and not the whole transaction.
@@ -4586,7 +4622,8 @@ yb_attempt_to_restart_on_error(int attempt,
 			{
 				HandleYBStatus(YBCPgRestartReadPoint());
 			}
-			else if (YBCIsTxnConflictError(edata->yb_txn_errcode))
+			else if (YBCIsTxnConflictError(edata->yb_txn_errcode) ||
+					 YBCIsTxnDeadlockError(edata->yb_txn_errcode))
 			{
 				HandleYBStatus(YBCPgResetTransactionReadPoint());
 				yb_maybe_sleep_on_txn_conflict(attempt);
@@ -4620,7 +4657,8 @@ yb_attempt_to_restart_on_error(int attempt,
 			{
 				YBCRestartTransaction();
 			}
-			else if (YBCIsTxnConflictError(edata->yb_txn_errcode))
+			else if (YBCIsTxnConflictError(edata->yb_txn_errcode) ||
+					 YBCIsTxnDeadlockError(edata->yb_txn_errcode))
 			{
 				/*
 				 * Recreate the YB state for the transaction. This call preserves the

src/postgres/src/backend/utils/misc/pg_yb_utils.c

@@ -558,6 +558,17 @@ GetStatusMsgAndArgumentsByCode(const uint32_t pg_err_code,
 			*msg_args = (const char **) palloc(sizeof(const char *));
 			(*msg_args)[0] = FetchUniqueConstraintName(YBCStatusRelationOid(s));
 			break;
+		case ERRCODE_T_R_DEADLOCK_DETECTED:
+			if (YBCIsTxnDeadlockError(txn_err_code)) {
+				*msg_buf = "deadlock detected";
+				*msg_nargs = 0;
+				*msg_args = NULL;
+
+				*detail_buf = status_msg;
+				*detail_nargs = status_nargs;
+				*detail_args = status_args;
+			}
+			break;
 		default:
 			break;
 	}

src/yb/common/transaction_error.h

@@ -26,7 +26,8 @@ YB_DEFINE_ENUM(TransactionErrorCode,
     (kReadRestartRequired)
     (kConflict)
     (kSnapshotTooOld)
-    (kSkipLocking));
+    (kSkipLocking)
+    (kDeadlock));
 
 struct TransactionErrorTag : IntegralErrorTag<TransactionErrorCode> {
   // It is part of the wire protocol and should not be changed once released.

src/yb/docdb/deadlock_detector.cc

@@ -21,6 +21,7 @@
 
 #include "yb/common/pgsql_error.h"
 #include "yb/common/transaction.h"
+#include "yb/common/transaction_error.h"
 #include "yb/common/wire_protocol.h"
 
 #include "yb/gutil/stl_util.h"
@@ -380,7 +381,7 @@ std::string ConstructDeadlockedMessage(const TransactionId& waiter,
       ss << Format(" -> $0", *id_or_status);
     }
   }
-  ss << Format(" -> $0\n", waiter.ToString());
+  ss << Format(" -> $0 ", waiter.ToString());
   return ss.str();
 }
 
@@ -627,8 +628,10 @@ class DeadlockDetector::Impl : public std::enable_shared_from_this<DeadlockDetec
     if (it == recently_deadlocked_txns_info_.end()) {
       return Status::OK();
     }
+    // Deadlock status is currently used in wait-queues alone. It is required that we set status
+    // type as InternalError for pg to decode the error and throw ERRCODE_T_R_DEADLOCK_DETECTED.
     return STATUS_EC_FORMAT(
-        Expired, PgsqlError(YBPgErrorCode::YB_PG_T_R_DEADLOCK_DETECTED), it->second.first);
+        InternalError, TransactionError(TransactionErrorCode::kDeadlock), it->second.first);
   }
 
  private:

src/yb/tablet/running_transaction.cc

@@ -289,15 +289,18 @@ bool RunningTransaction::UpdateStatus(
   }
   last_known_status_hybrid_time_ = time_of_status;
 
+  // Reset last_known_deadlock_status_ with the latest status. deadlock status is currently
+  // used in wait-queues alone and shouldn't cause any correctness issues.
+  DCHECK(expected_deadlock_status.ok() || transaction_status == TransactionStatus::ABORTED);
+  last_known_deadlock_status_ = expected_deadlock_status;
+
   if (transaction_status == last_known_status_) {
     return false;
   }
   if (last_known_status_ == TransactionStatus::ABORTED) {
     context_.NotifyAbortedTransactionDecrement(id());
   }
   last_known_status_ = transaction_status;
-  DCHECK(expected_deadlock_status.ok() || transaction_status == TransactionStatus::ABORTED);
-  last_known_deadlock_status_ = expected_deadlock_status;
 
   return transaction_status == TransactionStatus::ABORTED;
 }

src/yb/yql/pggate/util/ybc_util.cc

@@ -172,6 +172,9 @@ YBPgErrorCode FetchErrorCode(YBCStatus s) {
         case TransactionErrorCode::kSnapshotTooOld:
           result = YBPgErrorCode::YB_PG_SNAPSHOT_TOO_OLD;
           break;
+        case TransactionErrorCode::kDeadlock:
+          result = YBPgErrorCode::YB_PG_T_R_DEADLOCK_DETECTED;
+          break;
         case TransactionErrorCode::kNone: FALLTHROUGH_INTENDED;
         default:
           break;
@@ -276,6 +279,10 @@ bool YBCIsTxnSkipLockingError(uint16_t txn_errcode) {
   return txn_errcode == to_underlying(TransactionErrorCode::kSkipLocking);
 }
 
+bool YBCIsTxnDeadlockError(uint16_t txn_errcode) {
+  return txn_errcode == to_underlying(TransactionErrorCode::kDeadlock);
+}
+
 uint16_t YBCGetTxnConflictErrorCode() {
   return to_underlying(TransactionErrorCode::kConflict);
 }

src/yb/yql/pggate/util/ybc_util.h

@@ -148,6 +148,7 @@ bool YBCIsRestartReadError(uint16_t txn_errcode);
 
 bool YBCIsTxnConflictError(uint16_t txn_errcode);
 bool YBCIsTxnSkipLockingError(uint16_t txn_errcode);
+bool YBCIsTxnDeadlockError(uint16_t txn_errcode);
 uint16_t YBCGetTxnConflictErrorCode();
 
 void YBCResolveHostname();

src/yb/yql/pgwrapper/libpq_utils.cc

@@ -680,13 +680,16 @@ std::string PqEscapeIdentifier(const std::string& input) {
 }
 
 bool HasTransactionError(const Status& status) {
+  // TODO: Refactor the function to check for specific error codes instead of checking multiple
+  // errors as few tests that shouldn't encounter a 40P01 would also get through on usage of a
+  // generic check. Refer https://github.com/yugabyte/yugabyte-db/issues/18478 for details.
   static const auto kExpectedErrors = {
-      "could not serialize access due to concurrent update",
-      "Transaction aborted:",
-      "expired or aborted by a conflict:",
-      "Unknown transaction, could be recently aborted:"
+      // ERRCODE_T_R_SERIALIZATION_FAILURE
+      "pgsql error 40001",
+      // ERRCODE_T_R_DEADLOCK_DETECTED
+      "pgsql error 40P01"
   };
-  return HasSubstring(status.message(), kExpectedErrors);
+  return HasSubstring(status.ToString(), kExpectedErrors);
 }
 
 bool IsRetryable(const Status& status) {

src/yb/yql/pgwrapper/pg_libpq-test.cc

@@ -940,6 +940,10 @@ TEST_F(PgLibPqTest, TestConcurrentCounterRepeatableRead) {
   TestConcurrentCounter(IsolationLevel::SNAPSHOT_ISOLATION);
 }
 
+TEST_F(PgLibPqTest, TestConcurrentCounterReadCommitted) {
+  TestConcurrentCounter(IsolationLevel::READ_COMMITTED);
+}
+
 TEST_F(PgLibPqTest, SecondaryIndexInsertSelect) {
   constexpr int kThreads = 4;
 

src/yb/yql/pgwrapper/pg_wait_on_conflict-test.cc

@@ -94,6 +94,10 @@ class PgWaitQueuesTest : public PgMiniTestBase {
   }
 
   void TestDeadlockWithWrites() const;
+
+  virtual IsolationLevel GetIsolationLevel() const {
+    return SNAPSHOT_ISOLATION;
+  }
 };
 
 auto GetBlockerIdx(auto idx, auto cycle_length) {
@@ -188,7 +192,7 @@ void PgWaitQueuesTest::TestDeadlockWithWrites() const {
     thread_holder.AddThreadFunctor(
         [this, i, &first_update, &done, &succeeded_second_update, &succeeded_commit] {
       auto conn = ASSERT_RESULT(Connect());
-      ASSERT_OK(conn.StartTransaction(IsolationLevel::SNAPSHOT_ISOLATION));
+      ASSERT_OK(conn.StartTransaction(GetIsolationLevel()));
 
       ASSERT_OK(conn.ExecuteFormat("UPDATE foo SET v=$0 WHERE k=$0", i));
       first_update.CountDown();
@@ -1029,6 +1033,17 @@ TEST_F(PgWaitQueuesTest, YB_DISABLE_TEST_IN_TSAN(ParallelUpdatesDetectDeadlock))
   }
 }
 
+class PgWaitQueuesReadCommittedTest : public PgWaitQueuesTest {
+ protected:
+  IsolationLevel GetIsolationLevel() const override {
+    return IsolationLevel::READ_COMMITTED;
+  }
+};
+
+TEST_F(PgWaitQueuesReadCommittedTest, TestDeadlockSimple) {
+  TestDeadlockWithWrites();
+}
+
 class PgWaitQueuePackedRowTest : public PgWaitQueuesTest {
   void SetUp() override {
     ANNOTATE_UNPROTECTED_WRITE(FLAGS_ysql_enable_packed_row) = true;