Fix binlog end log pos over lost data by shaohk · Pull Request #1367 · github/gh-ost

shaohk · 2024-01-17T09:02:44Z

Related issue: #1366

Description

This PR [Fix binlog end log pos over lost data]

In case this PR introduced Go code changes:

contributed code is using same conventions as original code
script/cibuild returns with no formatting errors, build errors or unit test errors.

… over 4g lost data

…er-lost-data Calculate whether the binlog log_pos overflows beyond 4G using end_log_pos and event_size.

x

dikang123 · 2024-01-18T07:29:54Z

when will this PR be merged?

timvaillancourt · 2024-02-01T15:45:00Z

@shaohk thanks a lot for this PR, this sounds like a difficult problem to debug!

Can you think of any ways that this check can be tested in go unit tests? I wonder if a test could:

Test that a normal *replication.RowsEvent event will be accepted
Test that an oversized *replication.RowsEvent event it not accepted

Luckily, handleRowEvents doesn't need a real MySQL server as it pushes events to entriesChannel chan<- *BinlogEntry, which should make this relatively simple to test

timvaillancourt

Looks good! Added one comment requesting unit tests 🙇

…aohk/gh-ost into fix-binlog-end-log-pos-over-lost-data

x

shaohk · 2024-02-07T06:17:30Z

@shaohk thanks a lot for this PR, this sounds like a difficult problem to debug!

Can you think of any ways that this check can be tested in go unit tests? I wonder if a test could:

Test that a normal *replication.RowsEvent event will be accepted

Test that an oversized *replication.RowsEvent event it not accepted

Luckily, handleRowEvents doesn't need a real MySQL server as it pushes events to entriesChannel chan<- *BinlogEntry, which should make this relatively simple to test

@timvaillancourt Thank you for reviewing this PR. I have added unit tests as requested. Could you please take a look again? Thank you for your hard work.

timvaillancourt · 2024-02-24T01:27:50Z

go/mysql/binlog.go

 	return this.LogFile == other.LogFile && this.LogPos == other.LogPos
 }
+
+// IsLogPosOverflowBeyond4Bytes returns true if the coordinate endpos is overflow beyond 4 bytes.


@shaohk last request, can we add a reference to why a value over 4 bytes is an overflow?

@timvaillancourt OK, I have added. Please take a look again, Thks.

change IsLogPosOverflowBeyond4Bytes comment.

x

meiji163 · 2024-02-27T22:44:57Z

@shaohk Can you take a look at the failing tests and linter?

x

shaohk · 2024-02-29T08:20:34Z

@shaohk Can you take a look at the failing tests and linter?

@meiji163
OK, Please take a look again, Thks.

meiji163 · 2024-03-12T05:50:40Z

LGTM, can you take another look @timvaillancourt?

timvaillancourt · 2024-03-12T21:28:55Z

LGTM, can you take another look @timvaillancourt?

@meiji163 / @shaohk: I think erroring here is the easiest next step, so LGTM 👍

But longer term, what do we "want" gh-ost to do here? Or to put it differently, what does the MySQL replica do in this scenario? I'm assuming it doesn't error right? 🤔

timvaillancourt

Approved with longer-term question

meiji163 · 2024-03-12T23:15:44Z

I'm not sure how MySQL handles it
I found this bug report from last year https://bugs.mysql.com/bug.php?id=112189

shaohk · 2024-03-13T03:20:30Z

LGTM, can you take another look @timvaillancourt?

@meiji163 / @shaohk: I think erroring here is the easiest next step, so LGTM 👍

But longer term, what do we "want" gh-ost to do here? Or to put it differently, what does the MySQL replica do in this scenario? I'm assuming it doesn't error right? 🤔

https://dev.mysql.com/doc/refman/8.0/en/replication-options-binary-log.html#sysvar_max_binlog_cache_size.

