Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unmerged commits from golang/protobuf #191

Open
awalterschulze opened this issue Jul 26, 2016 · 40 comments
Open

Unmerged commits from golang/protobuf #191

awalterschulze opened this issue Jul 26, 2016 · 40 comments
Assignees

Comments

@awalterschulze
Copy link
Member

awalterschulze commented Jul 26, 2016

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

@awalterschulze awalterschulze changed the title Unmerged commits Unmerged commits from golang/protobuf Aug 21, 2016
@awalterschulze awalterschulze self-assigned this Sep 11, 2016
@tamird
Copy link
Contributor

tamird commented Aug 6, 2017

I've opened #319 to track merging up to golang/protobuf@748d386 - feel free to close that issue.

@awalterschulze
Copy link
Member Author

Sorry for the delay.

@tamird
Copy link
Contributor

tamird commented Aug 8, 2017

No worries, thanks! Could you roll forward up to golang/protobuf@1909bc2?

@awalterschulze
Copy link
Member Author

done

@a-robinson
Copy link

a-robinson commented Nov 9, 2017

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.

@tamird
Copy link
Contributor

tamird commented Nov 9, 2017 via email

@awalterschulze
Copy link
Member Author

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.

@awalterschulze
Copy link
Member Author

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.

@awalterschulze
Copy link
Member Author

Sorry for the bad news. The process is really old school and painful.

@a-robinson
Copy link

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!

@awalterschulze
Copy link
Member Author

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.

@awalterschulze
Copy link
Member Author

awalterschulze commented Feb 24, 2018

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

@awalterschulze
Copy link
Member Author

awalterschulze commented Feb 25, 2018

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?

@a-robinson
Copy link

@awalterschulze yes, I'll test out the gogoprotobuf dev branch in cockroach this week.

@awalterschulze
Copy link
Member Author

@a-robinson that would be awesome. Thank you so much.
gogoprotobuf has a lots of tests, but nothing beats a real world test.

@a-robinson
Copy link

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.

  1. The generated struct types have two new extra fields in them: XXX_NoUnkeyedLiteral struct{} and XXX_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.
  2. 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.Clone implementation, 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

@a-robinson
Copy link

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

@a-robinson
Copy link

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

@a-robinson
Copy link

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

@awalterschulze
Copy link
Member Author

  1. 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.

  2. 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.

@a-robinson
Copy link

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.

@awalterschulze
Copy link
Member Author

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.

@a-robinson
Copy link

Ok wait. I just want to make sure. Are you saying everything works on the upstream branch?

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)
}

@awalterschulze
Copy link
Member Author

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.

@a-robinson
Copy link

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)
}

@a-robinson
Copy link

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.

@awalterschulze
Copy link
Member Author

awalterschulze commented Mar 14, 2018 via email

@awalterschulze
Copy link
Member Author

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.

@awalterschulze
Copy link
Member Author

@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?

@awalterschulze
Copy link
Member Author

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

@awalterschulze
Copy link
Member Author

Testing of the up to date dev branch is welcomed.

@a-robinson
Copy link

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.

  1. The internal XXX_NoUnkeyedLiteral and XXX_sizecache fields are showing up in the output of yaml.Marshal on 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: []
  1. I hit a segfault inside the gogoproto code in a couple different tests (TestBackfillErrors and TestLogic, 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

@awalterschulze
Copy link
Member Author

awalterschulze commented Apr 9, 2018 via email

@RaduBerinde
Copy link

Hi, I know this is an old issue but I'm working on updating dependencies for cockroach and hit this issue.

  1. 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?

@RaduBerinde
Copy link

@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?

@RaduBerinde
Copy link

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) {

@awalterschulze
Copy link
Member Author

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

@awalterschulze
Copy link
Member Author

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.

@RaduBerinde
Copy link

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?

@awalterschulze
Copy link
Member Author

awalterschulze commented Dec 9, 2018 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants