Merge pull request #15882 from andreimatei/txn-update-debug

kv: fix racy access to a Transaction proto

pkg/kv/dist_sender.go

@@ -530,12 +530,7 @@ func (ds *DistSender) initAndVerifyBatch(
 				//
 				// We already have a clone of our txn (see above), so we can
 				// modify it freely.
-				//
-				// Zero the existing data. That makes sure that if we had
-				// something of size zero but with capacity, we don't re-use the
-				// existing space (which others may also use). This is just to
-				// satisfy paranoia/OCD and not expected to matter in practice.
-				ba.Txn.ResetObservedTimestamps()
+
 				// OrigTimestamp is the HLC timestamp at which the Txn started, so
 				// this effectively means no more uncertainty on this node.
 				ba.Txn.UpdateObservedTimestamp(nDesc.NodeID, ba.Txn.OrigTimestamp)

pkg/kv/txn_coord_sender.go

@@ -548,9 +548,8 @@ func (tc *TxnCoordSender) maybeRejectClientLocked(
 		// educated guess based on the incoming transaction timestamp.
 		return roachpb.NewError(errNoState)
 	case txnMeta.txn.Status == roachpb.ABORTED:
-		txn := txnMeta.txn.Clone()
-		tc.cleanupTxnLocked(ctx, txn)
-		abortedErr := roachpb.NewErrorWithTxn(roachpb.NewTransactionAbortedError(), &txn)
+		tc.cleanupTxnLocked(ctx, txnMeta.txn)
+		abortedErr := roachpb.NewErrorWithTxn(roachpb.NewTransactionAbortedError(), &txnMeta.txn)
 		// TODO(andrei): figure out a UserPriority to use here.
 		newTxn := roachpb.PrepareTransactionForRetry(
 			ctx, abortedErr,
@@ -559,10 +558,9 @@ func (tc *TxnCoordSender) maybeRejectClientLocked(
 		return roachpb.NewError(roachpb.NewHandledRetryableTxnError(
 			abortedErr.Message, txn.ID, newTxn))
 	case txnMeta.txn.Status == roachpb.COMMITTED:
-		txn := txnMeta.txn.Clone()
-		tc.cleanupTxnLocked(ctx, txn)
+		tc.cleanupTxnLocked(ctx, txnMeta.txn)
 		return roachpb.NewErrorWithTxn(roachpb.NewTransactionStatusError(
-			"transaction is already committed"), &txn)
+			"transaction is already committed"), &txnMeta.txn)
 	default:
 		return nil
 	}
@@ -613,8 +611,10 @@ func (tc *TxnCoordSender) cleanupTxnLocked(ctx context.Context, txn roachpb.Tran
 	}
 
 	// The supplied txn may be newer than the one in txnMeta, which is relevant
-	// for stats.
-	txnMeta.txn = txn
+	// for stats. We clone the txn before storing it, as the caller might not
+	// have provided a deep-copy, and we don't want to share Transactions in the
+	// TxnCoordSender's map with anyone.
+	txnMeta.txn = txn.Clone()
 	// Trigger heartbeat shutdown.
 	close(txnMeta.txnEnd)
 	txnMeta.txnEnd = nil

pkg/roachpb/data.go

@@ -715,7 +715,7 @@ func (t Transaction) LastActive() hlc.Timestamp {
 // keys with the intent spans.
 func (t Transaction) Clone() Transaction {
 	mt := t.ObservedTimestamps
-	if len(mt) != 0 {
+	if mt != nil {
 		t.ObservedTimestamps = make([]ObservedTimestamp, len(mt))
 		copy(t.ObservedTimestamps, mt)
 	}

pkg/roachpb/errors.go

@@ -75,6 +75,8 @@ func NewError(err error) *Error {
 }
 
 // NewErrorWithTxn creates an Error from the given error and a transaction.
+//
+// txn is cloned before being stored in Error.
 func NewErrorWithTxn(err error, txn *Transaction) *Error {
 	e := NewError(err)
 	e.SetTxn(txn)