VReplication: Pad binlog values for binary() columns to match the value returned by mysql selects #7969

rohit-nayak-ps · 2021-04-27T16:55:00Z

Signed-off-by: Rohit Nayak [email protected]

Description

For fixed length binary columns, mysql internally pads the values on the right will nulls. However the binlogs contain the original value. This leads to a problem in vreplication workflows if a binary column is used as a sharding key.

During the copy phase the row is read using a mysql select and the column has the padded value. However while replicating we get the unpadded value. This means that we get different keyspace_ids. We found bugs where the copy phase inserted the row in one shard but the update during the catchup phase routed this row to another shard where, of course, the update failed.

The PR pads the value found in the binlog to match the value returned my mysql to fix this issue.

Related Issue(s)

#3984

Checklist

Tests were added or are not required
Documentation was added or is not required

deepthi

LGTM

go/vt/vttablet/tabletserver/vstreamer/vstreamer.go

rafael · 2021-04-30T18:06:36Z

go/vt/vttablet/tabletserver/vstreamer/planbuilder.go

+		// the data without this padding. In particular, this causes an issue if a binary(n) column is part of the
+		// sharding key: the keyspace_id() returned during the copy phase (where the value is the result of a mysql query)
+		// is different from the one during replication (where the value is the one from the binlogs)
+		// Hence we need to add the padding here


What a catch this one!!

rohit-nayak-ps · 2021-05-02T21:45:41Z

Just reviewed with Sugu and he recommended that the padding should be done right when it is read from the binlog and not just for computing the keyspace id. That way the padded value, which is also the result of a mysql select, will be sent to all consumers of the vstream.

So moving it to draft for now.

… a select query. This also ensures that if such columns are used as sharding keys we get the same keyspace_id Signed-off-by: Rohit Nayak <[email protected]>

…rs see the padded value instead of doing it later in vstreamer or doint it just for keyspace id computation Signed-off-by: Rohit Nayak <[email protected]>

Signed-off-by: Rohit Nayak <[email protected]>

rohit-nayak-ps · 2021-05-04T11:54:02Z

@sougou I was able to pad the binary columns in the binlog reader. One benefit with this fix is that we could remove the cast we had added earlier to cast binary pks in sql which should improve workflow performance too.

@deepthi, please re-review

shlomi-noach

It's painful to see how small the change is :)

shlomi-noach · 2021-05-04T13:04:15Z

go/mysql/binlog_event_rbr.go

+				paddedData := make([]byte, max)
+				copy(paddedData[:l], mdata)
+				mdata = paddedData
+			}


There's a bug here for CHAR columns using binary collations. If you have e.g. CHAR (3) COLLATE UTF8MB4_BIN then the padding goes out to the max byte length of 12 and you get this in the vreplication stream:
'FOO\0\0\0\0\0\0\0\0\0' and the SQL fails because it's too long for the column.

sougou

I feel like a similar bug may exist with CHAR(N), where we may need to pad with spaces.

rafael · 2021-05-04T17:07:43Z

go/test/endtoend/vreplication/config.go

 	  },
