Add linearizable support to SQL VSchema management #17401

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 <[email protected]>

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

Codecov Report

Attention: Patch coverage is 75.00000% with 65 lines in your changes missing coverage. Please review.

Project coverage is 67.70%. Comparing base (71ccd6d) to head (d8c9c53).

Files with missing lines	Patch %	Lines
go/vt/vtctl/vtctl.go	31.25%	22 Missing ⚠️
go/vt/vtgate/executorcontext/vcursor_impl.go	60.00%	8 Missing ⚠️
go/cmd/vtcombo/cli/vschema_watcher.go	0.00%	7 Missing ⚠️
go/vt/topotools/vschema_ddl.go	86.04%	6 Missing ⚠️
go/vt/vtcombo/tablet_map.go	0.00%	5 Missing ⚠️
go/vt/topo/vschema.go	90.00%	4 Missing ⚠️
go/vt/vtctl/grpcvtctldserver/server.go	85.71%	4 Missing ⚠️
go/vt/vtgate/vschema_manager.go	55.55%	4 Missing ⚠️
go/vt/topo/helpers/copy.go	33.33%	2 Missing ⚠️
go/test/utils/diff.go	0.00%	1 Missing ⚠️
... and 2 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #17401      +/-   ##
==========================================
- Coverage   67.71%   67.70%   -0.02%     
==========================================
  Files        1584     1584              
  Lines      254721   254799      +78     
==========================================
+ Hits       172473   172499      +26     
- Misses      82248    82300      +52

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

This reverts commit cd455a0. Signed-off-by: Matt Lord <[email protected]>

Signed-off-by: Matt Lord <[email protected]>

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

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 😍

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

Fix unrelated unit test

cd455a0

Signed-off-by: Matt Lord <[email protected]>

Fix vtgate tests

8d0d520

Signed-off-by: Matt Lord <[email protected]>

mattlord added 2 commits January 13, 2025 22:09

Revert "Fix unrelated unit test"

ea76325

This reverts commit cd455a0. Signed-off-by: Matt Lord <[email protected]>

Add new test and error message

269d30b

Signed-off-by: Matt Lord <[email protected]>

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

Changes from self review

0d3cc67

Signed-off-by: Matt Lord <[email protected]>

mattlord force-pushed the vschema_topo_version branch 6 times, most recently from e1122ea to 1e3a496 Compare January 15, 2025 00:34

Fix test when run with others in the package

f5c6c63

Signed-off-by: Matt Lord <[email protected]>

mattlord force-pushed the vschema_topo_version branch from 1e3a496 to f5c6c63 Compare January 15, 2025 01:29

Nits

9a19131

Signed-off-by: Matt Lord <[email protected]>

mattlord marked this pull request as ready for review January 15, 2025 02:42

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

mattlord removed request for shlomi-noach, timvaillancourt, ajm188, notfelineit and rohit-nayak-ps 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 <[email protected]>

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

View reviewed changes

GuptaManan100 approved these changes Jan 18, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add linearizable support to SQL VSchema management #17401

Add linearizable support to SQL VSchema management #17401

mattlord commented Dec 17, 2024 •

edited

Loading

vitess-bot bot commented Dec 17, 2024

codecov bot commented Jan 13, 2025 •

edited

Loading

rohit-nayak-ps Jan 17, 2025

mattlord Jan 17, 2025 •

edited

Loading

mattlord Jan 17, 2025

GuptaManan100 left a comment

GuptaManan100 Jan 18, 2025 •

edited

Loading

mattlord Jan 18, 2025

GuptaManan100 Jan 18, 2025 •

edited

Loading

	// 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;
	}

Add linearizable support to SQL VSchema management #17401

Are you sure you want to change the base?

Add linearizable support to SQL VSchema management #17401

Conversation

mattlord commented Dec 17, 2024 • edited Loading

Description

Related Issue(s)

Checklist

vitess-bot bot commented Dec 17, 2024

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

codecov bot commented Jan 13, 2025 • edited Loading

Codecov Report

rohit-nayak-ps Jan 17, 2025

Choose a reason for hiding this comment

mattlord Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

mattlord Jan 17, 2025

Choose a reason for hiding this comment

GuptaManan100 left a comment

Choose a reason for hiding this comment

GuptaManan100 Jan 18, 2025 • edited Loading

Choose a reason for hiding this comment

mattlord Jan 18, 2025

Choose a reason for hiding this comment

GuptaManan100 Jan 18, 2025 • edited Loading

Choose a reason for hiding this comment

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