Bug Report: Update VSchema not send version if there are multiple vtgates operating at the same time.

[javamyth]

Overview of the Issue

hi,There is no version number passed here. There may be concurrency issues when updating VSchema if there are multiple vtgates operating at the same time.

Reproduction Steps

N/A

Binary Version

vtgate version Version: 18.0.4

Operating System and Environment details

N/A

Log Fragments

N/A

[mattlord]

The vschema has a timestamp in it. But more importantly it's versioned in the topology server (e.g. etcd). I'm going to close this for now as it's not clear to me what bug is being called out. If I'm missing something please let us know and we can re-open this any time. Thanks!

[javamyth]

The vschema has a timestamp in it. But more importantly it's versioned in the topology server (e.g. etcd). I'm going to close this for now as it's not clear to me what bug is being called out. If I'm missing something please let us know and we can re-open this any time. Thanks!

The UpdateVSchema method does not pass the version number and will be overwritten directly. This will cause it to be unsafe when multiple vtgate processes execute UpdateVSchema at the same time. Please take a look at the version of ts.globalCell.Update that passes nil, thank you.

[javamyth]

What is passed here is null, not version. If the data of a certain node is updated, there will be security issues.

[mattlord]

Thanks! I think you are correct here and we’re not providing serializability (although we could via etcd key versions and revisions) so you could read an old version, update it, and save the updated old structure overwriting intermediate changes. Because there’s no synchronization or concurrency controls concurrent writes from multiple vtgates will have a non-deterministic result and we could lose updates. I’ll reopen and let the query serving team comment.

[mattlord]

We should be able to enforce serializable changes across vtgates by leveraging the topo key version. For an example of where we're doing this today, look at the topo Tablet records/keys and the TabletInfo struct used in memory — same for the topo Shard records/keys and the ShardInfo in memory structure. For example:

vitess/go/vt/topo/shard.go

Lines 165 to 222 in f27d287

	// GetShard is a high level function to read shard data.
	// It generates trace spans.
	func (ts *Server) GetShard(ctx context.Context, keyspace, shard string) (*ShardInfo, error) {
	if err := ValidateKeyspaceName(keyspace); err != nil {
	return nil, err
	}
	if _, _, err := ValidateShardName(shard); err != nil {
	return nil, err
	}
	span, ctx := trace.NewSpan(ctx, "TopoServer.GetShard")
	span.Annotate("keyspace", keyspace)
	span.Annotate("shard", shard)
	defer span.Finish()
	shardPath := shardFilePath(keyspace, shard)
	data, version, err := ts.globalCell.Get(ctx, shardPath)
	if err != nil {
	return nil, err
	}
	value := &topodatapb.Shard{}
	if err = value.UnmarshalVT(data); err != nil {
	return nil, vterrors.Wrapf(err, "GetShard(%v,%v): bad shard data", keyspace, shard)
	}
	return NewShardInfo(keyspace, shard, value, version), nil
	}
	// updateShard updates the shard data, with the right version.
	// It also creates a span, and dispatches the event.
	func (ts *Server) updateShard(ctx context.Context, si *ShardInfo) error {
	span, ctx := trace.NewSpan(ctx, "TopoServer.UpdateShard")
	span.Annotate("keyspace", si.keyspace)
	span.Annotate("shard", si.shardName)
	defer span.Finish()
	data, err := si.Shard.MarshalVT()
	if err != nil {
	return err
	}
	shardPath := shardFilePath(si.keyspace, si.shardName)
	newVersion, err := ts.globalCell.Update(ctx, shardPath, data, si.version)
	if err != nil {
	return err
	}
	si.version = newVersion
	event.Dispatch(&events.ShardChange{
	KeyspaceName: si.Keyspace(),
	ShardName: si.ShardName(),
	Shard: si.Shard,
	Status: "updated",
	})
	return nil
	}

Writing the Shard topo key will fail if the version provided in the Update() call is older than the key's current version.