-   "tables": {
+   "tables":  {


nit nit - extra whitespace.

rafael

LGTM

deepthi

LGTM

deepthi · 2021-05-04T22:59:26Z

Let us plan to backport this to 10.0, 9.0 and 8.0.
We also need to document it as a "Known Issue" in the 10.0 release notes.

shlomi-noach · 2021-05-05T06:07:10Z

Also, just realized there was an almost identical recent contribution to gh-ost. I should be more wary.

…ue returned by mysql selects Backport of vitessio#7969 * Pad binlog values for binary() columns to match the value returned by a select query. This also ensures that if such columns are used as sharding keys we get the same keyspace_id * Pad binary() values in the binlog reader directly so that all consumers see the padded value instead of doing it later in vstreamer or doint it just for keyspace id computation Signed-off-by: Rohit Nayak <[email protected]> Signed-off-by: Andres Taylor <[email protected]>

Signed-off-by: Rohit Nayak <[email protected]>

rohit-nayak-ps force-pushed the rn-vr-binary-bug branch from 91b04bf to 57842d7 Compare April 27, 2021 17:42

rohit-nayak-ps marked this pull request as ready for review April 27, 2021 18:40

rohit-nayak-ps requested review from harshit-gangal, shlomi-noach and systay as code owners April 27, 2021 18:40

rohit-nayak-ps requested review from sougou and deepthi April 27, 2021 18:40

systay added Component: VReplication Type: Enhancement Logical improvement (somewhere between a bug and feature) labels Apr 28, 2021

deepthi approved these changes Apr 29, 2021

View reviewed changes

go/vt/vttablet/tabletserver/vstreamer/vstreamer.go Show resolved Hide resolved

rafael reviewed Apr 30, 2021

View reviewed changes

rohit-nayak-ps marked this pull request as draft May 2, 2021 21:40

rohit-nayak-ps added Type: Bug and removed Type: Enhancement Logical improvement (somewhere between a bug and feature) labels May 3, 2021

rohit-nayak-ps added 2 commits May 3, 2021 23:15

Pad binlog values for binary() columns to match the value returned by…

0efd023

… a select query. This also ensures that if such columns are used as sharding keys we get the same keyspace_id Signed-off-by: Rohit Nayak <[email protected]>

Pad binary() values in the binlog reader directly so that all consume…

91357c6

…rs see the padded value instead of doing it later in vstreamer or doint it just for keyspace id computation Signed-off-by: Rohit Nayak <[email protected]>

rohit-nayak-ps force-pushed the rn-vr-binary-bug branch from 966763e to 91357c6 Compare May 3, 2021 21:15

Fix tests

9f8baac

Signed-off-by: Rohit Nayak <[email protected]>

rohit-nayak-ps marked this pull request as ready for review May 4, 2021 11:47

rohit-nayak-ps requested a review from a team May 4, 2021 11:54

shlomi-noach approved these changes May 4, 2021

View reviewed changes

sougou approved these changes May 4, 2021

View reviewed changes

sougou reviewed May 4, 2021

View reviewed changes

rafael reviewed May 4, 2021

View reviewed changes

go/test/endtoend/vreplication/config.go

},

"tables": {

"tables": {

Copy link

Member

rafael May 4, 2021

Choose a reason for hiding this comment

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

nit nit - extra whitespace.

rafael approved these changes May 4, 2021

View reviewed changes

rafael mentioned this pull request May 4, 2021

Slack vitess 9 2021.03.16r4 tinyspeck/vitess#207

Merged

deepthi approved these changes May 4, 2021

View reviewed changes

deepthi added the Backport me! label May 4, 2021

rohit-nayak-ps merged commit f3a2411 into vitessio:master May 5, 2021

rohit-nayak-ps deleted the rn-vr-binary-bug branch May 5, 2021 08:34

systay mentioned this pull request May 6, 2021

VReplication: Pad binlog values for binary() columns to match the value returned by mysql selects #8061

Merged

deepthi mentioned this pull request May 7, 2021

VReplication errors when fixed-length binary column is used as sharding key #8080

Closed

rohit-nayak-ps mentioned this pull request May 17, 2021

VReplication: Pad binlog values for binary() columns to match the value returned by mysql selects #8137

Merged

rohit-nayak-ps mentioned this pull request May 17, 2021

Backport of 7969 into 8.0 #8138

Merged

rohit-nayak-ps added a commit to planetscale/vitess that referenced this pull request Aug 5, 2021

Reapply changes made in vitessio#7969 as a backport to 8.0

5a361c8

Signed-off-by: Rohit Nayak <[email protected]>

tokikanno mentioned this pull request Aug 30, 2021

vreplication: can not replicate char column with binary collate #8743

Closed

jakon89 mentioned this pull request Nov 12, 2021

VReplication discrepancy between copy vs catchup phases for binary columns #9207

Closed

mattlord mentioned this pull request Dec 8, 2021

Take MySQL Column Type Into Account in VStreamer #9331

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VReplication: Pad binlog values for binary() columns to match the value returned by mysql selects #7969

VReplication: Pad binlog values for binary() columns to match the value returned by mysql selects #7969

rohit-nayak-ps commented Apr 27, 2021 •

edited

Loading

deepthi left a comment

rafael Apr 30, 2021

rohit-nayak-ps commented May 2, 2021

rohit-nayak-ps commented May 4, 2021

shlomi-noach left a comment

shlomi-noach May 4, 2021

mattlord Aug 30, 2021 •

edited

Loading

sougou left a comment

rafael May 4, 2021

rafael left a comment

deepthi left a comment

deepthi commented May 4, 2021

shlomi-noach commented May 5, 2021

VReplication: Pad binlog values for binary() columns to match the value returned by mysql selects #7969

VReplication: Pad binlog values for binary() columns to match the value returned by mysql selects #7969

Conversation

rohit-nayak-ps commented Apr 27, 2021 • edited Loading

Description

Related Issue(s)

Checklist

deepthi left a comment

Choose a reason for hiding this comment

rafael Apr 30, 2021

Choose a reason for hiding this comment

rohit-nayak-ps commented May 2, 2021

rohit-nayak-ps commented May 4, 2021

shlomi-noach left a comment

Choose a reason for hiding this comment

shlomi-noach May 4, 2021

Choose a reason for hiding this comment

mattlord Aug 30, 2021 • edited Loading

Choose a reason for hiding this comment

sougou left a comment

Choose a reason for hiding this comment

rafael May 4, 2021

Choose a reason for hiding this comment

rafael left a comment

Choose a reason for hiding this comment

deepthi left a comment

Choose a reason for hiding this comment

deepthi commented May 4, 2021

shlomi-noach commented May 5, 2021

rohit-nayak-ps commented Apr 27, 2021 •

edited

Loading

mattlord Aug 30, 2021 •

edited

Loading