fix: lost data due to mysql tow-phase-commit #1054

shaohk · 2021-11-15T10:38:22Z

fix: lost data due to mysql tow-phase-commit

A Pull Request should be associated with an Issue.

We wish to have discussions in Issues. A single issue may be targeted by multiple PRs.
If you're offering a new feature or fixing anything, we'd like to know beforehand in Issues,
and potentially we'll be able to point development in a particular direction.

Related issue: #1030

Further notes in https://github.com/github/gh-ost/blob/master/.github/CONTRIBUTING.md
Thank you! We are open to PRs, but please understand if for technical reasons we are unable to accept each and any PR

Description

This PR [fix lost data due to mysql tow-phase-commit]

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.

…o mysql tow-phase-commit

Fanduzi · 2021-11-16T04:04:00Z

Very clever!

timvaillancourt · 2021-11-16T21:03:15Z

@wangzihuacool are you able to test if this change fixes the issue?

I think this approach is a bit cleaner than using a lock in #1040, assuming it all works

wangzihuacool · 2021-11-17T13:51:23Z

Both using a shared lock in #1040 and triggering a WriteChangelog event can fix the issue. I have tested.
I think using a shared lock is a direct and effective way to fix it, with better compatibility, just like the way rowcopy does. By contract triggering a writechangelog event seems to be a more clever way, but a bit confusing, with a small side effect that gh-ost migration may exits abnormally if WriteChangelog fails or timeout for some reason.

shaohk · 2021-11-18T02:51:58Z

Both using a shared lock in #1040 and triggering a WriteChangelog event can fix the issue. I have tested. I think using a shared lock is a direct and effective way to fix it, with better compatibility, just like the way rowcopy does. By contract triggering a writechangelog event seems to be a more clever way, but a bit confusing, with a small side effect that gh-ost migration may exits abnormally if WriteChangelog fails or timeout for some reason.

Yes, using share lock is also a very effective way. But we are worried about an unexpected problem caused by share lock, so I added a write heartbeat here. Even if the gh-ost fails due to the heartbeat write timeout, this is a small probability time and acceptable.

Fanduzi · 2022-01-06T01:53:10Z

Any progress?

Fanduzi · 2022-04-30T03:11:11Z

Hi @dm-2 @timvaillancourt
#1040 and #1054 has had a PR for fixing the semi-sync data loss bug for some time now, is there any plans to merge?

timvaillancourt

LGTM. Updating the heartbeat is an extremely safe operation with very low risk 👍

cc @dm-2 / @rashiq for review/merge

timvaillancourt · 2022-06-16T23:06:02Z

go/logic/migrator.go

