roachpb/storage: complete removal of BeginTransactionRequest

This commit completes the migration started in cockroachdb#32971 (a year ago to the week).
It removes all references to `BeginTransactionRequest` and deletes the request
type entirely. This reduces the number of request types that can create transaction
records down to just two: `EndTransactionRequest` and `HeartbeatTxnRequest` (see
the state transition diagram in `replica_tscache.go`).

It is safe to remove server-side handling of `BeginTransactionRequest` because we
never use it in v19.2 binaries, so v20.1 binaries will never run in a cluster that
is using the request type.

Once this commit is merged, I'm going to `s/EndTransaction/EndTxn/g` to bring
that request name in line with all other transaction metadata requests (e.g.
`HeartbeatTxnRequest`, `PushTxnRequest`, `QueryTxnRequest`, and `RecoverTxnRequest`).

Release note: None

pkg/internal/client/batch.go

@@ -242,7 +242,6 @@ func (b *Batch) fillResults(ctx context.Context) {
 
 			// Nothing to do for all methods below as they do not generate
 			// any rows.
-			case *roachpb.BeginTransactionRequest:
 			case *roachpb.EndTransactionRequest:
 			case *roachpb.AdminMergeRequest:
 			case *roachpb.AdminSplitRequest:

pkg/kv/dist_sender.go

@@ -577,7 +577,7 @@ func (ds *DistSender) initAndVerifyBatch(
 			case *roachpb.QueryIntentRequest, *roachpb.ResolveIntentRangeRequest:
 				continue
 
-			case *roachpb.BeginTransactionRequest, *roachpb.EndTransactionRequest, *roachpb.ReverseScanRequest:
+			case *roachpb.EndTransactionRequest, *roachpb.ReverseScanRequest:
 				continue
 
 			case *roachpb.RevertRangeRequest:
@@ -594,7 +594,7 @@ func (ds *DistSender) initAndVerifyBatch(
 			switch req.GetInner().(type) {
 			case *roachpb.ScanRequest, *roachpb.ReverseScanRequest:
 				// Scans are supported.
-			case *roachpb.BeginTransactionRequest, *roachpb.EndTransactionRequest:
+			case *roachpb.EndTransactionRequest:
 				// These requests are ignored.
 			default:
 				return roachpb.NewErrorf("batch with scan option has non-scans: %s", ba)

pkg/kv/txn_coord_sender.go

@@ -88,7 +88,7 @@ const (
 // Since it is stateful, the TxnCoordSender needs to understand when a
 // transaction is "finished" and the state can be destroyed. As such there's a
 // contract that the client.Txn needs obey. Read-only transactions don't matter
-// - they're stateless. For the others, once a BeginTransaction is sent by the
+// - they're stateless. For the others, once an intent write is sent by the
 // client, the TxnCoordSender considers the transactions completed in the
 // following situations:
 // - A batch containing an EndTransactions (commit or rollback) succeeds.
@@ -783,10 +783,6 @@ func (tc *TxnCoordSender) Send(
 
 	startNs := tc.clock.PhysicalNow()
 
-	if _, ok := ba.GetArg(roachpb.BeginTransaction); ok {
-		return nil, roachpb.NewErrorf("BeginTransaction added before the TxnCoordSender")
-	}
-
 	ctx, sp := tc.AnnotateCtxWithSpan(ctx, opTxnCoordSender)
 	defer sp.Finish()
 

pkg/kv/txn_coord_sender_test.go

@@ -1440,12 +1440,12 @@ func TestRollbackErrorStopsHeartbeat(t *testing.T) {
 }
 
 // Test that intent tracking behaves correctly for transactions that attempt to
-// run a batch containing both a BeginTransaction and an EndTransaction. Since
-// in case of an error it's not easy to determine whether any intents have been
-// laid down (i.e. in case the batch was split by the DistSender and then there
-// was mixed success for the sub-batches, or in case a retriable error is
-// returned), the test verifies that all possible intents are properly tracked
-// and attached to a subsequent EndTransaction.
+// run a batch containing an EndTransaction. Since in case of an error it's not
+// easy to determine whether any intents have been laid down (i.e. in case the
+// batch was split by the DistSender and then there was mixed success for the
+// sub-batches, or in case a retriable error is returned), the test verifies
+// that all possible intents are properly tracked and attached to a subsequent
+// EndTransaction.
 func TestOnePCErrorTracking(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	ctx := context.Background()

pkg/kv/txn_interceptor_pipeliner.go

@@ -308,9 +308,6 @@ func (tp *txnPipeliner) chainToInFlightWrites(ba roachpb.BatchRequest) roachpb.B
 	// batch synchronously and avoid all of the overhead of pipelining.
 	if maxBatch := pipelinedWritesMaxBatchSize.Get(&tp.st.SV); maxBatch > 0 {
 		batchSize := int64(len(ba.Requests))
-		if _, hasBT := ba.GetArg(roachpb.BeginTransaction); hasBT {
-			batchSize--
-		}
 		if batchSize > maxBatch {
 			asyncConsensus = false
 		}
@@ -329,14 +326,7 @@ func (tp *txnPipeliner) chainToInFlightWrites(ba roachpb.BatchRequest) roachpb.B
 			// short-circuit immediately.
 			break
 		}
-
 		req := ru.GetInner()
-		if req.Method() == roachpb.BeginTransaction {
-			// Ignore BeginTransaction requests. They'll always be the first
-			// request in a batch and will never need to chain on any existing
-			// writes.
-			continue
-		}
 
 		if asyncConsensus {
 			// If we're currently planning on performing the batch with

pkg/kv/txn_interceptor_seq_num_allocator_test.go

@@ -105,8 +105,8 @@ func TestSequenceNumberAllocation(t *testing.T) {
 }
 
 // TestSequenceNumberAllocationTxnRequests tests sequence number allocation's
-// interaction with transaction state requests (BeginTxn, HeartbeatTxn, and
-// EndTxn). Only EndTxn requests should be assigned unique sequence numbers.
+// interaction with transaction state requests (HeartbeatTxn and EndTxn). Only
+// EndTxn requests should be assigned unique sequence numbers.
 func TestSequenceNumberAllocationTxnRequests(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	ctx := context.Background()
@@ -117,15 +117,13 @@ func TestSequenceNumberAllocationTxnRequests(t *testing.T) {
 
 	var ba roachpb.BatchRequest
 	ba.Header = roachpb.Header{Txn: &txn}
-	ba.Add(&roachpb.BeginTransactionRequest{RequestHeader: roachpb.RequestHeader{Key: keyA}})
 	ba.Add(&roachpb.HeartbeatTxnRequest{RequestHeader: roachpb.RequestHeader{Key: keyA}})
 	ba.Add(&roachpb.EndTransactionRequest{RequestHeader: roachpb.RequestHeader{Key: keyA}})
 
 	mockSender.MockSend(func(ba roachpb.BatchRequest) (*roachpb.BatchResponse, *roachpb.Error) {
-		require.Len(t, ba.Requests, 3)
+		require.Len(t, ba.Requests, 2)
 		require.Equal(t, enginepb.TxnSeq(0), ba.Requests[0].GetInner().Header().Sequence)
-		require.Equal(t, enginepb.TxnSeq(0), ba.Requests[1].GetInner().Header().Sequence)
-		require.Equal(t, enginepb.TxnSeq(1), ba.Requests[2].GetInner().Header().Sequence)
+		require.Equal(t, enginepb.TxnSeq(1), ba.Requests[1].GetInner().Header().Sequence)
 
 		br := ba.CreateReply()
 		br.Txn = ba.Txn

pkg/roachpb/api.go

@@ -504,9 +504,6 @@ func (*ReverseScanRequest) Method() Method { return ReverseScan }
 // Method implements the Request interface.
 func (*CheckConsistencyRequest) Method() Method { return CheckConsistency }
 
-// Method implements the Request interface.
-func (*BeginTransactionRequest) Method() Method { return BeginTransaction }
-
 // Method implements the Request interface.
 func (*EndTransactionRequest) Method() Method { return EndTransaction }
 
@@ -672,12 +669,6 @@ func (ccr *CheckConsistencyRequest) ShallowCopy() Request {
 	return &shallowCopy
 }
 
-// ShallowCopy implements the Request interface.
-func (btr *BeginTransactionRequest) ShallowCopy() Request {
-	shallowCopy := *btr
-	return &shallowCopy
-}
-
 // ShallowCopy implements the Request interface.
 func (etr *EndTransactionRequest) ShallowCopy() Request {
 	shallowCopy := *etr
@@ -1066,7 +1057,6 @@ func (*ScanRequest) flags() int { return isRead | isRange | isTxn | updatesReadT
 func (*ReverseScanRequest) flags() int {
 	return isRead | isRange | isReverse | isTxn | updatesReadTSCache | needsRefresh
 }
-func (*BeginTransactionRequest) flags() int { return isWrite | isTxn }
 
 // EndTransaction updates the write timestamp cache to prevent
 // replays. Replays for the same transaction key and timestamp will