Merge pull request #9524 from tschottdorf/txn-gc

stability: prevent GC from preventing all new transactions

storage/replica_command.go

@@ -434,25 +434,25 @@ func (r *Replica) BeginTransaction(
 	reply.Txn = &clonedTxn
 
 	// Verify transaction does not already exist.
-	txn := roachpb.Transaction{}
-	ok, err := engine.MVCCGetProto(ctx, batch, key, hlc.ZeroTimestamp, true, nil, &txn)
+	tmpTxn := roachpb.Transaction{}
+	ok, err := engine.MVCCGetProto(ctx, batch, key, hlc.ZeroTimestamp, true, nil, &tmpTxn)
 	if err != nil {
 		return reply, err
 	}
 	if ok {
-		switch txn.Status {
+		switch tmpTxn.Status {
 		case roachpb.ABORTED:
 			// Check whether someone has come in ahead and already aborted the
 			// txn.
 			return reply, roachpb.NewTransactionAbortedError()
 
 		case roachpb.PENDING:
-			if h.Txn.Epoch > txn.Epoch {
+			if h.Txn.Epoch > tmpTxn.Epoch {
 				// On a transaction retry there will be an extant txn record
 				// but this run should have an upgraded epoch. The extant txn
 				// record may have been pushed or otherwise updated, so update
 				// this command's txn and rewrite the record.
-				reply.Txn.Update(&txn)
+				reply.Txn.Update(&tmpTxn)
 			} else {
 				// Our txn record already exists. This is either a client error, sending
 				// a duplicate BeginTransaction, or it's an artefact of DistSender
@@ -462,12 +462,12 @@ func (r *Replica) BeginTransaction(
 
 		case roachpb.COMMITTED:
 			return reply, roachpb.NewTransactionStatusError(
-				fmt.Sprintf("BeginTransaction can't overwrite %s", txn),
+				fmt.Sprintf("BeginTransaction can't overwrite %s", tmpTxn),
 			)
 
 		default:
 			return reply, roachpb.NewTransactionStatusError(
-				fmt.Sprintf("bad txn state: %s", txn),
+				fmt.Sprintf("bad txn state: %s", tmpTxn),
 			)
 		}
 	}
@@ -482,7 +482,7 @@ func (r *Replica) BeginTransaction(
 	// (which may have been written before this entry).
 	//
 	// See #9265.
-	if txn.LastActive().Less(threshold) {
+	if reply.Txn.LastActive().Less(threshold) {
 		return reply, roachpb.NewTransactionAbortedError()
 	}
 

storage/replica_test.go

@@ -2511,6 +2511,17 @@ func TestEndTransactionTxnSpanGCThreshold(t *testing.T) {
 			t.Fatalf("expected txn aborted error, got %v and response %+v", pErr, resp)
 		}
 	}
+
+	// A transaction which starts later (i.e. at a higher timestamp) should not
+	// be prevented from writing its record.
+	// See #9522.
+	{
+		txn := newTransaction("foo", key, 1, enginepb.SERIALIZABLE, tc.clock)
+		beginArgs, header := beginTxnArgs(key, txn)
+		if _, pErr := tc.SendWrappedWith(header, &beginArgs); pErr != nil {
+			t.Fatal(pErr)
+		}
+	}
 }
 
 // TestEndTransactionDeadline_1PC verifies that a transaction that