Vtctl Materialize optimizations by teejae · Pull Request #6207 · vitessio/vitess

teejae · 2020-05-20T21:21:17Z

Only get source and target schemas once, saving network roundtrips and O(n) -> O(1) performance
Allow empty source entries to be equivalent to select * from table, as used by VReplication
Change variables for better readability

teejae · 2020-05-20T21:24:49Z

read PR w/ hide whitespace changes, as there's a few places that have only been indented.

Signed-off-by: Toliver Jue <toliver@planetscale.com>

rohit-nayak-ps

Other than the issue I commented about LGTM.

Nice, this should help greatly once we add --all to MoveTables as well!

rohit-nayak-ps · 2020-05-21T19:13:56Z

go/vt/wrangler/materializer_test.go


 	err := env.wr.Materialize(context.Background(), ms)
-	assert.EqualError(t, err, "source and target table names must match for copying schema: t2 vs t1")
+	assert.EqualError(t, err, "copy: source tables do not exist: [t1].")


It is the target table that is missing, right? We are copying over "select * from t2" from source into t1 on target ... Validations have to take into account that we could have tables missing in the source or tables missing in the target where we are materializing to a different table ...

I've split the test now into 2 cases, "copy" and not. "copy" still requires the same table name, and thus this is right error.

the other "non copy" case is a separate test, where CreateDdl="", and has the same original error.

rohit-nayak-ps · 2020-05-21T20:06:51Z

go/vt/wrangler/materializer.go

+
+			if ts.CreateDdl == createDDLAsCopy {
+				needsCopy = true
+				copyTables[ts.TargetTable] = true


Target table and source table can be different, so the table to be copied will have to use sqlparser.TableFromStatement(ts.SourceExpression) if the source expression is present ...

see other comment

Signed-off-by: Toliver Jue <toliver@planetscale.com>

teejae · 2020-05-22T09:08:00Z

@rohit-nayak-ps ptal. adjusted only the tests. the logic was still right, but the tests were not sufficiently granular previously.

sougou

This feels a little over-complicated. Maybe you were not aware that you could GetSchema for all tables using []string{"/.*/"}.
What I would do is a full GetSchema for target outside the table loop.
For the source, I'd lazy-load a full GetSchema of source on first use, or even just load it upfront to keep it simple.
Then the existing code would be practically unchanged except for the additional logic for SourceExpression.

teejae · 2020-05-24T05:34:21Z

@sougou, I had wanted to keep GetSchema to only grabbing the tables we needed, hence didn't retrieve all of them (in case of 10k+ table schemas).

So are you suggesting what I should change in this PR is just grabbing the whole schemas without making the sub table lists?

The parsing of the table lists into maps would still be necessary, since we need to otherwise index on them.

teejae · 2020-05-24T05:48:20Z

@sougou I've made the changes to grab and index the full schemas before going through the table loop. PTAL.

Signed-off-by: Toliver Jue <toliver@planetscale.com>

sougou

I was hoping it would be simpler, but the fact that the schema returned is not a map complicates this code a bit more.

We need a test to demonstrate the new behavior. After this, we're good to go.

sougou · 2020-05-24T16:37:11Z

go/vt/wrangler/materializer.go

-			if mz.targetVSchema.Keyspace.Sharded && mz.targetVSchema.Tables[ts.TargetTable].Type != vindexes.TypeReference {
-				cv, err := vindexes.FindBestColVindex(mz.targetVSchema.Tables[ts.TargetTable])
+
+			if ts.SourceExpression != "" {


Change this to:

if ts.SourceExpression == "" { continue }

but don't i still need to add to the Filter.Rules after this block?

Oh. Right. Missed that. You can duplicate that append line here before continuing.
Main concern is that there are already too many indents. I'm not happy about the existing if-else block already.

duped. also i pulled out the filter as default assign so that we can get rid of an else as well.

go/vt/wrangler/materializer_env_test.go

go/vt/wrangler/materializer.go

Signed-off-by: Toliver Jue <toliver@planetscale.com>

teejae · 2020-05-24T18:25:46Z

@sougou also what test are you proposing for this new behavior? that we don't get multiple requests for GetSchema in particular?

sougou · 2020-05-24T18:43:20Z

@sougou also what test are you proposing for this new behavior? that we don't get multiple requests for GetSchema in particular?

Mainly one where the source expression is empty.

Signed-off-by: Toliver Jue <toliver@planetscale.com>

sougou · 2020-05-24T20:26:38Z

go/vt/wrangler/materializer.go

+				filter = sqlparser.String(sel)
 			}
+
+			rule.Filter = filter


comments addressed.

teejae requested a review from sougou as a code owner May 20, 2020 21:21

teejae force-pushed the tj-materialize-optimization branch 2 times, most recently from 12f6bcb to b217f5b Compare May 21, 2020 11:17

Vtctl Materialize optimizations

368ccd3

Signed-off-by: Toliver Jue <toliver@planetscale.com>

teejae force-pushed the tj-materialize-optimization branch from b217f5b to 368ccd3 Compare May 21, 2020 11:40

rohit-nayak-ps previously requested changes May 21, 2020

View reviewed changes

split materializer test for copy/non-copy

5f4f7f7

Signed-off-by: Toliver Jue <toliver@planetscale.com>

teejae requested a review from rohit-nayak-ps May 22, 2020 09:07

sougou reviewed May 24, 2020

View reviewed changes

teejae force-pushed the tj-materialize-optimization branch 4 times, most recently from c7564b7 to b33f629 Compare May 24, 2020 07:51

teejae marked this pull request as draft May 24, 2020 10:11

teejae marked this pull request as ready for review May 24, 2020 10:11

always grab full schema from source and target

9171b55

Signed-off-by: Toliver Jue <toliver@planetscale.com>

teejae force-pushed the tj-materialize-optimization branch from b33f629 to 9171b55 Compare May 24, 2020 10:13

merge

cb09316

Signed-off-by: Toliver Jue <toliver@planetscale.com>

sougou reviewed May 24, 2020

View reviewed changes

teejae requested a review from sougou May 24, 2020 17:12

unindent, remove else

cc40ce7

Signed-off-by: Toliver Jue <toliver@planetscale.com>

empty source expression test

ccdcb63

Signed-off-by: Toliver Jue <toliver@planetscale.com>

teejae force-pushed the tj-materialize-optimization branch from 8a9c6d2 to ccdcb63 Compare May 24, 2020 19:04

sougou approved these changes May 24, 2020

View reviewed changes

go/vt/wrangler/materializer.go

filter = sqlparser.String(sel)

}

rule.Filter = filter

Copy link
Copy Markdown

Contributor

sougou May 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice.

sougou merged commit 2bbe822 into vitessio:master May 24, 2020

deepthi added this to the v7.0 milestone Jul 27, 2020

ameetkotian mentioned this pull request Aug 20, 2020

Slack vitess 2020.08.19.r0 tinyspeck/vitess#180

Merged

rohit-nayak-ps mentioned this pull request Aug 21, 2020

Materialize: Only get schema from source tablets if target is missing tables #6601

Merged

Conversation

teejae commented May 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

teejae commented May 20, 2020

Uh oh!

rohit-nayak-ps 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!

teejae commented May 22, 2020

Uh oh!

sougou left a comment

Choose a reason for hiding this comment

Uh oh!

teejae commented May 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

teejae commented May 24, 2020

Uh oh!

sougou 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!

teejae May 24, 2020 • 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!

Uh oh!

Uh oh!

teejae commented May 24, 2020

Uh oh!

sougou commented May 24, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

teejae commented May 20, 2020 •

edited

Loading

teejae commented May 24, 2020 •

edited

Loading

teejae May 24, 2020 •

edited

Loading