When [gtid_mode](https://dev.mysql.com/doc/refman/8.0/en/replication-options-gtids.html#sysvar_gtid_mode) is not ON, the maximum recommended value is 4GB, due to the fact that, in this case, MySQL cannot work with binary log positions greater than 4GB; when gtid_mode is ON, this limitation does not apply, and the server can work with binary log positions of arbitrary size.

So I believe that MySQL master-slave replication is based on the GTID mode, so there is no problem with MySQL master-slave replication. The gh-ost tool for synchronizing binlogs uses a non-GTID mode, which is why this issue arises.

timvaillancourt · 2024-03-13T23:52:45Z

So I believe that MySQL master-slave replication is based on the GTID mode, so there is no problem with MySQL master-slave replication. The gh-ost tool for synchronizing binlogs uses a non-GTID mode, which is why this issue arises.

@shaohk great, thanks for clarifying! I made an attempt to add GTID support to gh-ost long ago and I may revive this effort for many reasons. This adds a new reason to the pile

shaohoukun added 2 commits January 16, 2024 15:25

fix(fix binlog end-log-pos over 4g lost data): fix binlog end-log-pos…

3b7ae99

… over 4g lost data

fix(fix-binlog-end-log-pos-over-lost-data): fix-binlog-end-log-pos-ov…

6f77684

…er-lost-data Calculate whether the binlog log_pos overflows beyond 4G using end_log_pos and event_size.

shaohk requested a review from a user January 17, 2024 09:02

shaohk requested review from meiji163, rashiq and timvaillancourt as code owners January 17, 2024 09:02

fix(x): x

88fea51

x

Merge branch 'master' into fix-binlog-end-log-pos-over-lost-data

a27a339

Merge branch 'master' into fix-binlog-end-log-pos-over-lost-data

751bb62

timvaillancourt requested changes Feb 1, 2024

View reviewed changes

shaohk requested a review from timvaillancourt February 5, 2024 10:48

shaohoukun added 3 commits February 6, 2024 11:15

test(fix-binlog-end-log-pos-over-lost-data): add unit test

60134c4

Merge branch 'fix-binlog-end-log-pos-over-lost-data' of github.com:sh…

e640f03

…aohk/gh-ost into fix-binlog-end-log-pos-over-lost-data

fix(fix-binlog-end-log-pos-over-lost-data): x

a18e231

x

timvaillancourt reviewed Feb 24, 2024

View reviewed changes

shaohk added 2 commits February 26, 2024 10:38

Update binlog.go

d58be83

change IsLogPosOverflowBeyond4Bytes comment.

Update binlog.go

bad5296

x

fix(fix-binlog-end-log-pos-over-lost-data): fix doc

f869a00

x

meiji163 added 2 commits March 11, 2024 20:03

Merge branch 'master' into fix-binlog-end-log-pos-over-lost-data

58927af

Merge branch 'master' into fix-binlog-end-log-pos-over-lost-data

b2e742c

meiji163 approved these changes Mar 12, 2024

View reviewed changes

Merge branch 'master' into fix-binlog-end-log-pos-over-lost-data

2f1d1ad

timvaillancourt self-requested a review March 12, 2024 21:29

timvaillancourt approved these changes Mar 12, 2024

View reviewed changes

meiji163 merged commit 48b34bc into github:master Mar 12, 2024

ramshad-sha mentioned this pull request Mar 25, 2024

PR #1400

Closed

timvaillancourt added this to the v1.1.7 milestone Apr 8, 2024

Conversation

shaohk commented Jan 17, 2024

Description

Uh oh!

dikang123 commented Jan 18, 2024

Uh oh!

timvaillancourt commented Feb 1, 2024

Uh oh!

timvaillancourt left a comment

Choose a reason for hiding this comment

Uh oh!

shaohk commented Feb 7, 2024

Uh oh!

timvaillancourt Feb 24, 2024

Choose a reason for hiding this comment

Uh oh!

shaohk Feb 26, 2024 • 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!

meiji163 commented Feb 27, 2024

Uh oh!

shaohk commented Feb 29, 2024

Uh oh!

meiji163 commented Mar 12, 2024

Uh oh!

timvaillancourt commented Mar 12, 2024

Uh oh!

timvaillancourt left a comment

Choose a reason for hiding this comment

Uh oh!

meiji163 commented Mar 12, 2024

Uh oh!

shaohk commented Mar 13, 2024

Uh oh!

timvaillancourt commented Mar 13, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

shaohk Feb 26, 2024 •

edited

Loading