Skip to content

Pt 2531 pt online schema change creates sleeping threads in replicas - #1171

Open
svetasmirnova wants to merge 10 commits into
3.xfrom
PT-2531_pt-online-schema-change_creates_sleeping_threads_in_replicas
Open

svetasmirnova wants to merge 10 commits into
3.xfrom
PT-2531_pt-online-schema-change_creates_sleeping_threads_in_replicas

Conversation

@svetasmirnova

Copy link
Copy Markdown
Collaborator
  • The contributed code is licensed under GPL v2.0
  • Contributor Licence Agreement (CLA) is signed
  • util/update-modules has been ran
  • Documentation updated
  • Test suite update

- Restored replication failure behavior on stopped replica unless option --no-fail-on-stopped-replication is specified
Copilot AI lite review requested due to automatic review settings September 7, 2026 16:02

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The new replica-refresh callback invocation and stale-replica disconnect logic introduce confirmed runtime/behavioral bugs (callback argument mismatch, undef deref, and incorrect disconnect behavior).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR aims to prevent pt-online-schema-change from leaving sleeping (stale) connections on replicas by reusing existing replica connections across refreshes and disconnecting replicas that are no longer part of the refreshed replica set.

Changes:

  • Pass the current replica list into replica refresh paths to enable connection reuse.
  • Reuse known replica connections when reading replicas from a DSN table and disconnect stale replicas.
  • Adjust the pt-2168 test timing and fix minor typos/formatting.
File summaries
File Description
t/pt-online-schema-change/pt-2168.t Tweaks replica-delay timing used by the test harness.
lib/ReplicaLagWaiter.pm Changes how replica refresh callbacks are invoked during lag waiting.
lib/MasterSlave.pm Reuses known replica connections when reading from DSN tables and attempts to disconnect stale replicas.
bin/pt-online-schema-change Mirrors the library changes in the embedded code paths (replica refresh + DSN-table reuse + disconnect stale replicas) and adjusts stopped-replication behavior.
Review details

Suppressed comments (1)

lib/MasterSlave.pm:1142

  • The stale-replica disconnect loop disconnects known_replica as soon as it finds any non-matching new_replica (i.e., it can disconnect a replica even if it exists later in @cxn). Also, $args{replicas} may be undef. Only disconnect after verifying no match against any new replica, and guard the dereference.
   # Close database connections in stale replicas
   REPLICAS:
   foreach my $known_replica ( @{$args{replicas}} ) {
      foreach my $new_replica ( @cxn ) {
         if ( $known_replica->{dsn}->{h} eq $new_replica->{dsn}->{h} and
            $known_replica->{dsn}->{P} eq $new_replica->{dsn}->{P} ) {
            next REPLICAS;
         }
         $known_replica->{dbh}->disconnect;
      }
   }
  • Files reviewed: 3/4 changed files
  • Comments generated: 2
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread lib/MasterSlave.pm Outdated
Comment thread lib/ReplicaLagWaiter.pm
Co-authored-by: svetasmirnova <2982342+svetasmirnova@users.noreply.github.com>
@it-percona-cla

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ svetasmirnova
❌ Copilot
You have signed the CLA already but the status is still pending? Let us recheck it.

svetasmirnova and others added 2 commits September 7, 2026 21:16
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
- Reverted Copilot change for replicas array
- Added comparison for replicas by user and password, so tools will reconnect if user account change in the DSN table
@svetasmirnova
svetasmirnova marked this pull request as ready for review September 7, 2026 21:35

@matias-sanchez matias-sanchez left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

adding enhancements based on testing with 1 source and 4 replicas in the dsns table. With both applied i saw no reconnections over many chunks.

Comment thread lib/MasterSlave.pm Outdated
Comment thread lib/MasterSlave.pm Outdated
svetasmirnova and others added 2 commits September 8, 2026 22:53
Co-authored-by: matias-sanchez <49377960+matias-sanchez@users.noreply.github.com>
Co-authored-by: matias-sanchez <49377960+matias-sanchez@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants