Add linearizable support to SQL VSchema management by mattlord · Pull Request #17401 · vitessio/vitess

mattlord · 2024-12-17T19:27:35Z

Description

This PR prevents lost writes when using the VTGate SQL API for VSchema management (see issue).

It also lays the foundation for supporting linearizability guarantees for vschemas within Vitess. Please see the tracking issue for the other known pieces.

Related Issue(s)

Fixes: Add versioning to the low level topo interface for VSchemas using the existing Info struct model #17399
Fixes: Bug Report: Update VSchema not send version if there are multiple vtgates operating at the same time. #15794
Part of: Tracking Issue: Make VSchema management linearizable #17398

Checklist

"Backport to:" labels have been added if this change should be back-ported to release branches
If this change is to be back-ported to previous releases, a justification is included in the PR description
Tests were added or are not required
Did the new or modified tests pass consistently locally and on CI?
Documentation was added or is not required

vitess-bot · 2024-12-17T19:27:38Z

Signed-off-by: Matt Lord <mattalord@gmail.com>

codecov · 2025-01-13T23:43:50Z

Codecov Report

Attention: Patch coverage is 76.35659% with 61 lines in your changes missing coverage. Please review.

Project coverage is 67.64%. Comparing base (1df1dc8) to head (2e008f6).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
go/vt/vtctl/vtctl.go	37.03%	17 Missing ⚠️
go/vt/topotools/vschema_ddl.go	71.42%	16 Missing ⚠️
go/cmd/vtcombo/cli/vschema_watcher.go	0.00%	7 Missing ⚠️
go/vt/vtcombo/tablet_map.go	0.00%	5 Missing ⚠️
go/vt/topo/vschema.go	90.00%	4 Missing ⚠️
go/vt/vtgate/vschema_manager.go	55.55%	4 Missing ⚠️
go/vt/vtctl/grpcvtctldserver/server.go	88.88%	3 Missing ⚠️
go/vt/topo/helpers/copy.go	33.33%	2 Missing ⚠️
go/test/utils/diff.go	0.00%	1 Missing ⚠️
go/vt/wrangler/materializer.go	92.30%	1 Missing ⚠️
... and 1 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #17401      +/-   ##
==========================================
+ Coverage   67.63%   67.64%   +0.01%     
==========================================
  Files        1586     1586              
  Lines      255566   255629      +63     
==========================================
+ Hits       172850   172924      +74     
+ Misses      82716    82705      -11

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

This reverts commit cd455a0. Signed-off-by: Matt Lord <mattalord@gmail.com>

Signed-off-by: Matt Lord <mattalord@gmail.com>

rohit-nayak-ps · 2025-01-17T10:00:42Z

go/vt/vtgate/executorcontext/vcursor_impl.go

+			return err
+		}
+	} else {
+		// Use the cached version as we are in read-only mode


When do we get to this code path? Later on we are calling vc.vm.UpdateVSchema() which is expected to write back to the topo, correct? Should we just return an error here, if it is going to fail anyway later on?

When the topo connection is read-only, which would be e.g. when using --keyspaces_to_watch (see #8988). In that case it's fine to use the cached version because we know that the subsequent ApplyVSchemaDDL() call will fail. I could return an error here rather than make the Apply call. I left it this way to leave the flow (and unit test) unchanged.

I agree that it's a little awkward, so I'm happy to instead return an error about the topo being in read-only mode when vc.topoServer == nil if others prefer that.

This is the current error that users see:

go/test/endtoend/vtgate/keyspace_watches/keyspace_watch_test.go: vschemaDDLError = fmt.Sprintf("Error 1105 (HY000): cannot perform Update on keyspaces/%s/VSchema as the topology server connection is read-only",

I'll see if I can maintain that with an earlier failure here.

@harshit-gangal what do you think? I'll hold off until you have a chance to review. Perhaps the error here doesn't really matter much and I can instead just return:

cannot update the VSchema as the topology server connection is read-only

fwiw, I think this is fine.

We should error out upfront.

Addressed in 60e9de3

GuptaManan100

One change needs to be made, but otherwise looks good to me. Approving so that PR can be merged after the change has been made. ❤️

GuptaManan100 · 2025-01-18T14:06:26Z

go/vt/topo/vschema.go

+// keyspace's vschema.
+type KeyspaceVSchemaInfo struct {
+	Name string
+	*vschemapb.Keyspace
+	version Version
+}


This struct is a duplicate of an existing one in this same package. There exists KeyspaceInfo in keyspace.go. The struct definition is as follows -

// KeyspaceInfo is a meta struct that contains metadata to give the // data more context and convenience. This is the main way we interact // with a keyspace. type KeyspaceInfo struct { keyspace string version Version *topodatapb.Keyspace }

It pretty much has the same information just with a different nomenclature. I think we should remove this and use that struct directly or vice versa.

KeyspaceInfo is for the topo Keyspace record:

vitess/proto/topodata.proto

Lines 259 to 301 in eaaa206

// A Keyspace contains data about a keyspace.

message Keyspace {

// OBSOLETE string sharding_column_name = 1;

reserved 1;

// OBSOLETE KeyspaceIdType sharding_column_type = 2;

reserved 2;

// OBSOLETE int32 split_shard_count = 3;

reserved 3;

// OBSOLETE ServedFrom served_froms = 4;

reserved 4;

// keyspace_type will determine how this keyspace is treated by

// vtgate / vschema. Normal keyspaces are routable by

// any query. Snapshot keyspaces are only accessible

// by explicit addresssing or by calling "use keyspace" first

KeyspaceType keyspace_type = 5;

// base_keyspace is the base keyspace from which a snapshot

// keyspace is created. empty for normal keyspaces

string base_keyspace = 6;

// snapshot_time (in UTC) is a property of snapshot

// keyspaces which tells us what point in time

// the snapshot is of

vttime.Time snapshot_time = 7;

// DurabilityPolicy is the durability policy to be

// used for the keyspace.

string durability_policy = 8;

// ThrottlerConfig has the configuration for the tablet

// server's lag throttler, and applies to the entire

// keyspace, across all shards and tablets.

ThrottlerConfig throttler_config = 9;

// SidecarDBName is the name of the Vitess sidecar database

// used for various system metadata that is stored in each

// tablet's mysqld instance.

string sidecar_db_name = 10;

}

KeyspaceVSchemaInfo is for the keyspace's vschema:

vitess/proto/vschema.proto

Lines 40 to 60 in eaaa206

// Keyspace is the vschema for a keyspace.

message Keyspace {

// If sharded is false, vindexes and tables are ignored.

bool sharded = 1;

map<string, Vindex> vindexes = 2;

map<string, Table> tables = 3;

// If require_explicit_routing is true, vindexes and tables are not added to global routing

bool require_explicit_routing = 4;

// foreign_key_mode dictates how Vitess should handle foreign keys for this keyspace.

ForeignKeyMode foreign_key_mode = 5;

enum ForeignKeyMode {

unspecified = 0;

disallow = 1;

unmanaged = 2;

managed = 3;

}

// multi_tenant_mode specifies that the keyspace is multi-tenant. Currently used during migrations with MoveTables.

MultiTenantSpec multi_tenant_spec = 6;

}

Definitely not the same thing 🙂

GuptaManan100 · 2025-01-18T14:10:03Z

go/test/endtoend/vtgate/vschema/vschema_test.go

+
+// TestVSchemaSQLAPIConcurrency tests that we prevent lost writes when we have
+// concurrent vschema changes being made via the SQL API.
+func TestVSchemaSQLAPIConcurrency(t *testing.T) {


Good test 😍

harshit-gangal · 2025-01-22T03:48:33Z

go/vt/topo/vschema.go

+	version Version
+}
+
+func (k *KeyspaceVSchemaInfo) CloneVT() *KeyspaceVSchemaInfo {


super nit: why did we name this as CloneVT?

I wanted to override the embedded vschemapb.Keyspace CloneVT function. I'm fine renaming it to Clone e.g. though.

harshit-gangal · 2025-01-22T04:02:32Z

go/vt/topotools/vschema_ddl.go

 // keyspace definition and returns the modified keyspace object.
-func ApplyVSchemaDDL(ksName string, ks *vschemapb.Keyspace, alterVschema *sqlparser.AlterVschema) (*vschemapb.Keyspace, error) {
-	if ks == nil {
-		ks = new(vschemapb.Keyspace)
+func ApplyVSchemaDDL(ksName string, ksvs *topo.KeyspaceVSchemaInfo, alterVschema *sqlparser.AlterVschema) (*topo.KeyspaceVSchemaInfo, error) {
+	if ksvs.Tables == nil {
+		ksvs.Tables = map[string]*vschemapb.Table{}
 	}


I believe the GetVSchema should be done inside this method to ensure all the callers of ApplyVSchemaDDL have a common code. Post that they can have a action method of their own which they can pass in.
This will avoid the issue of missing this call outside of ApplyVSchemaDDL

Good idea. I did that here 60e9de3

Signed-off-by: Matt Lord <mattalord@gmail.com>

github-actions bot added this to the v22.0.0 milestone Dec 17, 2024

mattlord force-pushed the vschema_topo_version branch 4 times, most recently from 4b4578d to be1ccd1 Compare December 18, 2024 22:11

Get all e2e tests working

b2e2ba6

Signed-off-by: Matt Lord <mattalord@gmail.com>

mattlord force-pushed the vschema_topo_version branch from be1ccd1 to b2e2ba6 Compare December 19, 2024 22:21

mattlord added 2 commits January 5, 2025 13:00

Merge remote-tracking branch 'origin/main' into vschema_topo_version

1333150

Fixups after merge

d3ebbe7

Signed-off-by: Matt Lord <mattalord@gmail.com>

mattlord force-pushed the vschema_topo_version branch 2 times, most recently from e9319d3 to 1ccc129 Compare January 6, 2025 21:46

Fix unit tests

e02adaf

Signed-off-by: Matt Lord <mattalord@gmail.com>

mattlord force-pushed the vschema_topo_version branch from 1ccc129 to e02adaf Compare January 6, 2025 22:48

mattlord added 3 commits January 13, 2025 14:48

Merge remote-tracking branch 'origin/main' into vschema_topo_version

6398a7f

Signed-off-by: Matt Lord <mattalord@gmail.com>

Fix unrelated unit test

cd455a0

Signed-off-by: Matt Lord <mattalord@gmail.com>

Fix vtgate tests

8d0d520

Signed-off-by: Matt Lord <mattalord@gmail.com>

mattlord added 2 commits January 13, 2025 22:09

Revert "Fix unrelated unit test"

ea76325

This reverts commit cd455a0. Signed-off-by: Matt Lord <mattalord@gmail.com>

Add new test and error message

269d30b

Signed-off-by: Matt Lord <mattalord@gmail.com>

mattlord removed the NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work label Jan 14, 2025

mattlord requested review from GuptaManan100, ajm188, deepthi, frouioui, harshit-gangal, notfelineit, shlomi-noach, systay and timvaillancourt as code owners January 15, 2025 02:42

mattlord removed request for ajm188, notfelineit, rohit-nayak-ps, shlomi-noach and timvaillancourt January 15, 2025 02:42

deepthi requested a review from rohit-nayak-ps January 16, 2025 01:09

Merge remote-tracking branch 'origin/main' into vschema_topo_version

d8c9c53

Signed-off-by: Matt Lord <mattalord@gmail.com>

rohit-nayak-ps approved these changes Jan 17, 2025

View reviewed changes

GuptaManan100 approved these changes Jan 18, 2025

View reviewed changes

harshit-gangal reviewed Jan 22, 2025

View reviewed changes

mattlord added 3 commits January 22, 2025 09:36

Merge remote-tracking branch 'origin/main' into vschema_topo_version

0a59270

Signed-off-by: Matt Lord <mattalord@gmail.com>

Fixes after merging in origin/main

30b7e2f

Signed-off-by: Matt Lord <mattalord@gmail.com>

Move getting current vschema to ApplyVSchemaDDL

60e9de3

Signed-off-by: Matt Lord <mattalord@gmail.com>

harshit-gangal approved these changes Jan 23, 2025

View reviewed changes

Merge remote-tracking branch 'origin/main' into vschema_topo_version

2e008f6

Signed-off-by: Matt Lord <mattalord@gmail.com>

mattlord merged commit a6c2896 into vitessio:main Jan 23, 2025

mattlord deleted the vschema_topo_version branch January 23, 2025 23:53

This was referenced Jan 9, 2026

vschema revert: initialize as nil so that nil checks do not pass later #19114

Merged

Bug Report: Sharded target VSchema cleared by MoveTablesCreate error path #19115

Closed

	// A Keyspace contains data about a keyspace.
	message Keyspace {
	// OBSOLETE string sharding_column_name = 1;
	reserved 1;

	// OBSOLETE KeyspaceIdType sharding_column_type = 2;
	reserved 2;

	// OBSOLETE int32 split_shard_count = 3;
	reserved 3;

	// OBSOLETE ServedFrom served_froms = 4;
	reserved 4;

	// keyspace_type will determine how this keyspace is treated by
	// vtgate / vschema. Normal keyspaces are routable by
	// any query. Snapshot keyspaces are only accessible
	// by explicit addresssing or by calling "use keyspace" first
	KeyspaceType keyspace_type = 5;

	// base_keyspace is the base keyspace from which a snapshot
	// keyspace is created. empty for normal keyspaces
	string base_keyspace = 6;

	// snapshot_time (in UTC) is a property of snapshot
	// keyspaces which tells us what point in time
	// the snapshot is of
	vttime.Time snapshot_time = 7;

	// DurabilityPolicy is the durability policy to be
	// used for the keyspace.
	string durability_policy = 8;

	// ThrottlerConfig has the configuration for the tablet
	// server's lag throttler, and applies to the entire
	// keyspace, across all shards and tablets.
	ThrottlerConfig throttler_config = 9;

	// SidecarDBName is the name of the Vitess sidecar database
	// used for various system metadata that is stored in each
	// tablet's mysqld instance.
	string sidecar_db_name = 10;
	}

	// Keyspace is the vschema for a keyspace.
	message Keyspace {
	// If sharded is false, vindexes and tables are ignored.
	bool sharded = 1;
	map<string, Vindex> vindexes = 2;
	map<string, Table> tables = 3;
	// If require_explicit_routing is true, vindexes and tables are not added to global routing
	bool require_explicit_routing = 4;
	// foreign_key_mode dictates how Vitess should handle foreign keys for this keyspace.
	ForeignKeyMode foreign_key_mode = 5;

	enum ForeignKeyMode {
	unspecified = 0;
	disallow = 1;
	unmanaged = 2;
	managed = 3;
	}

	// multi_tenant_mode specifies that the keyspace is multi-tenant. Currently used during migrations with MoveTables.
	MultiTenantSpec multi_tenant_spec = 6;
	}

Conversation

mattlord commented Dec 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue(s)

Checklist

Uh oh!

vitess-bot bot commented Dec 17, 2024

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

Uh oh!

codecov bot commented Jan 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattlord Jan 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GuptaManan100 left a comment

Choose a reason for hiding this comment

Uh oh!

GuptaManan100 Jan 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GuptaManan100 Jan 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

harshit-gangal Jan 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mattlord commented Dec 17, 2024 •

edited

Loading

codecov bot commented Jan 13, 2025 •

edited

Loading

mattlord Jan 17, 2025 •

edited

Loading

GuptaManan100 Jan 18, 2025 •

edited

Loading

GuptaManan100 Jan 18, 2025 •

edited

Loading

harshit-gangal Jan 22, 2025 •

edited

Loading