Skip to content

fix: because lock is not release, drop cutover sentry table is hanged #1180

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 4 commits into from
Feb 6, 2023

Conversation

lmtwga
Copy link
Contributor

@lmtwga lmtwga commented Sep 16, 2022

Related issue: #1171

@lmtwga
Copy link
Contributor Author

lmtwga commented Sep 16, 2022

@dm-2
Please review when you have time.
Thank you very much!

@lmtwga
Copy link
Contributor Author

lmtwga commented Sep 22, 2022

@dm-2 @timvaillancourt
Please review when you have time.
Thank you very much!

@dm-2
Copy link
Contributor

dm-2 commented Oct 21, 2022

The problem and the fix make sense to me 👍

@timvaillancourt would you mind taking a look as well? 🙇 Your thoughts would be much appreciated, given that it's such a critical section of code!

@gtowey-air
Copy link

Hey @dm-2; I just got hit with this bug as well and I would love to get this or some related fix merged -- would you mind if I took a look and did some testing on this PR to validate?

}
})
if _, err := tx.Exec(query); err != nil {
this.migrationContext.Log.Errore(err)

Choose a reason for hiding this comment

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

It might be better to execute this query with a timeout and exit the program if it fails. While this PR should change ghost so that it will always successfully unlock the tables, in the event that there is some other unforseen race condition or deadlock here, then stopping program execution would be a guaranteed way to at least release all locks held by this program.

It would have the unfortunate side effect of leaving the _del table in existence which would need to be cleaned up later, but IMO that's operationally better than holding table locks which will cause any application actively trying to use the tables to stall.

@gtowey-air
Copy link

@rashiq @dm-2 I've tested this PR and it looks like it fixes the problem. I recommend that we go ahead and merge this!

@timvaillancourt
Copy link
Collaborator

@timvaillancourt would you mind taking a look as well?

Sorry for the delay, this LGTM 👍

@gtowey-air
Copy link

@rashiq Thanks for the approval! Would you be able to merge this?

@rashiq
Copy link
Member

rashiq commented Feb 6, 2023

@gtowey-air Hey! (also nice to see you again!) - sorry for the delay, merging now

@rashiq rashiq merged commit b7db8c6 into github:master Feb 6, 2023
@SocalNick SocalNick mentioned this pull request May 16, 2023
btruhand pushed a commit to Faire/gh-ost that referenced this pull request Oct 20, 2023
btruhand pushed a commit to Faire/gh-ost that referenced this pull request Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants