Unmerged commits from golang/protobuf
awalterschulze opened this issue · 40 comments
gogoprotobuf: All commits on master up to Aug 27, 2019 have been merged - golang/protobuf@822fe56
This message is repeatedly edited to reflect the newest merged commit
I've opened #319 to track merging up to golang/protobuf@748d386 - feel free to close that issue.
Sorry for the delay.
No worries, thanks! Could you roll forward up to golang/protobuf@1909bc2?
done
Sorry for the noob question, but what's the actual process for doing the update? I'm interested in locally updating gogoproto on top of upstream's dev branch to test out some recent performance improvements (golang/protobuf@8cc9e46), but the process for merging in new upstream commits doesn't appear to be documented.
The process is: I manually do it when I get time using DiffMerge.app
I also read most of the code to make sure it won't interfere with gogoproto features.
I have to read especially carefully in generator.go and jsonpb.go, since there is quite a big diff already.
That diff seems really big so I'll probably do that in several steps.
Sorry for the bad news. The process is really old school and painful.
The merge process does look like a bit too much effort for me to do just for some early performance testing, so I'll just wait until it gets moved into the master branch there and eventually ends up here.
Thanks for the quick reply and for all the work you do maintaining gogoproto!
I agree :)
Looks like its going to be a while before it is moved to master. So I should probably get started on my own dev branch. I don't know when I'll get time, but I hope it will be before they merge dev into their master.
I have finally started work to merge the golang/protobuf dev into gogoprotobuf.
I am starting with an upstream branch created to merge this single commit
golang/protobuf@8cc9e46
Then I will merge this branch into a dev branch where I will continue merging the rest of the dev branch.
Status of Upstream
One package test is failing
github.com/gogo/protobuf/test/oneofembed
Todo
merge protoc-gen-go/generator/generator.go
merge proto/discard_test.go requires merge changes to generator.go
merge proto/proto3_test.go requires Proto3UnknownFields
update .proto files including well known types
regenerate well known types using new generator
merge protoc-gen-go/descriptor/descriptor.pb.go
merge ptypes/timestamp/timestamp.proto
Done
_conformance/conformance.go
proto/Makefile
proto/all_test.go
proto/any_test.go
proto/clone.go
proto/clone_test.go
proto/decode.go
proto/discard.go
proto/encode.go
proto/equal.go
proto/equal_test.go
proto/extensions.go
proto/extensions_test.go
proto/lib.go
proto/message_set.go
proto/message_set_test.go
proto/pointer_reflect.go
proto/pointer_unsafe.go
proto/properties.go
proto/proto3_proto/proto3.proto
proto/size_test.go
proto/size2_test.go
proto/table_marshal.go
proto/table_merge.go
proto/{testdata -> test_proto}/Makefile
proto/{testdata -> test_proto}/golden_test.go
proto/{testdata -> test_proto}/test.proto
proto/text.go
proto/text_parser.go
proto/text_parser_test.go
proto/text_test.go
proto/table_unmarshal.go
protoc-gen-gogo/generator/internal/remap/remap.go
protoc-gen-gogo/generator/internal/remap/remap_test.go
protoc-gen-gogo/generator/name_test.go
golang/protobuf/descriptor/descriptor_test.go -> protoc-gen-gogo/descriptor/descriptor_test.go
The golang/protobuf@8cc9e46 commit has now fully been merged into the gogoprotobuf upstream branch.
All tests and vet are passing.
I want to merge upstream into the gogoprotobuf dev branch and then continue merging the golang/protobuf dev branch changes in the gogoprotobuf dev branch.
It would be nice to hear from at least one gogoprotobuf user that the upstream branch is working though, since this is such a large change. And I had to rewrite quite a few things from scratch.
@a-robinson @tamird do you have time to test the gogoprotobuf upstream branch against your code?
@awalterschulze yes, I'll test out the gogoprotobuf dev branch in cockroach this week.
@a-robinson that would be awesome. Thank you so much.
gogoprotobuf has a lots of tests, but nothing beats a real world test.
I'm not sure yet whether it's due to the upstream protobuf changes or to your tweaks in this repo, but I've hit a couple problems integrating the upstream branch into cockroach so far.
- The generated struct types have two new extra fields in them:
XXX_NoUnkeyedLiteral struct{}andXXX_sizecache int32. This broke a couple places in our code -- one where we were creating a struct literal that assumed the number of fields in the struct, and another where we were casting from a non-proto type to the proto type, which depends on them having exactly the same fields. Both of these are really just problems in our code, and are trivial to fix. - A crash when trying to start up a cockroach process. I haven't yet dug into what the cause is, but it's coming in our own custom
proto.Cloneimplementation, so there's a decent chance it's due to tight coupling of our code with something in gogo/protobuf that isn't guaranteed to stay compatible:
panic: message field roachpb.ReplicaDescriptor without pointer [recovered]
panic: message field roachpb.ReplicaDescriptor without pointer [recovered]
panic: message field roachpb.ReplicaDescriptor without pointer
goroutine 35 [running]:
github.com/cockroachdb/cockroach/pkg/util/stop.(*Stopper).Recover(0xc42087e630, 0x27dc760, 0xc42086e120)
/go/src/github.com/cockroachdb/cockroach/pkg/util/stop/stopper.go:180 +0x11f
panic(0x21a0440, 0xc420278ba0)
/usr/local/go/src/runtime/panic.go:505 +0x229
github.com/cockroachdb/cockroach/pkg/util/stop.(*Stopper).Stop(0xc42087e630, 0x27dc760, 0xc42086e120)
/go/src/github.com/cockroachdb/cockroach/pkg/util/stop/stopper.go:415 +0x44c
panic(0x21a0440, 0xc420278ba0)
/usr/local/go/src/runtime/panic.go:505 +0x229
github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto.(*mergeInfo).computeMergeInfo(0xc420500280)
/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/table_merge.go:534 +0xfe4
github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto.(*mergeInfo).merge(0xc420500280, 0xc4204fa300, 0xc4204fa2a0)
/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/table_merge.go:113 +0x4eb
github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto.(*InternalMessageInfo).Merge(0x3c35be0, 0x27ce520, 0xc4204fa300, 0x27ce520, 0xc4204fa2a0)
/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/table_merge.go:50 +0x58
github.com/cockroachdb/cockroach/pkg/roachpb.(*RangeDescriptor).XXX_Merge(0xc4204fa300, 0x27ce520, 0xc4204fa2a0)
/go/src/github.com/cockroachdb/cockroach/pkg/roachpb/metadata.pb.go:149 +0x57
github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto.Merge(0x27ce520, 0xc4204fa300, 0x27ce520, 0xc4204fa2a0)
/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/clone.go:95 +0x277
github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto.Clone(0x27ce520, 0xc4204fa2a0, 0xc4204fa2a0, 0x27ce520)
/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/clone.go:52 +0x168
github.com/cockroachdb/cockroach/pkg/util/protoutil.Clone(0x27e7b80, 0xc4204fa2a0, 0x18, 0xc422b1b2a0)
/go/src/github.com/cockroachdb/cockroach/pkg/util/protoutil/clone.go:69 +0x92
github.com/cockroachdb/cockroach/pkg/storage/stateloader.StateLoader.Load(0xc420232000, 0xc420300780, 0x3, 0x20, 0x27dc760, 0xc420333a10, 0x27ec980, 0xc4206c0300, 0xc4204fa2a0, 0x0, ...)
/go/src/github.com/cockroachdb/cockroach/pkg/storage/stateloader/stateloader.go:73 +0xaa
github.com/cockroachdb/cockroach/pkg/storage.(*Replica).initRaftMuLockedReplicaMuLocked(0xc4201e5180, 0xc4204fa2a0, 0xc42048e280, 0x0, 0x0, 0x0)
/go/src/github.com/cockroachdb/cockroach/pkg/storage/replica.go:713 +0x332
github.com/cockroachdb/cockroach/pkg/storage.(*Replica).init(0xc4201e5180, 0xc4204fa2a0, 0xc42048e280, 0x0, 0x0, 0x0)
/go/src/github.com/cockroachdb/cockroach/pkg/storage/replica.go:678 +0xd9
github.com/cockroachdb/cockroach/pkg/storage.NewReplica(0xc4204fa2a0, 0xc420469400, 0x0, 0x0, 0x1, 0x9)
/go/src/github.com/cockroachdb/cockroach/pkg/storage/replica.go:668 +0x6f
github.com/cockroachdb/cockroach/pkg/storage.(*Store).Start.func1(0x1, 0x3c8c528, 0x0, 0x0, 0xc420042ad8, 0x2, 0x8, 0xc420042af0, 0x1, 0x1, ...)
/go/src/github.com/cockroachdb/cockroach/pkg/storage/store.go:1268 +0x10c
github.com/cockroachdb/cockroach/pkg/storage.IterateRangeDescriptors.func1(0xc420462088, 0x9, 0x2a, 0xc42046209b, 0x17, 0x17, 0x15178d665dae93d8, 0x0, 0x0, 0x0, ...)
/go/src/github.com/cockroachdb/cockroach/pkg/storage/store.go:1158 +0x291
github.com/cockroachdb/cockroach/pkg/storage/engine.MVCCIterateUsingIter(0x27dc760, 0xc420774600, 0x27ec980, 0xc4206c0300, 0xc420042a40, 0x9, 0x10, 0xc420042a70, 0xb, 0x10, ...)
/go/src/github.com/cockroachdb/cockroach/pkg/storage/engine/mvcc.go:1915 +0x10a
github.com/cockroachdb/cockroach/pkg/storage/engine.MVCCIterate(0x27dc760, 0xc420774600, 0x27ec980, 0xc4206c0300, 0xc420042a40, 0x9, 0x10, 0xc420042a70, 0xb, 0x10, ...)
/go/src/github.com/cockroachdb/cockroach/pkg/storage/engine/mvcc.go:1865 +0x1b2
github.com/cockroachdb/cockroach/pkg/storage.IterateRangeDescriptors(0x27dc760, 0xc420774600, 0x27ec980, 0xc4206c0300, 0xc4202ed6a0, 0x1, 0x1)
/go/src/github.com/cockroachdb/cockroach/pkg/storage/store.go:1161 +0x27b
github.com/cockroachdb/cockroach/pkg/storage.(*Store).Start(0xc420469400, 0x27dc760, 0xc420774600, 0xc42087e630, 0x100000001, 0xe00000000)
/go/src/github.com/cockroachdb/cockroach/pkg/storage/store.go:1262 +0x334
github.com/cockroachdb/cockroach/pkg/server.bootstrapCluster(0x27dc760, 0xc42086e120, 0x27d7260, 0xc42027a420, 0x0, 0xc4202b6190, 0x27dc760, 0xc4203332f0, 0xbebc200, 0xf, ...)
/go/src/github.com/cockroachdb/cockroach/pkg/server/node.go:226 +0x72d
github.com/cockroachdb/cockroach/pkg/server.(*Node).bootstrap(0xc4201b7b00, 0x27dc760, 0xc42086e120, 0xc42048b7f0, 0x1, 0x1, 0x100000001, 0xe00000000, 0x100000000, 0x1, ...)
/go/src/github.com/cockroachdb/cockroach/pkg/server/node.go:368 +0x1f8
github.com/cockroachdb/cockroach/pkg/server.(*Server).Start(0xc4201e7500, 0x27dc760, 0xc42086e120, 0x0, 0x0)
/go/src/github.com/cockroachdb/cockroach/pkg/server/server.go:998 +0x419c
github.com/cockroachdb/cockroach/pkg/cli.runStart.func1.2(0xc42087e5a0, 0xc4204b0010, 0xc420476540, 0x27dc760, 0xc42086e030, 0x30db8f9e, 0xed228dd95, 0x0, 0xc4200fafe0, 0x1007f0a03cad000)
/go/src/github.com/cockroachdb/cockroach/pkg/cli/start.go:520 +0x109
github.com/cockroachdb/cockroach/pkg/cli.runStart.func1(0xc4204b0010, 0x27dc760, 0xc42086e030, 0x27faf80, 0xc42086a000, 0xc42087e5a0, 0xc420476540, 0x30db8f9e, 0xed228dd95, 0x0, ...)
/go/src/github.com/cockroachdb/cockroach/pkg/cli/start.go:603 +0x11c
created by github.com/cockroachdb/cockroach/pkg/cli.runStart
/go/src/github.com/cockroachdb/cockroach/pkg/cli/start.go:476 +0x5eb
The smaller reproduction of running the test in https://github.com/cockroachdb/cockroach/blob/master/pkg/util/protoutil/clone_test.go also breaks:
--- FAIL: TestCloneProto (0.00s)
clone_test.go:78: *config.ZoneConfig: got unexpected panic message field config.GCPolicy without pointer
clone_test.go:78: *gossip.Info: got unexpected panic message field roachpb.Value without pointer
clone_test.go:78: *gossip.BootstrapInfo: got unexpected panic message field util.UnresolvedAddr without pointer
clone_test.go:78: *sqlbase.IndexDescriptor: got unexpected panic message field sqlbase.ForeignKeyReference without pointer
clone_test.go:78: *roachpb.SplitTrigger: got unexpected panic message field roachpb.RangeDescriptor without pointer
clone_test.go:78: *roachpb.Value: got unexpected panic message field hlc.Timestamp without pointer
clone_test.go:78: *roachpb.RangeDescriptor: got unexpected panic message field roachpb.ReplicaDescriptor without pointer
clone_test.go:78: *sqlbase.PartitioningDescriptor: got unexpected panic message field sqlbase.PartitioningDescriptor_List without pointer
The relevant code for not allowing merges of non-pointer fields does appear to be new in the upstream branch: https://github.com/gogo/protobuf/blob/upstream/proto/table_merge.go#L534
Minimal repro:
$ cat gogo_panic.go
package main
import (
"fmt"
"github.com/cockroachdb/cockroach/pkg/gossip"
"github.com/cockroachdb/cockroach/pkg/util/protoutil"
"github.com/gogo/protobuf/proto"
)
func main() {
cloned1 := proto.Clone(&gossip.Info{})
fmt.Printf("proto.Clone success: %v\n", cloned1)
cloned2 := protoutil.Clone(&gossip.Info{})
fmt.Printf("protoutil.Clone success: %v\n", cloned2)
}
$ go run gogo_panic.go
proto.Clone success: value:<timestamp:<> >
panic: message field roachpb.Value without pointer
goroutine 1 [running]:
github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto.(*mergeInfo).computeMergeInfo(0xc420209800)
/Users/alex/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/table_merge.go:534 +0x10eb
github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto.(*mergeInfo).merge(0xc420209800, 0xc420086dc0, 0xc420086d70)
/Users/alex/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/table_merge.go:113 +0x504
github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto.(*InternalMessageInfo).Merge(0x52bece0, 0x511c4e0, 0xc420086dc0, 0x511c4e0, 0xc420086d70)
/Users/alex/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/table_merge.go:50 +0x58
github.com/cockroachdb/cockroach/pkg/gossip.(*Info).XXX_Merge(0xc420086dc0, 0x511c4e0, 0xc420086d70)
/Users/alex/go/src/github.com/cockroachdb/cockroach/pkg/gossip/gossip.pb.go:190 +0x57
github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto.Merge(0x511c4e0, 0xc420086dc0, 0x511c4e0, 0xc420086d70)
/Users/alex/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/clone.go:95 +0x283
github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto.Clone(0x511c4e0, 0xc420086d70, 0xc420086d70, 0x511c4e0)
/Users/alex/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/clone.go:52 +0x168
github.com/cockroachdb/cockroach/pkg/util/protoutil.Clone(0x51260a0, 0xc420086d70, 0xc42010bf60, 0x1)
/Users/alex/go/src/github.com/cockroachdb/cockroach/pkg/util/protoutil/clone.go:69 +0x92
main.main()
/Users/alex/play/gogo_panic.go:15 +0xc5
exit status 2
-
Ok so the extra fields is a thing golang/protobuf intentionally added and they actually intentionally also added XXX_unkeyed... to avoid people creating instances of structs in an unnamed field manner. I am very open to a new extension(s) that disables the generation of these new XXX fields.
-
So it seems your Clone function would be fixed if I fix merge, right?
2.1 Could you provide a test that breaks merge, purely using the gogoprotobuf repo. Something we can commit as a regression test?
2.2 If you want to upstream your Clone function, that would also help to fix this long standing issue
#14
I am open to supporting Clone, I just haven't had time to write test and make sure it works.
Does that cover everything, or did I miss something?
Thank you so much for testing. This is really helpful.
Ok, I think the Clone crash was actually just user error on my side. I can't get it to reproduce after coming back and rebuilding everything. I'm able to start a cockroach cluster and run our tests without issues.
We can't switch to it particularly soon since we're in the midst of polishing off a major release right now, but should be ready to switch within 1-1.5 months if you think it'll be fully integrated by then.
Ok wait. I just want to make sure. Are you saying everything works on the upstream branch?
No worries about actual adoption. If it works for you, then I will merge upstream into dev and then start merging the rest of dev. But it is much less work than this initial patch, so it should go quick.
Ok wait. I just want to make sure. Are you saying everything works on the
upstreambranch?
I thought so, but apparently not. I had a non-upstream version of protoc-gen-gogo in my path that I think got used when I did my test this morning.
There's something weird going on with golang's interpretation of types across packages that causes the XXX_Merge code path to be taken when I call protoutil.Clone, but the old mergeStruct path to be taken when I call proto.Clone directly. The weird part is that the latter path gets taken when I copy the exact contents of protoutil.Clone into my main package and call that.
But since it's now clear that the problem is in XXX_Merge, here's an easy repro that should work with your non-cockroach proto of choice that has a non-pointer struct field:
func main() {
desc1 := &roachpb.RangeDescriptor{}
desc2 := &roachpb.RangeDescriptor{}
desc2.XXX_Merge(desc1)
}
Damn, I thought it was too good to be true.
I don't think we are supposed to call XXX_Merge directly?
And can you also provide non cockroach proto messages that we can use in a test and commit to gogoprotobuf, without adding any extra dependencies.
It breaks with just:
syntax = "proto3";
package foo;
option go_package = "foo";
import "gogoproto/gogo.proto";
message Foo {
Bar bar = 1 [(gogoproto.nullable) = false];
}
message Bar {
int64 baz = 1;
}
and
func main() {
f1 := &foo.Foo{}
f2 := &foo.Foo{}
f2.XXX_Merge(f1)
}
And I don't know whether we're supposed to call XXX_Merge directly, but the code I linked to in proto.Merge (which gets called by proto.Clone) does so.
I could reproduce it simply using proto.Clone or proto.Merge, without resorting to calling XXX methods directly.
Technically I don't support proto.Clone, but this did work before the merge.
#14
I'll see how hard it would be to fix.
@a-robinson I have fixed the one particular case that you reported
056fc7f
I suspect you might have more problems. Could you try running the new version on the upstream branch again?
I have merged upstream into dev and I am now starting to merge the following commits on golang/protobuf's branch into gogo/protobuf's dev branch
Testing of the up to date dev branch is welcomed.
Sorry for the delay, I haven't had much spare time lately.
I was able to successfully build cockroach and run a multi-node cluster, but ran into two problems when running tests.
- The internal
XXX_NoUnkeyedLiteralandXXX_sizecachefields are showing up in the output ofyaml.Marshalon a proto, which seems pretty undesirable:
--- FAIL: TestZoneConfigMarshalYAML (0.00s)
--- FAIL: TestZoneConfigMarshalYAML/#00 (0.00s)
zone_test.go:582: yaml.Marshal({RangeMinBytes:1 RangeMaxBytes:1 GC:{TTLSeconds:1 XXX_NoUnkeyedLiteral:{} XXX_sizecache:0} NumReplicas:1 Constraints:[] LeasePreferences:[] Subzones:[] SubzoneSpans:[] XXX_NoUnkeyedLiteral:{} XXX_sizecache:0})
got:
range_min_bytes: 1
range_max_bytes: 1
gc:
ttlseconds: 1
xxx_nounkeyedliteral: {}
xxx_sizecache: 0
num_replicas: 1
constraints: []
want:
range_min_bytes: 1
range_max_bytes: 1
gc:
ttlseconds: 1
num_replicas: 1
constraints: []
- I hit a segfault inside the gogoproto code in a couple different tests (
TestBackfillErrorsandTestLogic, on the same basic code path each time:
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x1 pc=0x80adc0]
goroutine 130435 [running]:
github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto.(*mergeInfo).computeMergeInfo.func1(0xc420b55558, 0xc422cde088)
/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/table_merge.go:234 +0x8c
github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto.(*mergeInfo).merge(0xc423bc9e40, 0xc420b55548, 0xc422cde078)
/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/table_merge.go:139 +0x110
github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto.(*mergeInfo).computeMergeInfo.func27(0xc420b55548, 0xc422cde078)
/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/table_merge.go:536 +0x3e
github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto.(*mergeInfo).merge(0xc423bc9b40, 0xc420b55530, 0xc422cde060)
/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/table_merge.go:139 +0x110
github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto.(*mergeInfo).computeMergeInfo.func27(0xc420b55530, 0xc422cde060)
/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/table_merge.go:536 +0x3e
github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto.(*mergeInfo).merge(0xc423bc9340, 0xc420b55500, 0xc422cde030)
/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/table_merge.go:139 +0x110
github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto.(*InternalMessageInfo).Merge(0x368c160, 0x261db40, 0xc420b55500, 0x261db40, 0xc422cde030)
/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/table_merge.go:50 +0x58
github.com/cockroachdb/cockroach/pkg/sql/sqlbase.(*TableDescriptor).XXX_Merge(0xc420b55500, 0x261db40, 0xc422cde030)
/go/src/github.com/cockroachdb/cockroach/pkg/sql/sqlbase/structured.pb.go:1328 +0x57
github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto.Merge(0x261db40, 0xc420b55500, 0x261db40, 0xc422cde030)
/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/clone.go:95 +0x277
github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto.Clone(0x261db40, 0xc422cde030, 0xc422cde030, 0x261db40)
/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/clone.go:52 +0x168
github.com/cockroachdb/cockroach/pkg/util/protoutil.Clone(0x2634a80, 0xc422cde030, 0x2601500, 0xc420454db0)
/go/src/github.com/cockroachdb/cockroach/pkg/util/protoutil/clone.go:69 +0x92
github.com/cockroachdb/cockroach/pkg/sql/distsqlrun.ConvertBackfillError(0x2626c80, 0xc422957d80, 0xc422cde030, 0xc421b0ee00, 0x2601500, 0xc420454db0)
/go/src/github.com/cockroachdb/cockroach/pkg/sql/distsqlrun/backfiller.go:310 +0x4c
github.com/cockroachdb/cockroach/pkg/sql/distsqlrun.(*indexBackfiller).runChunk.func2.1(0x2626c80, 0xc422957d80, 0xc42567cb60, 0x1, 0xc421f06e38)
/go/src/github.com/cockroachdb/cockroach/pkg/sql/distsqlrun/indexbackfiller.go:211 +0x26c
github.com/cockroachdb/cockroach/pkg/internal/client.(*DB).Txn.func1(0x2626c80, 0xc422957d80, 0xc42567cb60, 0xc421f06e38, 0x1523a2624a3e3557, 0x0)
/go/src/github.com/cockroachdb/cockroach/pkg/internal/client/db.go:516 +0x43
github.com/cockroachdb/cockroach/pkg/internal/client.(*Txn).Exec(0xc42567cb60, 0x2626c80, 0xc422957d80, 0xc425670101, 0xc42052c480, 0xc424e13870, 0xc424e13720)
/go/src/github.com/cockroachdb/cockroach/pkg/internal/client/txn.go:753 +0xfe
github.com/cockroachdb/cockroach/pkg/internal/client.(*DB).Txn(0xc420b12390, 0x2626c80, 0xc422957d80, 0xc423782180, 0x0, 0x0)
/go/src/github.com/cockroachdb/cockroach/pkg/internal/client/db.go:515 +0x133
github.com/cockroachdb/cockroach/pkg/sql/distsqlrun.(*indexBackfiller).runChunk.func2(0x2626d40, 0xc423cd9620, 0xc400000002, 0x2311c7e)
/go/src/github.com/cockroachdb/cockroach/pkg/sql/distsqlrun/indexbackfiller.go:200 +0xa6
github.com/cockroachdb/cockroach/pkg/sql/distsqlrun.(*indexBackfiller).runChunk(0xc422cde000, 0x2626d40, 0xc423cd9620, 0xc423cd9650, 0x1, 0x1, 0xc42385cc88, 0x6, 0x38, 0xc42003fc48, ...)
/go/src/github.com/cockroachdb/cockroach/pkg/sql/distsqlrun/indexbackfiller.go:276 +0x745
github.com/cockroachdb/cockroach/pkg/sql/distsqlrun.(*backfiller).mainLoop(0xc422cde000, 0x2626d40, 0xc423cd9620, 0x22c645c, 0x8)
/go/src/github.com/cockroachdb/cockroach/pkg/sql/distsqlrun/backfiller.go:137 +0x64a
github.com/cockroachdb/cockroach/pkg/sql/distsqlrun.(*backfiller).Run(0xc422cde000, 0xc42159aac0)
/go/src/github.com/cockroachdb/cockroach/pkg/sql/distsqlrun/backfiller.go:89 +0x1fa
created by github.com/cockroachdb/cockroach/pkg/sql/distsqlrun.(*Flow).Start
/go/src/github.com/cockroachdb/cockroach/pkg/sql/distsqlrun/flow.go:481 +0x393
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x1 pc=0x80adc0]
goroutine 22949 [running]:
github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto.(*mergeInfo).computeMergeInfo.func1(0xc4215b2e58, 0xc425c7d288)
/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/table_merge.go:234 +0x8c
github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto.(*mergeInfo).merge(0xc427a89c80, 0xc4215b2e48, 0xc425c7d278)
/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/table_merge.go:139 +0x110
github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto.(*mergeInfo).computeMergeInfo.func27(0xc4215b2e48, 0xc425c7d278)
/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/table_merge.go:536 +0x3e
github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto.(*mergeInfo).merge(0xc427a89940, 0xc4215b2e30, 0xc425c7d260)
/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/table_merge.go:139 +0x110
github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto.(*mergeInfo).computeMergeInfo.func27(0xc4215b2e30, 0xc425c7d260)
/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/table_merge.go:536 +0x3e
github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto.(*mergeInfo).merge(0xc427a889c0, 0xc4215b2e00, 0xc425c7d230)
/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/table_merge.go:139 +0x110
github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto.(*InternalMessageInfo).Merge(0x340bfa0, 0x2434580, 0xc4215b2e00, 0x2434580, 0xc425c7d230)
/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/table_merge.go:50 +0x58
github.com/cockroachdb/cockroach/pkg/sql/sqlbase.(*TableDescriptor).XXX_Merge(0xc4215b2e00, 0x2434580, 0xc425c7d230)
/go/src/github.com/cockroachdb/cockroach/pkg/sql/sqlbase/structured.pb.go:1328 +0x57
github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto.Merge(0x2434580, 0xc4215b2e00, 0x2434580, 0xc425c7d230)
/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/clone.go:95 +0x277
github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto.Clone(0x2434580, 0xc425c7d230, 0xc425c7d230, 0x2434580)
/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/clone.go:52 +0x168
github.com/cockroachdb/cockroach/pkg/util/protoutil.Clone(0x2448140, 0xc425c7d230, 0x2419800, 0xc4291d9800)
/go/src/github.com/cockroachdb/cockroach/pkg/util/protoutil/clone.go:69 +0x92
github.com/cockroachdb/cockroach/pkg/sql/distsqlrun.ConvertBackfillError(0x243b380, 0xc427a88540, 0xc425c7d230, 0xc42d963100, 0x2419800, 0xc4291d9800)
/go/src/github.com/cockroachdb/cockroach/pkg/sql/distsqlrun/backfiller.go:310 +0x4c
github.com/cockroachdb/cockroach/pkg/sql/distsqlrun.(*indexBackfiller).runChunk.func2.1(0x243b380, 0xc427a88540, 0xc42f9c8b60, 0x1, 0xc4320849d0)
/go/src/github.com/cockroachdb/cockroach/pkg/sql/distsqlrun/indexbackfiller.go:211 +0x26c
github.com/cockroachdb/cockroach/pkg/internal/client.(*DB).Txn.func1(0x243b380, 0xc427a88540, 0xc42f9c8b60, 0xc4320849d0, 0x1523a258ab5bea0a, 0x0)
/go/src/github.com/cockroachdb/cockroach/pkg/internal/client/db.go:516 +0x43
github.com/cockroachdb/cockroach/pkg/internal/client.(*Txn).Exec(0xc42f9c8b60, 0x243b380, 0xc427a88540, 0xc42f9c0101, 0xc4291d9780, 0xc432775870, 0xc432775720)
/go/src/github.com/cockroachdb/cockroach/pkg/internal/client/txn.go:753 +0xfe
github.com/cockroachdb/cockroach/pkg/internal/client.(*DB).Txn(0xc420fbe390, 0x243b380, 0xc427a88540, 0xc431a74fe0, 0x0, 0x0)
/go/src/github.com/cockroachdb/cockroach/pkg/internal/client/db.go:515 +0x133
github.com/cockroachdb/cockroach/pkg/sql/distsqlrun.(*indexBackfiller).runChunk.func2(0x243b440, 0xc42a8d38f0, 0xc400000002, 0x2170414)
/go/src/github.com/cockroachdb/cockroach/pkg/sql/distsqlrun/indexbackfiller.go:200 +0xa6
github.com/cockroachdb/cockroach/pkg/sql/distsqlrun.(*indexBackfiller).runChunk(0xc425c7d200, 0x243b440, 0xc42a8d38f0, 0xc42a8d3920, 0x1, 0x1, 0xc4320843c8, 0x2, 0x8, 0xc4320843d0, ...)
/go/src/github.com/cockroachdb/cockroach/pkg/sql/distsqlrun/indexbackfiller.go:276 +0x745
github.com/cockroachdb/cockroach/pkg/sql/distsqlrun.(*backfiller).mainLoop(0xc425c7d200, 0x243b440, 0xc42a8d38f0, 0x212ecaf, 0x8)
/go/src/github.com/cockroachdb/cockroach/pkg/sql/distsqlrun/backfiller.go:137 +0x64a
github.com/cockroachdb/cockroach/pkg/sql/distsqlrun.(*backfiller).Run(0xc425c7d200, 0xc42d269b40)
/go/src/github.com/cockroachdb/cockroach/pkg/sql/distsqlrun/backfiller.go:89 +0x1fa
created by github.com/cockroachdb/cockroach/pkg/sql/distsqlrun.(*Flow).Start
/go/src/github.com/cockroachdb/cockroach/pkg/sql/distsqlrun/flow.go:481 +0x393
Hi, I know this is an old issue but I'm working on updating dependencies for cockroach and hit this issue.
- What happens with xxx_unrecognized and yaml? Or what do you do to avoid
it? If you are not generating xxx_unrecognized then maybe we can add an
option to not generate these xxx fields either?
I think the generated code for those fields should have yaml:"-", just like it has for json. What do you think?
@awalterschulze can you expand on gogoprotobuf not supporting merge/clone? There is plenty of code for this, e.g. in https://github.com/gogo/protobuf/blob/master/proto/table_merge.go. Is this code known not to work?
This patch makes things work for us. Would you be open to making a change like this, even if we don't promise full support for Clone?
diff --git a/github.com/gogo/protobuf/proto/table_merge.go b/github.com/gogo/protobuf/proto/table_merge.go
index f520106e..ffb5d6b1 100644
--- a/github.com/gogo/protobuf/proto/table_merge.go
+++ b/github.com/gogo/protobuf/proto/table_merge.go
@@ -37,6 +37,7 @@ import (
"strings"
"sync"
"sync/atomic"
+ "unsafe"
)
// Merge merges the src message into dst.
@@ -530,6 +531,32 @@ func (mi *mergeInfo) computeMergeInfo() {
}
case reflect.Struct:
switch {
+ case !isPointer && isSlice: // E.g., []pb.T
+ mergeInfo := getMergeInfo(tf)
+ fieldSize := int(tf.Size())
+ mfi.merge = func(dst, src pointer) {
+ sbp := src.toBytes()
+ if *sbp != nil && len(*sbp) > 0 {
+ srcSlice := *sbp
+ dbp := dst.toBytes()
+ if len(*dbp) != 0 {
+ panic("merge for []T not implemented")
+ }
+ dstSlice := make([]byte, len(srcSlice)*fieldSize)
+ srcPtr := uintptr(unsafe.Pointer(&srcSlice[0]))
+ dstPtr := uintptr(unsafe.Pointer(&dstSlice[0]))
+ for i := 0; i < len(srcSlice); i++ {
+ mergeInfo.merge(
+ pointer{p: unsafe.Pointer(dstPtr + uintptr(i*fieldSize))},
+ pointer{p: unsafe.Pointer(srcPtr + uintptr(i*fieldSize))},
+ )
+ }
+ // Convert the length/cap to account for fieldSize.
+ *dbp = dstSlice[:len(srcSlice):len(srcSlice)]
+ }
+ }
case !isPointer:
mergeInfo := getMergeInfo(tf)
mfi.merge = func(dst, src pointer) {
There are three (two new) extensions that you can use to turn off the generation of XXX fields so you don't have any problems with yaml:
goproto_unrecognized
goproto_unkeyed (alpha) | Message | bool | if false, XXX_unkeyed field is not generated. | true
goproto_sizecache (alpha) | Message | bool | if false, XXX_sizecache field is not generated. | true
Merge and Clone are not supported, because someone has not taken the time to write tests for a lot of combinations and made sure that it is working. Or in other words, I was lazy :)
I believe @jmarais and @donaldgraham the new maintainers of gogoprotobuf would be open to this support being added, but you would have to at least provide test cases, to make sure that no regression occurs. That way limited support could be grown into full support over time.
I hope that helps
Side note: As a rule: unsafe imports should only be added to files that have the unsafe build tag.
Thanks! I will work on a PR for the clone thing. On the yaml thing - those two extensions are not in the last release unfortunately (and the XXX fields are). Is there a catch with using goproto_sizecache? (I assume the field is used for something?) Anyway, I worked around it by implementing a custom MarshalYAML.
By the way, do you have a sense of when the next release might be?