v3: migration support for lookup vindexes by sougou · Pull Request #3600 · vitessio/vitess

sougou · 2018-01-28T15:28:54Z

Commits:

change the vindex protocol allowing a non-unique vindex to return either a keyrange or keyspace ids.
change lookup vindexes to support a new flag: "scatter_if_absent" that will cause it to return the full keyrange if an entry is not found. This behavior is changed in a newer commit.
change the vindex protocol and requires Lookup vindexes to supply the Update function. This allow them to be more efficient, and some vindexes can choose to return an error if they cannot support it.
add "autocommit" flag for lookup vindexes. Please see the commit comments for the associated behavior changes.
change "scatter_if_absent" to "write_only".

sougou · 2018-01-28T15:31:45Z

@tirsen: This is phase 1, which is the hardest part of the feature.
The subsequent ones are:

Create to be its own transaction.
Option to upsert on Create.
Disallow updates for such vindexes.

rafael

Should we add one test to show that the index is not a scatter when the rows are in the lookup table?

This is looking good to me. Added some small comments.

rafael · 2018-01-28T19:23:52Z

go/vt/srvtopo/resolve.go

 	uniqueShards := make(map[string]bool)
 	for _, kr := range krs {
-		ResolveKeyRangeToShards(allShards, uniqueShards, kr)
+		keyRangeToShardMap(allShards, uniqueShards, kr)


Maybe we can remove keyRangeToShardMap altogether and do something like:

func MapKeyRangesToShards(ctx context.Context, topoServ Server, cell, keyspace string, tabletType topodatapb.TabletType, krs []*topodatapb.KeyRange) (string, []string, error) { keyspace, _, allShards, err := GetKeyspaceShards(ctx, topoServ, cell, keyspace, tabletType) if err != nil { return "", nil, err } var res []string for _, kr := range krs { res = append(res, GetShardsForKeyRange(allShards, kr)...) } return keyspace, res, nil }

I was about to do it. Then realized that we can eventually deprecate this (all V2). So, left it mostly untouched.

rafael · 2018-01-28T20:34:00Z

go/vt/vtgate/vindexes/vindex.go


+// Ksids represents keyspace ids. It's either a list of keyspace ids
+// or a keyrange.
+type Ksids struct {


I was thinking if a different approach could be possible here. It seems like this struct is behaving like an algebraic data type, where later on there is alway a check to see if it's being used as a Range or as IDs.

I wonder if there is a way to encode this as methods in the interface, in that way the behavior will be more explicit and clearer. I went down this exercise and it's not trivial, but doesn't seem undoable at first glance.

I think the best abstraction would be for this struct to compute the list of shards, or a wrapper that did the same. However, the shards get added directly to another data structure (while iterating on IDs). The abstraction would have caused an extra allocation. So, I decided to keep the existing (and common) code paths efficient.

tirsen · 2018-01-29T08:49:20Z

Remind me why upsert on create is needed?

sougou · 2018-01-29T12:50:30Z

upsert_on_create is for this use case:

You're running in autocommit mode.
You insert a row: Vindex insert succeeds but the actual insert fails.
You attempt to insert the row again, but the Vindex will return a dup key error because the entry already exists.

With upsert_on_insert turned on, it will just replace the existing entry to match the new insert.

The downside:
You lose the ability to detect dup rows: If you had previously successfully completed an insert, then a new insert will replace the old vindex row making the previous row unreachable.

In other words, upsert_on_insert should be used only if you have other ways to guarantee uniqueness, and you want the ability to re-insert a previously failed insert.

demmer · 2018-01-29T14:34:04Z

go/vt/vtgate/engine/primitive.go

 	// Context returns the context of the current request.
 	Context() context.Context
+
+	// V3 functions.


For the newcomer to the code it would be great to provide some more context on what "V3" means -- something like "Routing functions used by Vindex operations" would be clearer than "V3".

Perhaps you eventually plan to have the VCursor used in other contexts, but AFAIK Execute (and now ExecuteAutocommit) are only used by the vindex implementations so having this be clear in the interface would definitely help those of us who have tried to wade through what's going on here.

demmer · 2018-01-29T14:39:45Z

Some comments/questions more about the overall approach than the code specifics:

As discussed in Slack, the scatter_if_absent flag has a potential correctness issue for non-unique lookup vindexes during a backfill migration since it might get a partial but incomplete result set from the lookup table query. In other words, suppose a given non-unique vindex eventually will have two entries for a given column value, but in the middle of the migration a read operation is applied when only one of the rows has been written to the backing table. This wouldn't fall back to scatter since it's not an empty set, but it still isn't complete.

It seems to me the only way to handle a transitional non-unique lookup vindex is to always scatter on read until you are sure the vindex is fully populated. Note that the current behavior seems correct for unique lookup vindexes.

I'd love to understand the motivation for running vindex operations in their own transactions - although I think I like this in general, but the complexities for autocommit are non-trivial as you point out here, and I haven't yet reasoned through how to properly sequence operations like secondary vindex updates or even keyspace id changes as mentioned in v3: support update for a primary vindex column #3596.
Speaking of which, can you motivate why we want this flag to autocommit on insert? Is it performance driven? If so, one thing I've been thinking of adding is support to parse directives out of query comments, e.g. INSERT /* multi-shard-autocommit=true */ ... would tell vitess to override the current logic and auto commit each row in a multi-row insert. This would of course violate ACID semantics but would have a performance benefit.

tirsen · 2018-01-30T06:23:53Z

go/vt/vtgate/executor_dml_test.go

It seems weird to use a string boolean value "true" when json supports boolean natively...

tirsen · 2018-01-30T06:34:41Z

@demmer

"Autocommit" is for "transactional" integrity. That means we can't have a committed transaction that inserts a new row with a "lookup vindexed column" without the lookup table being updated since the lookup table insert has to commit before the transaction that inserts the vindexed column. It's a crucial part of "productionizing" vindexes. More details in my final comment here: #3537

Regarding atomicity of non-unique lookup tables:

I think if you need atomicity during the backfill you have to insert all the lookup rows for a given key in the same transaction. That can be accomplished using a map/reduce style backfill (where we shuffle to the target column value and then insert all of those in one transaction). We will backfill in our application code for now but I think this is how Vitess native backfills have to work to ensure correctness. I think Sugu has been toying with the idea of a Vitess native MR implementation, this may be a good first use case.

I think we can also very easily add a "silent vindex" option where we simply keep the lookup table up to date but don't use it for routing. This seems like a useful option if you know your backfill isn't atomic.

googlebot · 2018-02-10T01:24:13Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot. The email used to register you as an authorized contributor must be the email used for the Git commit.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

sougou · 2018-02-10T01:29:04Z

I signed it!

googlebot · 2018-02-10T01:29:07Z

CLAs look good, thanks!

sougou · 2018-02-10T01:36:35Z

@demmer @rafael @tirsen this is now updated to match #3642.

derekperkins · 2018-02-10T01:55:30Z

@sougou Congrats on your first PR to the Vitess project!

This change allows non-unique vindexes to return a keyrange instead of a list of keyspace ids. Initially, this feature will be used for implementing a migration mode vindex. When we add a vindex to a column, there are no values for existing rows, and entries for those must be backfilled. Until the backfill happens, a vindex could return the full keyrange asking the engine to perform a full scatter. Once the backfill is done, the vindex can be switched to a more strict one. In the future, this approach can also be used for more sophisticated vindexes for mapping inequalities to keyranges.

Change non-unique lookup vindexes to return a full keyrange if no match is found. This mode can be used when adding a new vindex to a column. With this flag turned on new entries will use the lookup while old ones will result in a scatter. Once the backfill is done, the flag can be turned off.

With the Update function now part of Lookup, such vindexes can now find more efficient implementations than the previous Delete and Create sequence. Currently, the Update implementations just do the Delete and Create, but we'll soon improve those. This abstraction also allows some vindexes to reject Updates if it's not supported. While making changes, I found a couple of bugs that I fixed: * updateChangedVindexes fromIds computation was incorrect. It would have fetched incorrect values if the table had multiple multi-column and owned lookup vindexes. * The Values for changing columns in planbuilder was getting filled out in the SET order, and not in the column order of the vindex, which was causing the updates to set the wrong values. The above are not practical use cases, but it's better to strive for correctness.

Added autocommit support for lookup vindex. In this mode, as described in #3642, inserts are executed as upserts, deletes are ignored, and updates are disallowed.

scatterIfAbsent was an unsafe flag because it could return incorrect results if a lookup vindex was partially populated. The only safe behavior is to always return a full scatter until backfill is fully completed. The flag is accordingly renamed to write_only

rafael · 2018-01-28T23:54:25Z

go/vt/vtgate/planbuilder/dml.go

 	changedVindexes := make(map[string][]sqltypes.PlanValue)
 	for i, vindex := range colVindexes {
-		for _, assignment := range update.Exprs {
-			for _, vcol := range vindex.Columns {


oh good catch.

rafael · 2018-02-13T18:16:48Z

go/vt/vtgate/vindexes/lookup.go

Shouldn't be something like? When present, queries will be sent to all shards (i.e the lookup table won't be used).

This part if an entry is missing doesn't seem to reflect what the code is doing.

Good catch. Done.

rafael · 2018-02-13T19:24:06Z

go/vt/vtgate/vindexes/lookup_internal.go

I think we should add a comment here, explaining why we are doing this.

The new behavior for autocommit has been updated to reflect comments in #3642: * We never delete * All db operations use autocommit to prevent deadlocks * Non-unique vindexes upsert * Unique vindexes always insert * Updates are allowed as long as the underlying operations succeed

rafael · 2018-02-13T21:31:03Z

I went couple times through this. LGTM. Looking forward to having this throughly documented in vitess.io. It will be tricky to reason about all these different behaviors.

alainjobart · 2018-02-14T15:38:32Z

LGTM

googlebot added the cla: yes label Jan 28, 2018

sougou requested review from demmer and rafael January 28, 2018 15:29

rafael reviewed Jan 28, 2018

View reviewed changes

demmer reviewed Jan 29, 2018

View reviewed changes

tirsen reviewed Jan 30, 2018

View reviewed changes

sougou mentioned this pull request Feb 8, 2018

Transactionally resilient lookup vindexes #3642

Closed

googlebot added cla: no and removed cla: yes labels Feb 10, 2018

googlebot added cla: yes and removed cla: no labels Feb 10, 2018

sougou added 5 commits February 13, 2018 08:21

v3: support for Lookup vindex autocommit

64938d0

Added autocommit support for lookup vindex. In this mode, as described in #3642, inserts are executed as upserts, deletes are ignored, and updates are disallowed.

rafael reviewed Feb 13, 2018

View reviewed changes

alainjobart merged commit f615181 into vitessio:master Feb 14, 2018

sougou mentioned this pull request Feb 15, 2018

v3: write_only support for unique lookup vindexes #3658

Merged

Conversation

sougou commented Jan 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sougou commented Jan 28, 2018

Uh oh!

rafael left a comment

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!

tirsen commented Jan 29, 2018

Uh oh!

sougou commented Jan 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

demmer commented Jan 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tirsen commented Jan 30, 2018

Uh oh!

googlebot commented Feb 10, 2018

Uh oh!

sougou commented Feb 10, 2018

Uh oh!

googlebot commented Feb 10, 2018

Uh oh!

sougou commented Feb 10, 2018

Uh oh!

derekperkins commented Feb 10, 2018

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!

rafael commented Feb 13, 2018

Uh oh!

alainjobart commented Feb 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

sougou commented Jan 28, 2018 •

edited

Loading

alainjobart commented Feb 14, 2018 •

edited

Loading