storage: tolerate missing transaction records when pushing

docs/RFCS/20181209_lazy_txn_record_creation.md

@@ -1,5 +1,5 @@
 - Feature Name: Lazy Transaction Record Creation (a.k.a Deprecate BeginTransaction)
-- Status: draft
+- Status: in-progress
 - Start Date: 2018-12-09
 - Authors: Nathan VanBenschoten
 - RFC PR: #32971

pkg/roachpb/api.go

@@ -1028,8 +1028,13 @@ func (*AdminChangeReplicasRequest) flags() int { return isAdmin | isAlone }
 func (*AdminRelocateRangeRequest) flags() int  { return isAdmin | isAlone }
 func (*HeartbeatTxnRequest) flags() int        { return isWrite | isTxn }
 func (*GCRequest) flags() int                  { return isWrite | isRange }
-func (*PushTxnRequest) flags() int             { return isWrite | isAlone }
-func (*QueryTxnRequest) flags() int            { return isRead | isAlone }
+
+// PushTxnRequest updates the read timestamp cache when pushing a transaction's
+// timestamp and updates the write timestamp cache when aborting a transaction.
+func (*PushTxnRequest) flags() int {
+	return isWrite | isAlone | updatesReadTSCache | updatesWriteTSCache
+}
+func (*QueryTxnRequest) flags() int { return isRead | isAlone }
 
 // QueryIntent only updates the read timestamp cache when attempting
 // to prevent an intent that is found missing from ever being written

pkg/roachpb/api.proto

@@ -742,8 +742,8 @@ message PushTxnRequest {
   // this to PUSH_TOUCH to determine whether the pushee can be aborted
   // due to inactivity (based on the now field).
   PushTxnType push_type = 6;
-  // Forces the push by overriding the normal checks in PushTxn to
-  // either abort or push the timestamp.
+  // Forces the push by overriding the normal expiration and priority checks
+  // in PushTxn to either abort or push the timestamp.
   bool force = 7;
 
   reserved 8;

pkg/roachpb/errors.proto

@@ -123,12 +123,12 @@ enum TransactionAbortedReason {
   // For backwards compatibility.
   ABORT_REASON_UNKNOWN = 0;
 
-  // A BeginTransaction or EndTransaction(commit=true) request found an aborted
-  // transaction record. Another txn must have written this record - that other
-  // txn probably ran into one of our intents written before BeginTransaction
-  // (on a different range) and pushed it successfully. Or, a high-priority
-  // transaction simply pushed us, or we failed to heartbeat for a while and
-  // another txn (of any priority) considered us abandoned and pushed us.
+  // A BeginTransaction, HeartbeatTxn, or EndTransaction(commit=true) request
+  // found an aborted transaction record. Another txn must have written this
+  // record - that other txn probably ran into one of our intents and pushed our
+  // transaction record successfully. Either a high-priority transaction simply
+  // pushed us or we failed to heartbeat for a while and another txn (of any
+  // priority) considered us abandoned and pushed us.
   ABORT_REASON_ABORTED_RECORD_FOUND = 1;
 
   // The request attempting to create a transaction record has a timestamp below
@@ -152,11 +152,11 @@ enum TransactionAbortedReason {
   // meantime because the transaction was aborted.
   ABORT_REASON_ABORT_SPAN = 5;
 
-  // An EndTransaction encountered a timestamp cache entry for the txn key, and
-  // the entry identifies this transaction. This means that the transaction
-  // definitely committed or rolled back before.
-  // So, this EndTransaction is either a delayed replay of some sort, or it
-  // raced with an async abort and lost. If a client gets this
+  // A request attempting to create a transaction record encountered a write
+  // timestamp cache entry for the txn key, and the entry identifies this
+  // transaction. This means that the transaction definitely committed or rolled
+  // back before. So, this request is either a delayed replay of some sort, or
+  // it raced with an async abort and lost. If a client gets this
   // TransactionAbortedError (without it being wrapped in an ambiguous error),
   // it must be the latter case, and the transaction can be retried.
   ABORT_REASON_ALREADY_COMMITTED_OR_ROLLED_BACK_POSSIBLE_REPLAY = 6;

pkg/storage/batcheval/cmd_begin_transaction.go

@@ -118,8 +118,8 @@ func BeginTransaction(
 	}
 
 	// Verify that it is safe to create the transaction record.
-	if ok, reason := cArgs.EvalCtx.CanCreateTxnRecord(reply.Txn); !ok {
-		return result.Result{}, roachpb.NewTransactionAbortedError(reason)
+	if err := CanCreateTxnRecord(cArgs.EvalCtx, reply.Txn); err != nil {
+		return result.Result{}, err
 	}
 
 	// Write the txn record.

pkg/storage/batcheval/cmd_end_transaction.go

@@ -186,8 +186,8 @@ func evalEndTransaction(
 		// to perform this verification for commits. Rollbacks can always write
 		// an aborted txn record.
 		if args.Commit {
-			if ok, reason := cArgs.EvalCtx.CanCreateTxnRecord(reply.Txn); !ok {
-				return result.Result{}, roachpb.NewTransactionAbortedError(reason)
+			if err := CanCreateTxnRecord(cArgs.EvalCtx, reply.Txn); err != nil {
+				return result.Result{}, err
 			}
 		}
 	} else {
@@ -350,12 +350,12 @@ func evalEndTransaction(
 	// subsequent replay of this EndTransaction call because the txn timestamp
 	// will be too old. Replays of requests which attempt to create a new txn
 	// record (BeginTransaction, HeartbeatTxn, or EndTransaction) never succeed
-	// because EndTransaction inserts in the write timestamp cache, forcing the
-	// call to CanCreateTxnRecord to return false, resulting in a transaction
-	// retry error. If the replay didn't attempt to create a txn record, any
-	// push will immediately succeed as a missing txn record on push where
-	// CanCreateTxnRecord returns false succeeds. In both cases, the txn will
-	// be GC'd on the slow path.
+	// because EndTransaction inserts in the write timestamp cache in Replica's
+	// updateTimestampCache method, forcing the call to CanCreateTxnRecord to
+	// return false, resulting in a transaction retry error. If the replay
+	// didn't attempt to create a txn record, any push will immediately succeed
+	// as a missing txn record on push where CanCreateTxnRecord returns false
+	// succeeds. In both cases, the txn will be GC'd on the slow path.
 	//
 	// We specify alwaysReturn==false because if the commit fails below Raft, we
 	// don't want the intents to be up for resolution. That should happen only

pkg/storage/batcheval/cmd_heartbeat_txn.go

@@ -72,8 +72,8 @@ func HeartbeatTxn(
 		}
 
 		// Verify that it is safe to create the transaction record.
-		if ok, reason := cArgs.EvalCtx.CanCreateTxnRecord(&txn); !ok {
-			return result.Result{}, roachpb.NewTransactionAbortedError(reason)
+		if err := CanCreateTxnRecord(cArgs.EvalCtx, &txn); err != nil {
+			return result.Result{}, err
 		}
 	}