[#1974] Remove a superfluous check from WriteOperationCompletionCallback

Summary:
As shown by Jepsen log investigation, the initial version of D7063 ( ee7bc3e ) was enough to fix #1974.
The extra check that was added to WriteOperationCompletionCallback is superfluous and also contains a bug for the case where this operation is the first operation of the transaction on this tablet. In such a case, this check always decides that the transaction has been aborted,
while (obviously) some transactions don't get aborted. The reason the call to TransactionParticipant::CheckAborted returns an aborted
status for the first operation of a transaction on a particular tablet, in this case, is that the transaction metadata should be written by the operation
that ended up failing. Just to clarify, if an operation fails, we don't write metadata for this transaction on the transaction participant.
And the failure of the operation is what initiates this check that eventually calls CheckAborted.

Just as a reminder, absent metadata for a particular transaction id in TransactionParticipant is indistinguishable from the situation when this transaction that has been aborted and cleaned up.

This revision removes the extra check.

Test Plan: ybd --gtest_filter PgLibPqTest.OnConflict -n 100

Reviewers: alex, neha, mikhail

Reviewed By: mikhail

Subscribers: ybase, bogdan

Differential Revision: https://phabricator.dev.yugabyte.com/D7090

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

@@ -57,7 +57,8 @@ private String executeOneOrTwoAdminsAssertion(
   private static boolean isYBTxnException(PSQLException ex) {
     String msg = ex.getMessage();
     return msg.contains("Missing metadata for transaction:") ||
-           msg.contains("Conflicts with higher priority transaction:");
+           msg.contains("Conflicts with higher priority transaction:") ||
+           msg.contains("Transaction aborted:");
   }
 
   private int checkAssertion(String connDescription, Statement statement, boolean useCount)

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

@@ -56,6 +56,7 @@ private static boolean isYBTransactionError(PSQLException ex) {
     // TODO: test for error codes here when we move to more PostgreSQL-friendly transaction errors.
     return (
         msg.contains("Conflicts with higher priority transaction") ||
+        msg.contains("Transaction aborted") ||
         msg.contains("Transaction expired") ||
         msg.contains("Restart read required") ||
         msg.contains("Conflicts with committed transaction") ||

src/yb/client/transaction.cc

@@ -270,7 +270,7 @@ class YBTransaction::Impl final {
       }
       TabletStates::iterator it = tablets_.end();
       for (const auto& op : ops) {
-        if (op->yb_op->wrote_data()) {
+        if (op->yb_op->wrote_data(metadata_.isolation)) {
           const std::string& tablet_id = op->tablet->tablet_id();
           // Usually all ops belong to the same tablet. So we can avoid repeating lookup.
           if (it == tablets_.end() || it->first != tablet_id) {

src/yb/client/yb_op.h

@@ -100,7 +100,10 @@ class YBOperation {
   virtual bool succeeded() = 0;
   virtual bool returns_sidecar() = 0;
 
-  virtual bool wrote_data() { return succeeded() && !read_only(); }
+  virtual bool wrote_data(IsolationLevel isolation_level) {
+    return succeeded() &&
+           (!read_only() || isolation_level == IsolationLevel::SERIALIZABLE_ISOLATION);
+  }
 
   virtual void SetHashCode(uint16_t hash_code) = 0;
 
@@ -429,7 +432,9 @@ class YBPgsqlWriteOp : public YBPgsqlOp {
     is_single_row_txn_ = is_single_row_txn;
   }
 
-  bool wrote_data() override { return succeeded() && !read_only() && !response().skipped(); }
+  bool wrote_data(IsolationLevel isolation_level) override {
+    return YBOperation::wrote_data(isolation_level) && !response().skipped();
+  }
 
  protected:
   virtual Type type() const override {

src/yb/tablet/transaction_participant.cc

@@ -868,6 +868,8 @@ class TransactionParticipant::Impl : public RunningTransactionContext {
       auto it = transactions_.find(metadata.transaction_id);
       if (it != transactions_.end()) {
         RETURN_NOT_OK((**it).CheckAborted());
+      } else if (WasTransactionRecentlyRemoved(metadata.transaction_id)) {
+        return MakeAbortedStatus(metadata.transaction_id);
       }
       return metadata;
     }
@@ -1347,7 +1349,7 @@ class TransactionParticipant::Impl : public RunningTransactionContext {
     auto now = CoarseMonoClock::now();
     CleanupRecentlyRemovedTransactions(now);
     auto& transaction = **it;
-    recently_removed_transactions_cleanup_queue_.push_back({transaction.id(), now + 5s});
+    recently_removed_transactions_cleanup_queue_.push_back({transaction.id(), now + 15s});
     LOG_IF_WITH_PREFIX(DFATAL, !recently_removed_transactions_.insert(transaction.id()).second)
         << "Transaction removed twice: " << transaction.id();
     transactions_.erase(it);

src/yb/tserver/tablet_service.cc

@@ -270,28 +270,6 @@ class WriteOperationCompletionCallback : public OperationCompletionCallback {
       status_ = Status::OK();
     }
 
-    if (status_.ok() && state_->request()->write_batch().has_transaction()) {
-      bool has_failure = false;
-      for (const auto& pgsql_write_op : *state_->pgsql_write_ops()) {
-        if (pgsql_write_op->response()->status() != PgsqlResponsePB::PGSQL_STATUS_OK) {
-          has_failure = true;
-          break;
-        }
-      }
-      if (has_failure) {
-        auto participant = tablet_peer_->tablet()->transaction_participant();
-        if (participant) {
-          auto txn_id = FullyDecodeTransactionId(
-              state_->request()->write_batch().transaction().transaction_id());
-          if (txn_id.ok()) {
-            status_ = participant->CheckAborted(*txn_id);
-          } else {
-            LOG(DFATAL) << "Unable to decode transaction id: " << txn_id.status();
-          }
-        }
-      }
-    }
-
     if (!status_.ok()) {
       LOG(INFO) << "Write failed: " << status_;
       SetupErrorAndRespond(get_error(), status_, code_, context_.get());