vreplication: copy functionality by sougou · Pull Request #4791 · vitessio/vitess

sougou · 2019-04-07T20:16:41Z

This PR is harder to break up into smaller commits. So, I'll just explain in detail what this does. The high level approach is described in #4604.

QueryService adds a new function called VStreamRows. This is the source side function that streams all rows of a table. It does the following:
- Lock the table, obtain gtid position.
- Send initial packet: fields, pk fields and gtid. In this repeatable read mode, the snapshot of the rows will be as of the gtid.
- Unlock the table.
vplayer (on the target side) is split into three parts:
- vreplicator: is the high level object that orchestrates the copy and transition into replication.
- vcopier: goes back and forth between copying and replicating, until all copy is done.
- vplayer: the replication module, also used by the copier to catch up.
PlayerPlan renamed to ReplicatorPlan. This plan is used by the copier and player. The plan can be shared because of the uniformity of the vreplication model. We send the same query for both copying and replicating. The only difference is that the copier applies the rows as if they were inserts, but also in bulk.
TablePlan has to generate additional or different queries for applying:
- A three-part insert for generating the bulk insert statements for copying.
- Add a lastpk member. This contains the values of the last pk columns copied so far, in case of a partial table copy.
- The other Insert, Update and Delete statements are generated differently if lastpk is set. We have to make sure we don't apply changes from rows that haven't been copied yet. This strategy is described in the RFC: VReplication based SplitClone #4604.

dweitzman

Still reading with lots more to go, but sending a few minor comments on some of the files I've looked at so far.

dweitzman · 2019-04-08T23:54:17Z

go/vt/binlog/binlogplayer/binlog_player.go

Nit: in this function, could be nice to assign qr.Rows[0] to a local variable like vrRow or somesuch after checking that qr.RowsAffected == 1

dweitzman · 2019-04-08T23:58:48Z

go/vt/vttablet/grpcqueryservice/server.go

Slightly off topic: would be nice at some point to do the callerid stuff and HandlePanic in some kind of grpc interceptor instead of manually copying it for every function.

This is a good idea. I haven't been a fan of this pattern anyways.

dweitzman · 2019-04-08T23:59:57Z

go/vt/vttablet/grpcqueryservice/server.go

Looks like vterrors.ToGRPC(nil) would do the right thing anyway without an explicit nil check here. I'll leave you to decide whether that makes the code more clear, equally clear and shorter, or less clear.

Done. Fixed in a few more places also.

dweitzman · 2019-04-09T00:04:56Z

go/vt/vttablet/grpctabletconn/conn.go

I can imagine an argument for returning ctx.Err() here. If it's intentional that this method suppressing a canceled or deadline exceeded, seems like something to be clear about in docs

Alternatively (and maybe simpler?) it could return ctx.Err() and the caller could be responsible for interpreting any cancelation error or timeout error.

Nice catch. This function must return error on cancel. Otherwise the caller may think that they received all the rows.

dweitzman · 2019-04-09T00:22:08Z

go/vt/vttablet/tabletmanager/vreplication/replicator_plan.go

To my taste, at least, adding comments to a bunch of the functions in this file could make it easier to grok for people without previous context on what's going on.

Added comments in a few plances. Let me know if any other place needs clarification.

dweitzman · 2019-04-09T05:32:49Z

go/vt/vttablet/tabletmanager/vreplication/replicator_plan.go

The use of "a_" vs "b_" bind variable names for the before vs after values is slick, but wasn't immediately clear to me. That might be something to mention explicitly in the comment for TablePlan

dweitzman · 2019-04-09T05:41:33Z

go/vt/vttablet/tabletmanager/vreplication/replicator_plan.go

Observation: it's not immediately obvious to me why a table plan can't assume Delete is present but can assume that Insert is.

I've added a comment to TablePlan that explains why.

dweitzman · 2019-04-09T05:55:20Z

go/vt/vttablet/tabletmanager/vreplication/replicator_plan.go

Might help to call these BulkInsertFront and BulkInsertValues for clarity. I think I have another comment that suggests adding an overall comment for this struct with more explanation about the bind variables and such.

dweitzman · 2019-04-09T05:56:52Z

go/vt/vttablet/tabletmanager/vreplication/replicator_plan.go

Could this use InsertFront and InsertValues so that a separate Insert wouldn't need to exist?

Insert is different from the three-part insert:
The three-part insert is used by the copy operation for generating the bulk insert statement, and does not have conditionals.
The individual Insert is used by the player for replicating rows. It will have the pk < :pk conditional in the insert.
After the copy is finished, the player's insert reverts to the non-conditional insert, just like the three-part insert would. However, the three-part insert becomes obsolete by then.

So, for readability, I decided not to overload these variables. Copy always uses the three-part insert, and player always uses the single Insert.

dweitzman

Still reading. A few more minor comments

dweitzman · 2019-04-17T16:16:00Z

go/vt/vttablet/tabletmanager/vreplication/vcopier.go

Random observation: although these db queries are likely to be fast in practice, someday it would be nice if the context propagated to help with timing out / aborting vreplication-related db access.

dweitzman · 2019-04-17T16:23:13Z

go/vt/vttablet/tabletmanager/vreplication/vcopier.go

Would this channel read potentially block if the vplayer has already sent something? Should this be:

select { case <-errch: default: }

This part looks right to me:

Upon termination, vplayer is guaranteed to send one and only one message on the channel.

There are two places (here and a few lines below) where we fetch from this channel. So, one of those is guaranteed to receive.

However, I see that we return without waiting on errch if we detect that the context was canceled. I think it's safer to wait; If vplayer gets stuck somewhere, the next invocation will race with it.

I'll make this change.

dweitzman · 2019-04-17T16:27:03Z

go/vt/vttablet/tabletmanager/vreplication/vcopier.go

I'm a bit nervous about whether there could be some error case where SecondsBehindMaster gets set to 0 and vplayer keeps erroring out without updating SecondsBehindMaster, which could appear to this code as if vplayer ran successfully and there's no delay.

The legacy binlog player can do that kind of thing (failing to update seconds behind master when it's in an error state)

I was worried about this also, which is why I did this PR first, to track this more accurately: #4637.
But it's something we have to keep an eye one. There could be other corner cases.

dweitzman · 2019-04-17T16:33:59Z

go/vt/vttablet/tabletmanager/vreplication/vcopier.go

Should this explicitly verify that ctx.Err() is DeadlineExceeded before suppressing the error?

dweitzman · 2019-04-17T16:53:15Z

go/vt/vttablet/tabletmanager/vreplication/vcopier.go

It's not immediately clear to me as a reader what factors influence the number of rows that might be included here / the size of the query / the size of the transaction.

Added this comment:

// The number of rows we receive depends on the packet size set // for the row streamer. Since the packet size is roughly equivalent // to data size, this should map to a uniform amount of pages affected // per statement. A packet size of 30K will roughly translate to 8 // mysql pages of 4K each.

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

This refactor changes the insert format into the traditional 'values' syntax, which is reusable for bulk inserts during copying. We also generate a broken-up version of the insert to allow for those sub-parts to be composed into a bulk-insert statement. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

Also, Error state is obsolete. So, we're not using it any more. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

deepthi · 2019-04-17T18:45:45Z

go/vt/vttablet/tabletmanager/vreplication/controller.go

 		return player.ApplyBinlogEvents(ctx)
 	case ct.source.Filter != nil:
-		// VPlayer requires the timezone to be UTC.
+		// vreplicator requires the timezone to be UTC.


I assume this is so that we have a uniform way of measuring lag? If so, can you make that explicit in the comment?

It's because timestamps are always sent as UTC. Added a comment.

deepthi · 2019-04-17T20:48:30Z

go/vt/vttablet/tabletmanager/vreplication/replicator_plan.go

+	return tplan, nil
+}
+
+// buildFromFiedls builds a full TablePlan, but uses the field info as the


typo - buildFromFields

deepthi · 2019-04-17T21:00:42Z

go/vt/vttablet/tabletmanager/vreplication/table_plan_builder.go

 				}
 				plan.TargetTables[tableName] = tablePlan
 				plan.TablePlans[tableName] = tablePlan
+				continue nextTable


Was not having this line of code a bug/inefficiency in the previous version of code?

It was a minor mismatch from the spec I had in mind. Without the continue, the last rule would have matched. Instead, I wanted to stop at the first match. We should clarify this when we write the spec.

