github / gh-ost

GitHub's Online Schema-migration Tool for MySQL
MIT License
12.31k stars 1.25k forks source link

Refine `wait_timeout` override to be at cut-over only #1406

Closed timvaillancourt closed 1 month ago

timvaillancourt commented 5 months ago

Related issue: #1407

Description

This PR refines https://github.com/github/gh-ost/pull/1401 by overriding the session wait_timeout only where it is needed - at the cut-over time where an idle connection could lead to potentially-long table lock if the gh-ost process (or host running it) "freezes"/"stalls" at the cut-over stage

The change (at cut-over only):

  1. Before cut-over:
    • The server wait_timeout is fetched _(via an existing select that fetched time_zone)_
    • The applier session wait_timeout is set to be 3 x the lock-wait timeout
      • This is to ensure the lock is released if gh-ost stalls with a still-active connection here
  2. The cut-over proceeds as normal
  3. After cut-over, the original session wait_timeout is restored to what it was set to pre-cut-over

The --mysql-wait-timeout flag added in #1401 is removed because it is no longer needed. No release has been cut since #1401, so this isn't necessarily a breaking change

In case this PR introduced Go code changes:

timvaillancourt commented 5 months ago

@shlomi-noach I'm curious about your feedback on one consequence of this change

Following this PR, it is possible for the session holding the lock tables to timeout (and unlock tables) before the "magic table" is dropped here

If understand this right, the lock-session hitting wait_timeout will cause the rename tables to succeed. That all sounds better than before, but the "magic table" will be left behind. The impact of this I don't fully understand

-initially-drop-old-table
        Drop a possibly existing OLD table (remains from a previous run?) before beginning operation. Default is to panic and abort if such table exists

Would gh-ost just-fix this scenario for users with -initially-drop-old-table? Any other race-condition risks you can see the lock-release causing 🤔? 🙇

shlomi-noach commented 4 months ago

If understand this right, the lock-session hitting wait_timeout will cause the rename tables to succeed.

No, actually. The RENAME will not succeed, because the magic table is still in place. The RENAME statement attempts to rename original-table into magic-table. But since magic-table is there, the RENAME will fail.

The next cut-over attempt will first, before placing any locks, attempt to DropAtomicCutOverSentryTableIfExists() before re-creating it.

This should be safe.

timvaillancourt commented 1 month ago

If understand this right, the lock-session hitting wait_timeout will cause the rename tables to succeed.

No, actually. The RENAME will not succeed, because the magic table is still in place. The RENAME statement attempts to rename original-table into magic-table. But since magic-table is there, the RENAME will fail.

The next cut-over attempt will first, before placing any locks, attempt to DropAtomicCutOverSentryTableIfExists() before re-creating it.

This should be safe.

@shlomi-noach that makes sense (eventually)! Thanks for the validations and explanations

timvaillancourt commented 1 month ago

@shlomi-noach / @meiji163: I believe I've addressed the PR suggestions and this is ready for another review 🙇

timvaillancourt commented 1 month ago

Merging. @shlomi-noach let me know if I missed something and I'll make a follow-up PR 👍