Merge #27112 #27114 #27116 #27119

27112: roachpb: replace `gogoproto.onlyone` with `oneof` in BatchRequest/BatchResponse r=nvanbenschoten a=nvanbenschoten

All Requests and Responses pass through RequestUnion/ResponseUnion structs
when they are added to BatchRequests/BatchResponses. In order to ensure
that only one Request type can be assigned to one of these RequestUnion
or ResponseUnion structs, we currently use gogoproto's approach to tagged
unions: the `gogoproto.onlyone` option.

This option was introduced before proto3. Proto3
then added the `oneof` option, which for all intents and purposes addresses
the same issue: https://developers.google.com/protocol-buffers/docs/proto#oneof.
However, there is one major difference between the two options, which
is in their generated code. `gogoproto.onlyone` will generate
a single flat struct with pointers to each possible variant type.
`oneof` will generate a union interface and an interface "wrapper"
struct for each variant type. The effect of this is that `onlyone`
will generate code that looks like this:

```
type Union struct {
    Variant1 *Variant1Type
    Variant2 *Variant2Type
    ...
}
```

While `oneof` will generate code the looks like this:

```
type Union struct {
    Value isUnion_Value
}

type isUnion_Value interface {
    ...
}

type Union_Variant1 struct {
    Variant1 *Variant1Type
}

type Union_Variant2 struct {
    Variant2 *Variant2Type
}
```

There are pretty obvious tradeoffs to each. For one, `oneof` introduces an
extra layer of indirection, which forces an extra allocation. It also doesn't
generate particularly useful setters and getters. On the other hand, `onlyone`
creates a large struct that grows linearly with the number of variants.
Neither approach is great, and there has been **A LOT** of discussion on this:
- golang/protobuf#78
- golang/protobuf#283
- gogo/protobuf#103
- gogo/protobuf#168

Clearly neither approach is ideal, ergonomically or with regard to performance.
However, over time, the tradeoff has been getting worse for us and it's time we
consider switching over to `oneof` in `RequestUnion` and `ResponseUnion`. These
structs have gotten huge as more and more request variants have been added:
`RequestUnion` has grown to **328 bytes** and `ResponseUnion` has grown to **320 bytes**.
It has gotten to the point where the wasted space is non-negligible.

This change switches over to `oneof` to shrink these union structs down to more
manageable sizes (16 bytes each). The downside of this is that in reducing the struct
size we end up introducing an extra allocation. This isn't great, but we can avoid
the extra allocation in some places (like `BatchRequest.CreateReply`) by grouping
the allocation with that of the Request/Response itself. We've seen previous cases
like #4216 where adding in an extra allocation/indirection is a net-win if it
reduces a commonly used struct's size significantly.

The other downside to this change is that the ergonomics of `oneof` aren't quite
as nice as `gogo.onlyone`. Specifically, `gogo.onlyone` generates getters and
setters called `GetValue` and `SetValue` that provide access to the wrapped
`interface{}`, which we can assert to a `Request`. `oneof` doesn't provide
such facilities. This was the cause of a lot of the discussions linked above.
While it we be nice for this to be resolved upstream, I think we've waited long
enough (~3 years) for a resolution to those discussions. For now, we'll just
generate the getters and setters ourselves.

This change demonstrated about a **5%** improvement when running kv95 on my local
laptop. When run on a three-node GCE cluster (4 vCPUs), the improvements were
less pronounced but still present. kv95 showed a throughput improvement of **2.4%**.
Running kv100 showed a much more dramatic improvement of **18%** on the three-node
GCE cluster. I think this is because kv100 is essentially a hot loop where all reads miss
because the cluster remains empty, so it's the best-case scenario for this change. Still,
the impact was shocking.

Release note (performance improvement): Reduce the memory size of commonly used
Request and Response objects.

27114: opt/sql: fix explain analyze missing option r=asubiotto a=asubiotto

ConstructExplain previously ignored the ANALYZE option so any EXPLAIN
ANALYZE statement would result in execution as an EXPLAIN (DISTSQL)
statement. The ANALYZE option is now observed in ConstructExplain.

Additionally, the stmtType field from the explainDistSQLNode has been
removed because it was not necessary and it was unclear how to pass this
from the `execFactory`.

Release note: None

27116: Makefile: learn that roachtest depends on optimizer-generated code r=benesch a=benesch

As mentioned in cd4415c, the Makefile will one day be smart enough to
deduce this on its own, but for now it's simpler to explicitly list the
commands that require generated code. Note that the simple but coarse
solution of assuming that all commands depend on generated code is
inviable as some of these commands are used to generate the code in the
first place.

