Fix binlog end log pos over lost data

[shaohk]

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.

[dikang123]

when will this PR be merged?

[timvaillancourt]

@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:

1. Test that a normal *replication.RowsEvent event will be accepted
2. 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 🙇

[shaohk]

@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:

1. Test that a normal *replication.RowsEvent event will be accepted
2. 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]

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

[shaohk]

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

[meiji163]

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

[shaohk]

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

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

[meiji163]

LGTM, can you take another look @timvaillancourt?

[timvaillancourt]

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]

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]

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]

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