deepthi · 2019-04-17T21:23:02Z

go/vt/vttablet/tabletmanager/vreplication/table_plan_builder.go


-func (tpb *tablePlanBuilder) generateUpdate(buf *sqlparser.TrackedBuffer, bvf *bindvarFormatter, before, after bool) {
+func (tpb *tablePlanBuilder) generateOnDupPart(buf *sqlparser.TrackedBuffer) *sqlparser.ParsedQuery {
+	if tpb.onInsert != insertOndup {


can this enum value be renamed to insertOnDup for readability?

deepthi · 2019-04-17T21:34:12Z

go/vt/vttablet/tabletmanager/vreplication/table_plan_builder.go

-	case insertIgnore:
-		return nil
+func (tpb *tablePlanBuilder) generateValuesPart(buf *sqlparser.TrackedBuffer, bvf *bindvarFormatter) *sqlparser.ParsedQuery {
+	bvf.mode = bvAfter


A comment here will be helpful, something to the effect that setting the mode will control how Myprintf format's the bind variable.

Since these are set in multiple places, I've added a detailed comment on the bindvarFormatter type.

deepthi · 2019-04-17T21:35:07Z

go/vt/vttablet/tabletmanager/vreplication/table_plan_builder.go

-			if before {
-				bvf.mode = bvBefore
-				buf.Myprintf("-ifnull(%v, 0)", cexpr.expr)
+			bvf.mode = bvBefore


Comment here? On updates we want to subtract the old value and add the new value to the sum.

deepthi · 2019-04-18T21:49:21Z

go/vt/vttablet/tabletmanager/vreplication/table_plan_builder.go

 type bindvarMode int

 const (
-	bvNone = bindvarMode(iota)


+1 for removing this.

deepthi · 2019-04-18T23:44:11Z

go/vt/vttablet/tabletmanager/vreplication/vcopier.go

+	if err != nil {
+		return err
+	}
+	if settings.StartPos.IsZero() {


Cannot catchup, must copy first?

Added clarification:

// If there's no start position, it means we're copying the // first table. So, there's nothing to catch up to.

sougou

Comments addressed.

sougou · 2019-04-19T22:54:33Z

go/vt/vttablet/tabletmanager/vreplication/controller.go

 		return player.ApplyBinlogEvents(ctx)
 	case ct.source.Filter != nil:
-		// VPlayer requires the timezone to be UTC.
+		// vreplicator requires the timezone to be UTC.


It's because timestamps are always sent as UTC. Added a comment.

sougou · 2019-04-19T22:57:46Z

go/vt/vttablet/tabletmanager/vreplication/table_plan_builder.go

 				}
 				plan.TargetTables[tableName] = tablePlan
 				plan.TablePlans[tableName] = tablePlan
+				continue nextTable


It was a minor mismatch from the spec I had in mind. Without the continue, the last rule would have matched. Instead, I wanted to stop at the first match. We should clarify this when we write the spec.

sougou · 2019-04-19T23:02:44Z

go/vt/vttablet/tabletmanager/vreplication/table_plan_builder.go

-	case insertIgnore:
-		return nil
+func (tpb *tablePlanBuilder) generateValuesPart(buf *sqlparser.TrackedBuffer, bvf *bindvarFormatter) *sqlparser.ParsedQuery {
+	bvf.mode = bvAfter


Since these are set in multiple places, I've added a detailed comment on the bindvarFormatter type.

sougou · 2019-04-19T23:04:41Z

go/vt/vttablet/tabletmanager/vreplication/vcopier.go

+	if err != nil {
+		return err
+	}
+	if settings.StartPos.IsZero() {


Added clarification:

// If there's no start position, it means we're copying the // first table. So, there's nothing to catch up to.

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

deepthi

LGTM

@demmer

It's been busy and exciting in vitess upstream. Some changes that felt worth calling out as they could impact assumptions about behavior: - vitessio#4832 — this adds a timeout that is 3x the `healthCheckInterval` which at which point the tablet will stop reporting as healthy - vitessio#4814 — this changes when command line args are used by vttablet - vitessio#4811 — a planned reparent now sets `super_read_only` - vitessio#4805 — behavior coming back from a failed backup is different **Slack changes** @demmer vitessio#4827 Correct suppress logging for begin...commit in autocommit @rafael vitessio#4824 adds timeouts for all statements @demmer vitessio#4826 adds logging of stack traces as opt-in @demmer vitessio#4819 remove begin/commit logs from autocommit txns @demmer vitessio#4796 improves support for vtexplain for begin/dml/dml/commit txns **Non slack changes** vitessio#4839 Improve behavior with reference table routing & vreplication vitessio#4833 Support query routing given there could be multiple targets for a table vitessio#4832 tablets get new health check behavior (health checks time out) vitessio#4837 refresh bug around row streamer handling creds vitessio#4830 apply a default value to the db_name attribute in local_metadata vitessio#4785 SHOW SCHEMAS aliased to SHOW DATABASES vitessio#4829 add experimental support for split clone & vertical split clone via vreplication vitessio#4822 do not normalize in order by vitessio#4791 Adds vreplication row streaming support vitessio#4814 command line flags now used regardless of management mode vitessio#4811 vttablet sets super_read_only during planned reparent vitessio#4803 xtrabackup testing vitessio#4727 support multiple vttablets running against one mysql instance vitessio#4746 TopoCat can now produce JSON output vitessio#4805 mysqld is now restarted after a failed backup vitessio#4685 macos bootstrap bug vitessio#4874 ZK opts and java land vitessio#4695 Adds support for xtrabackup vitessio#4794 fixes build failures vitessio#4725 changes mysql8.0 start args vitessio#4736 introduce new states to support vreplication vitessio#4788 expands orc error **Docs etc** vitessio#4831 lint vitessio#4827 docs vitessio#4816 const declaration cleanup vitessio#4820 const declaration cleanup vitessio#4825 docs vitessio#4818 docs vitessio#4809 docs vitessio#4812 moves consts around vitessio#4813 docs vitessio#4808 docs vitessio#4800 docs vitessio#4795 docs

dweitzman reviewed Apr 9, 2019

View reviewed changes

dweitzman reviewed Apr 17, 2019

View reviewed changes

sougou added 20 commits April 17, 2019 11:04

vstreamer: VStreamRows rpc method

1d2d111

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vstreamer: rowStreamer for source tablet

3d30558

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vplayer: minor tweaks

d3d870d

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vplayer: copyTables initial cut

c096533

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vplayer: copyTables: basic functionality working

de5a2e3

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vplayer: split: vreplicator, vplayer, vcopier

bfddb58

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vplayer: improve naming of some types & vars

9692bf9

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vplayer: handle lastpk based replication

ec25345

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vplayer: don't blindly update state to 'Running'

231c7de

Also, Error state is obsolete. So, we're not using it any more. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vplayer: prep for integrating vcopier and vplayer

91bdf93

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vstreamer: rowstreamer locks the table

856a196

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vplayer: more robust position tracking

9b49699

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vplayer: copier player dance

9c15bbe

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vplayer: test copy and fix bugs

e7e2b5f

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vcopier: more tests and a few tweaks

11a8b30

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vstreamer: delete obsolete test

4636888

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vreplication: address review comments

1b8c596

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vreplication: address more review comments

1fba59e

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vreplication: fix after rebase

b808777

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

sougou force-pushed the ss-vrepl branch from ea87931 to b808777 Compare April 17, 2019 18:30

deepthi reviewed Apr 17, 2019

View reviewed changes

deepthi reviewed Apr 18, 2019

View reviewed changes

sougou commented Apr 19, 2019

View reviewed changes

vreplication: address review comments

8e63c81

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

sougou force-pushed the ss-vrepl branch from 8173eb2 to 8e63c81 Compare April 20, 2019 02:11

deepthi approved these changes Apr 20, 2019

View reviewed changes

sougou merged commit 6b34d20 into vitessio:master Apr 21, 2019

sougou deleted the ss-vrepl branch April 23, 2019 02:58

setassociative mentioned this pull request Apr 26, 2019

Merge upstream 2019-04-25 tinyspeck/vitess#130

Merged

Conversation

sougou commented Apr 7, 2019

Uh oh!

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

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!

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!

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!

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

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dweitzman Apr 17, 2019 • 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!

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!

Choose a reason for hiding this comment

dweitzman Apr 17, 2019 •

edited

Loading