Release note: None

27119: storage: extract replica unlinking into store method r=tschottdorf a=benesch

Extract some code that was duplicated in three places into a dedicated
helper method. Prerequisite for #27061.

Release note: None

Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Co-authored-by: Alfonso Subiotto Marqués <alfonso@cockroachlabs.com>
Co-authored-by: Nikhil Benesch <nikhil.benesch@gmail.com>

Author: 4 people

Makefile

@@ -1429,6 +1429,7 @@ logictestccl-package = ./pkg/ccl/logictestccl
 
 # Additional dependencies for binaries that depend on generated code.
 bin/workload bin/docgen bin/roachtest: $(SQLPARSER_TARGETS) $(PROTOBUF_TARGETS)
+bin/roachtest: $(OPTGEN_TARGETS)
 
 $(bins): bin/%: bin/%.d | bin/prereqs bin/.submodules-initialized
 	@echo go install -v $*

pkg/ccl/backupccl/backup_test.go

@@ -534,8 +534,9 @@ func checkInProgressBackupRestore(
 	params := base.TestClusterArgs{}
 	params.ServerArgs.Knobs.Store = &storage.StoreTestingKnobs{
 		TestingResponseFilter: func(ba roachpb.BatchRequest, br *roachpb.BatchResponse) *roachpb.Error {
-			for _, res := range br.Responses {
-				if res.Export != nil || res.Import != nil {
+			for _, ru := range br.Responses {
+				switch ru.GetInner().(type) {
+				case *roachpb.ExportResponse, *roachpb.ImportResponse:
 					<-allowResponse
 				}
 			}

pkg/kv/transport_race.go

@@ -19,7 +19,7 @@ package kv
 
 import (
 	"context"
-	"encoding/gob"
+	"encoding/json"
 	"io/ioutil"
 	"math/rand"
 	"sync/atomic"
@@ -74,7 +74,7 @@ func GRPCTransportFactory(
 				// are evicted in FIFO order.
 				const size = 1000
 				bas := make([]*roachpb.BatchRequest, size)
-				encoder := gob.NewEncoder(ioutil.Discard)
+				encoder := json.NewEncoder(ioutil.Discard)
 				for {
 					iters++
 					start := timeutil.Now()

pkg/kv/truncate_test.go

@@ -148,7 +148,7 @@ func TestTruncate(t *testing.T) {
 
 		original := roachpb.BatchRequest{Requests: make([]roachpb.RequestUnion, len(goldenOriginal.Requests))}
 		for i, request := range goldenOriginal.Requests {
-			original.Requests[i].SetValue(request.GetInner().ShallowCopy())
+			original.Requests[i].MustSetInner(request.GetInner().ShallowCopy())
 		}
 
 		desc := &roachpb.RangeDescriptor{

pkg/roachpb/api.go

@@ -446,22 +446,12 @@ func (*NoopResponse) Verify(_ Request) error {
 	return nil
 }
 
-// GetInner returns the Request contained in the union.
-func (ru RequestUnion) GetInner() Request {
-	return ru.GetValue().(Request)
-}
-
-// GetInner returns the Response contained in the union.
-func (ru ResponseUnion) GetInner() Response {
-	return ru.GetValue().(Response)
-}
-
 // MustSetInner sets the Request contained in the union. It panics if the
 // request is not recognized by the union type. The RequestUnion is reset
 // before being repopulated.
 func (ru *RequestUnion) MustSetInner(args Request) {
 	ru.Reset()
-	if !ru.SetValue(args) {
+	if !ru.SetInner(args) {
 		panic(fmt.Sprintf("%T excludes %T", ru, args))
 	}
 }
@@ -471,7 +461,7 @@ func (ru *RequestUnion) MustSetInner(args Request) {
 // before being repopulated.
 func (ru *ResponseUnion) MustSetInner(reply Response) {
 	ru.Reset()
-	if !ru.SetValue(reply) {
+	if !ru.SetInner(reply) {
 		panic(fmt.Sprintf("%T excludes %T", ru, reply))
 	}
 }

pkg/roachpb/api.pb.go

@@ -1258,101 +1258,96 @@ message GetSnapshotForMergeResponse {
 // Be cautious about deprecating fields as doing so can lead to inconsistencies
 // between replicas.
 message RequestUnion {
-  option (gogoproto.onlyone) = true;
-
-  GetRequest get = 1;
-  PutRequest put = 2;
-  ConditionalPutRequest conditional_put = 3;
-  IncrementRequest increment = 4;
-  DeleteRequest delete = 5;
-  DeleteRangeRequest delete_range = 6;
-  ClearRangeRequest clear_range = 38;
-  ScanRequest scan = 7;
-  BeginTransactionRequest begin_transaction = 8;
-  EndTransactionRequest end_transaction = 9;
-  AdminSplitRequest admin_split = 10;
-  AdminMergeRequest admin_merge = 11;
-  AdminTransferLeaseRequest admin_transfer_lease = 29;
-  AdminChangeReplicasRequest admin_change_replicas = 35;
-  HeartbeatTxnRequest heartbeat_txn = 12;
-  GCRequest gc = 13;
-  PushTxnRequest push_txn = 14;
-  reserved 15;
-  ResolveIntentRequest resolve_intent = 16;
-  ResolveIntentRangeRequest resolve_intent_range = 17;
-  MergeRequest merge = 18;
-  TruncateLogRequest truncate_log = 19;
-  RequestLeaseRequest request_lease = 20;
-  ReverseScanRequest reverse_scan = 21;
-  ComputeChecksumRequest compute_checksum = 22;
-  reserved 23;
-  CheckConsistencyRequest check_consistency = 24;
-  NoopRequest noop = 25;
-  InitPutRequest init_put = 26;
-  reserved 27;
-  TransferLeaseRequest transfer_lease = 28;
-  LeaseInfoRequest lease_info = 30;
-  WriteBatchRequest write_batch = 31;
-  ExportRequest export = 32;
-  ImportRequest import = 34;
-  QueryTxnRequest query_txn = 33;
-  QueryIntentRequest query_intent = 42;
-  AdminScatterRequest admin_scatter = 36;
-  AddSSTableRequest add_sstable = 37;
-  RecomputeStatsRequest recompute_stats = 39;
-  RefreshRequest refresh = 40;
-  RefreshRangeRequest refresh_range = 41;
-  GetSnapshotForMergeRequest get_snapshot_for_merge = 43;
+  oneof value {
+    GetRequest get = 1;
+    PutRequest put = 2;
+    ConditionalPutRequest conditional_put = 3;
+    IncrementRequest increment = 4;
+    DeleteRequest delete = 5;
+    DeleteRangeRequest delete_range = 6;
+    ClearRangeRequest clear_range = 38;
+    ScanRequest scan = 7;
+    BeginTransactionRequest begin_transaction = 8;
+    EndTransactionRequest end_transaction = 9;
+    AdminSplitRequest admin_split = 10;
+    AdminMergeRequest admin_merge = 11;
+    AdminTransferLeaseRequest admin_transfer_lease = 29;
+    AdminChangeReplicasRequest admin_change_replicas = 35;
+    HeartbeatTxnRequest heartbeat_txn = 12;
+    GCRequest gc = 13;
+    PushTxnRequest push_txn = 14;
+    ResolveIntentRequest resolve_intent = 16;
+    ResolveIntentRangeRequest resolve_intent_range = 17;
+    MergeRequest merge = 18;
+    TruncateLogRequest truncate_log = 19;
+    RequestLeaseRequest request_lease = 20;
+    ReverseScanRequest reverse_scan = 21;
+    ComputeChecksumRequest compute_checksum = 22;
+    CheckConsistencyRequest check_consistency = 24;
+    NoopRequest noop = 25;
+    InitPutRequest init_put = 26;
+    TransferLeaseRequest transfer_lease = 28;
+    LeaseInfoRequest lease_info = 30;
+    WriteBatchRequest write_batch = 31;
+    ExportRequest export = 32;
+    ImportRequest import = 34;
+    QueryTxnRequest query_txn = 33;
+    QueryIntentRequest query_intent = 42;
+    AdminScatterRequest admin_scatter = 36;
+    AddSSTableRequest add_sstable = 37;
+    RecomputeStatsRequest recompute_stats = 39;
+    RefreshRequest refresh = 40;
+    RefreshRangeRequest refresh_range = 41;
+    GetSnapshotForMergeRequest get_snapshot_for_merge = 43;
+  }
+  reserved 15, 23, 27;
 }
 
 // A ResponseUnion contains exactly one of the responses.
 // The values added here must match those in RequestUnion.
 message ResponseUnion {
-  option (gogoproto.onlyone) = true;
-
-  GetResponse get = 1;
-  PutResponse put = 2;
-  ConditionalPutResponse conditional_put = 3;
-  IncrementResponse increment = 4;
-  DeleteResponse delete = 5;
-  DeleteRangeResponse delete_range = 6;
-  ClearRangeResponse clear_range = 38;
-  ScanResponse scan = 7;
-  BeginTransactionResponse begin_transaction = 8;
-  EndTransactionResponse end_transaction = 9;
-  AdminSplitResponse admin_split = 10;
-  AdminMergeResponse admin_merge = 11;
-  AdminTransferLeaseResponse admin_transfer_lease = 29;
-  AdminChangeReplicasResponse admin_change_replicas = 35;
-  HeartbeatTxnResponse heartbeat_txn = 12;
-  GCResponse gc = 13;
-  PushTxnResponse push_txn = 14;
-  reserved 15;
-  ResolveIntentResponse resolve_intent = 16;
-  ResolveIntentRangeResponse resolve_intent_range = 17;
-  MergeResponse merge = 18;
-  TruncateLogResponse truncate_log = 19;
-  RequestLeaseResponse request_lease = 20;
-  ReverseScanResponse reverse_scan = 21;
-  ComputeChecksumResponse compute_checksum = 22;
-  reserved 23;
-  CheckConsistencyResponse check_consistency = 24;
-  NoopResponse noop = 25;
-  InitPutResponse init_put = 26;
-  reserved 27;
-  reserved 28; // TransferLease and RequestLease both use RequestLeaseResponse
-  LeaseInfoResponse lease_info = 30;
-  WriteBatchResponse write_batch = 31;
-  ExportResponse export = 32;
-  ImportResponse import = 34;
-  QueryTxnResponse query_txn = 33;
-  QueryIntentResponse query_intent = 42;
-  AdminScatterResponse admin_scatter = 36;
-  AddSSTableResponse add_sstable = 37;
-  RecomputeStatsResponse recompute_stats = 39;
-  RefreshResponse refresh = 40;
-  RefreshRangeResponse refresh_range = 41;
-  GetSnapshotForMergeResponse get_snapshot_for_merge = 43;
+  oneof value {
+    GetResponse get = 1;
+    PutResponse put = 2;
+    ConditionalPutResponse conditional_put = 3;
+    IncrementResponse increment = 4;
+    DeleteResponse delete = 5;
+    DeleteRangeResponse delete_range = 6;
+    ClearRangeResponse clear_range = 38;
+    ScanResponse scan = 7;
+    BeginTransactionResponse begin_transaction = 8;
+    EndTransactionResponse end_transaction = 9;
+    AdminSplitResponse admin_split = 10;
+    AdminMergeResponse admin_merge = 11;
+    AdminTransferLeaseResponse admin_transfer_lease = 29;
+    AdminChangeReplicasResponse admin_change_replicas = 35;
+    HeartbeatTxnResponse heartbeat_txn = 12;
+    GCResponse gc = 13;
+    PushTxnResponse push_txn = 14;
+    ResolveIntentResponse resolve_intent = 16;
+    ResolveIntentRangeResponse resolve_intent_range = 17;
+    MergeResponse merge = 18;
+    TruncateLogResponse truncate_log = 19;
+    RequestLeaseResponse request_lease = 20;
+    ReverseScanResponse reverse_scan = 21;
+    ComputeChecksumResponse compute_checksum = 22;
+    CheckConsistencyResponse check_consistency = 24;
+    NoopResponse noop = 25;
+    InitPutResponse init_put = 26;
+    LeaseInfoResponse lease_info = 30;
+    WriteBatchResponse write_batch = 31;
+    ExportResponse export = 32;
+    ImportResponse import = 34;
+    QueryTxnResponse query_txn = 33;
+    QueryIntentResponse query_intent = 42;
+    AdminScatterResponse admin_scatter = 36;
+    AddSSTableResponse add_sstable = 37;
+    RecomputeStatsResponse recompute_stats = 39;
+    RefreshResponse refresh = 40;
+    RefreshRangeResponse refresh_range = 41;
+    GetSnapshotForMergeResponse get_snapshot_for_merge = 43;
+  }
+  reserved 15, 23, 27, 28;
 }
 
 // A Header is attached to a BatchRequest, encapsulating routing and auxiliary

pkg/roachpb/api.proto

@@ -108,7 +108,7 @@ func TestMustSetInner(t *testing.T) {
 	if m := req.GetInner().Method(); m != EndTransaction {
 		t.Fatalf("unexpected request: %s in %+v", m, req)
 	}
-	if _, isET := res.GetValue().(*EndTransactionResponse); !isET {
+	if _, isET := res.GetInner().(*EndTransactionResponse); !isET {
 		t.Fatalf("unexpected response union: %+v", res)
 	}
 }