@@ -388,6 +388,12 @@ func (this *Migrator) Migrate() (err error) {
 	if err := this.addDMLEventsListener(); err != nil {
 		return err
 	}
+
+	// write a heartbeat data into the ghc table, and avoid lost data due to MySQL two-phase-commit.


@shaohk could you add a bit more background in this comment on "why" writing the heartbeat causes us to avoid lost data due to MySQL two-phase-commit? That will be useful to those without context

I'll try to describe the scene:

When using semi-sync and setting rpl_semi_sync_master_wait_point=AFTER_SYNC, if an INSERT statement is being committed but blocks due to an unmet ack count, the data inserted by the transaction is not visible to ReadMigrationRangeValues, so the copy of the existing data in the table does not include the new row inserted by the transaction. However, the binlog event for the transaction is already written to the binlog, so the addDMLEventsListener only captures the binlog event after the transaction, and thus the transaction's binlog event is not captured, resulting in data loss.

If you add a heartbeat, and the transaction commit blocks because the ack is not met, then the heartbeat will not be able to write, so the ReadMigrationRangeValues will not be run. When the heartbeat writes successfully, the ReadMigrationRangeValues will read the newly inserted data, thus Avoiding data loss due to the above problem

@Fanduzi that's my understanding too 👍

I'm hoping we can add some of this context into the code-comment on line 392, so that this is easier to read for those without context

@shaohk two more questions:

Would it make sense to move this to be within the .ReadMigrationRangeValues() func? The goal with the .Migrate() func is to leave a lot of the details to other functions

What do you think about updating a ChangeLogState instead of the heartbeat? .WriteChangelogState() in the applier is used for this. Perhaps a new ChangeLogState like ReadMigrationRangeValues would work here 🤔. Example PR

@shaohk two more questions:

Would it make sense to move this to be within the .ReadMigrationRangeValues() func? The goal with the .Migrate() func is to leave a lot of the details to other functions

What do you think about updating a ChangeLogState instead of the heartbeat? .WriteChangelogState() in the applier is used for this. Perhaps a new ChangeLogState like ReadMigrationRangeValues would work here 🤔. Example PR

Sorry, I just saw these messages.

I complete the annotation information in the code.

I think the above two suggestions are very good, I will revise the PR and mention it again.

@shaohk two more questions:

Would it make sense to move this to be within the .ReadMigrationRangeValues() func? The goal with the .Migrate() func is to leave a lot of the details to other functions

What do you think about updating a ChangeLogState instead of the heartbeat? .WriteChangelogState() in the applier is used for this. Perhaps a new ChangeLogState like ReadMigrationRangeValues would work here 🤔. Example PR

@timvaillancourt
Use changelog state instead of the heartbeat PR

I'll try to describe the scene:

When using semi-sync and setting rpl_semi_sync_master_wait_point=AFTER_SYNC, if an INSERT statement is being committed but blocks due to an unmet ack count, the data inserted by the transaction is not visible to ReadMigrationRangeValues, so the copy of the existing data in the table does not include the new row inserted by the transaction. However, the binlog event for the transaction is already written to the binlog, so the addDMLEventsListener only captures the binlog event after the transaction, and thus the transaction's binlog event is not captured, resulting in data loss.

If you add a heartbeat, and the transaction commit blocks because the ack is not met, then the heartbeat will not be able to write, so the ReadMigrationRangeValues will not be run. When the heartbeat writes successfully, the ReadMigrationRangeValues will read the newly inserted data, thus Avoiding data loss due to the above problem

@Fanduzi
Thanks, I added your description to the code comments. #1141

timvaillancourt · 2022-06-24T20:32:16Z

Closing in favour of #1141 👍

fix(fix lost data due to mysql tow-phase-commit): fix lost data due t…

42d293b

…o mysql tow-phase-commit

timvaillancourt added the bug label Jun 16, 2022

timvaillancourt self-requested a review June 16, 2022 19:24

timvaillancourt added this to the v1.1.5 milestone Jun 16, 2022

Merge branch 'master' into fix-two-phase-commit-lost-data

02a26a5

timvaillancourt approved these changes Jun 16, 2022

View reviewed changes

timvaillancourt added the low-risk label Jun 16, 2022

timvaillancourt reviewed Jun 16, 2022

View reviewed changes

timvaillancourt modified the milestones: v1.1.5, v1.1.6 Jun 23, 2022

timvaillancourt closed this Jun 24, 2022

timvaillancourt modified the milestones: v1.1.6, v1.1.5 Jul 6, 2022

dm-2 removed this from the v1.1.5 milestone Jul 7, 2022

wangzihuacool mentioned this pull request Oct 20, 2022

there is risk of data loss for the gh-ost #1186

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: lost data due to mysql tow-phase-commit #1054

fix: lost data due to mysql tow-phase-commit #1054

shaohk commented Nov 15, 2021 •

edited

Loading

Fanduzi commented Nov 16, 2021

timvaillancourt commented Nov 16, 2021

wangzihuacool commented Nov 17, 2021

shaohk commented Nov 18, 2021

Fanduzi commented Jan 6, 2022

Fanduzi commented Apr 30, 2022

timvaillancourt left a comment

timvaillancourt Jun 16, 2022

Fanduzi Jun 18, 2022

timvaillancourt Jun 18, 2022

timvaillancourt Jun 18, 2022 •

edited

Loading

shaohk Jun 24, 2022

shaohk Jun 24, 2022 •

edited

Loading

shaohk Jun 24, 2022 •

edited

Loading

timvaillancourt commented Jun 24, 2022

fix: lost data due to mysql tow-phase-commit #1054

fix: lost data due to mysql tow-phase-commit #1054

Conversation

shaohk commented Nov 15, 2021 • edited Loading

A Pull Request should be associated with an Issue.

Description

Fanduzi commented Nov 16, 2021

timvaillancourt commented Nov 16, 2021

wangzihuacool commented Nov 17, 2021

shaohk commented Nov 18, 2021

Fanduzi commented Jan 6, 2022

Fanduzi commented Apr 30, 2022

timvaillancourt left a comment

Choose a reason for hiding this comment

timvaillancourt Jun 16, 2022

Choose a reason for hiding this comment

Fanduzi Jun 18, 2022

Choose a reason for hiding this comment

timvaillancourt Jun 18, 2022

Choose a reason for hiding this comment

timvaillancourt Jun 18, 2022 • edited Loading

Choose a reason for hiding this comment

shaohk Jun 24, 2022

Choose a reason for hiding this comment

shaohk Jun 24, 2022 • edited Loading

Choose a reason for hiding this comment

shaohk Jun 24, 2022 • edited Loading

Choose a reason for hiding this comment

timvaillancourt commented Jun 24, 2022

shaohk commented Nov 15, 2021 •

edited

Loading

timvaillancourt Jun 18, 2022 •

edited

Loading

shaohk Jun 24, 2022 •

edited

Loading

shaohk Jun 24, 2022 •

edited

Loading