Skip to content

fix: issue 909 #915

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Mar 8, 2021
Merged

fix: issue 909 #915

merged 6 commits into from
Mar 8, 2021

Conversation

cenkore
Copy link

@cenkore cenkore commented Jan 13, 2021

Add a PR for #909

The reason is that the last byte of 0x00 was truncated when mysql wrote Binlog, so the data obtained when gh-ost parsed binlog through go-mysql was already truncated

you can read binlog with hexdump as follows, or debug at here , you will get the actual length is 15

Position Timestamp Type Master ID Size Master Pos Flags
c86 60 14 fc 5f 20 35 23 00 00 7f 00 00 00 05 0d 00 00 00 00
c99 99 5e 00 00 00 00 01 00 02 00 05 ff f0 10 00 2e |................|
ca9 36 3f 30 50 44 de 79 e9 b3 d0 10 5b c2 3e 01 00 |6.0PD.y.........|
cb9 00 00 10 e5 3c 11 1a fe f8 a2 d3 b9 a3 48 41 08 |.............HA.|
cc9 d0 8f 2e 99 a8 3e 74 c0 e0 0f 15 0b 7b 5b 51 6f |......t.......Qo|
cd9 87 12 5e 85 97 ff b4 4b f2 02 00 00 00 10 e5 3c |.......K........|
ce9 11 1a fe f8 a2 d3 b9 a3 48 41 08 d0 8f 2b 99 a8 |........HA......|
cf9 3e 74 80 99 a8 3e 75 00 43 17 a8 3b |.t....u.C...|
Delete_rows: table id 24217 flags: STMT_END_F

@rashiq
Copy link
Member

rashiq commented Jan 13, 2021

Hi @cenkore! Thanks for the contribution!

It seems like the build is failing because it's it's not properly formatted with gofmt.

Could you please run the following command on the branch?

gofmt -s -w  go/

@cenkore
Copy link
Author

cenkore commented Jan 14, 2021

@rashiq , thanks for your suggestion, it looks good!

@sfairat15
Copy link

Hi, @cenkore! We checked your PR on test db and it works like a charm! Thank you!

@rashiq rashiq temporarily deployed to production/mysql_role=ghost_testing January 19, 2021 17:02 Inactive
@rashiq
Copy link
Member

rashiq commented Jan 19, 2021

@cenkore Thanks! We've deployed it to our testing cluster, will monitor it the coming few days and if everything is fine we'll merge it 👍

@cenkore
Copy link
Author

cenkore commented Jan 20, 2021

@cenkore Thanks! We've deployed it to our testing cluster, will monitor it the coming few days and if everything is fine we'll merge it 👍

Thank you!

@vitalyu
Copy link

vitalyu commented Feb 10, 2021

@rashiq is any updates? thx

go/sql/types.go Outdated
Comment on lines 53 to 64

if this.Type == BinaryColumnType {
size := len([]byte(arg.(string)))
if uint(size) < this.BinaryOctetLength {
buf := bytes.NewBuffer([]byte(arg.(string)))
for i := uint(0); i < (this.BinaryOctetLength - uint(size)); i++ {
buf.Write([]byte{0})
}
arg = buf.String()
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please explain what the logic above does? Thank you!

Copy link

Choose a reason for hiding this comment

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

This is all about this issue #909
Also, U can read this https://dev.mysql.com/doc/refman/5.7/en/binary-varbinary.html

You should consider the preceding padding and stripping 
characteristics carefully if you plan to use the BINARY 
data type for storing binary data and you require that 
the value retrieved be exactly the same as the value stored. 
The following example illustrates how 0x00-padding 
of BINARY values affects column value comparisons

Please, read @cenkore's first message:

The reason is that the last byte of 0x00 was truncated 
when mysql wrote Binlog, so the data obtained when 
gh-ost parsed binlog through go-mysql was already truncated

So, this part of code appends missing zeroes.

We use this fork on production about a month, and all is OK.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you. Is there a limit on how many zeros you need to pad with, or can this number be very high?

Copy link
Author

Choose a reason for hiding this comment

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

@shlomi-noach Filling a high number zero bytes may affect processing efficiency. after thinking about it, we can fix it more lighter as below, other scenarios will not cause data consistency problems. 40bc5ae

  • only handle the binary column in the unique key
  • only handle update/delete dml event

Copy link
Contributor

@shlomi-noach shlomi-noach left a comment

Choose a reason for hiding this comment

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

LGTM pending testing

@rashiq rashiq temporarily deployed to production/mysql_role=ghost_testing March 8, 2021 19:20 Inactive
@rashiq rashiq merged commit d4c91e6 into github:master Mar 8, 2021
@rashiq
Copy link
Member

rashiq commented Mar 8, 2021

Sorry it took so long! merged! 🎉

timvaillancourt added a commit that referenced this pull request Mar 24, 2021
This reverts commit d4c91e6, reversing
changes made to 62ce678.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants