diff --git a/bin/pt-archiver b/bin/pt-archiver index d4b4ab7ba..886f30ebd 100755 --- a/bin/pt-archiver +++ b/bin/pt-archiver @@ -4742,6 +4742,7 @@ sub new { DSNParser => $dp, is_cluster_node => undef, parent => $args{parent}, + preserve_dbh => $args{preserve_dbh}, }; return bless $self, $class; @@ -4800,7 +4801,7 @@ sub set_dbh { $self->{hostname} = $hostname; } - if ( $self->{parent} ) { + if ( $self->{preserve_dbh} ) { PTDEBUG && _d($dbh, 'Setting InactiveDestroy=1 in parent'); $dbh->{InactiveDestroy} = 1; } @@ -4923,7 +4924,7 @@ sub DESTROY { PTDEBUG && _d('Destroying cxn'); - if ( $self->{parent} ) { + if ( $self->{preserve_dbh} ) { PTDEBUG && _d($self->{dbh}, 'Not disconnecting dbh in parent'); } elsif ( $self->{dbh} diff --git a/bin/pt-config-diff b/bin/pt-config-diff index 36932ebcb..845ef8323 100755 --- a/bin/pt-config-diff +++ b/bin/pt-config-diff @@ -2368,6 +2368,7 @@ sub new { DSNParser => $dp, is_cluster_node => undef, parent => $args{parent}, + preserve_dbh => $args{preserve_dbh}, }; return bless $self, $class; @@ -2426,7 +2427,7 @@ sub set_dbh { $self->{hostname} = $hostname; } - if ( $self->{parent} ) { + if ( $self->{preserve_dbh} ) { PTDEBUG && _d($dbh, 'Setting InactiveDestroy=1 in parent'); $dbh->{InactiveDestroy} = 1; } @@ -2549,7 +2550,7 @@ sub DESTROY { PTDEBUG && _d('Destroying cxn'); - if ( $self->{parent} ) { + if ( $self->{preserve_dbh} ) { PTDEBUG && _d($self->{dbh}, 'Not disconnecting dbh in parent'); } elsif ( $self->{dbh} diff --git a/bin/pt-deadlock-logger b/bin/pt-deadlock-logger index 2f21bc30c..cf796be3c 100755 --- a/bin/pt-deadlock-logger +++ b/bin/pt-deadlock-logger @@ -2715,6 +2715,7 @@ sub new { DSNParser => $dp, is_cluster_node => undef, parent => $args{parent}, + preserve_dbh => $args{preserve_dbh}, }; return bless $self, $class; @@ -2773,7 +2774,7 @@ sub set_dbh { $self->{hostname} = $hostname; } - if ( $self->{parent} ) { + if ( $self->{preserve_dbh} ) { PTDEBUG && _d($dbh, 'Setting InactiveDestroy=1 in parent'); $dbh->{InactiveDestroy} = 1; } @@ -2896,7 +2897,7 @@ sub DESTROY { PTDEBUG && _d('Destroying cxn'); - if ( $self->{parent} ) { + if ( $self->{preserve_dbh} ) { PTDEBUG && _d($self->{dbh}, 'Not disconnecting dbh in parent'); } elsif ( $self->{dbh} @@ -4711,7 +4712,7 @@ sub main { my $src = Cxn->new( dsn_string => shift @ARGV, - parent => $o->get('daemonize'), + preserve_dbh => $o->get('daemonize'), DSNParser => $dp, OptionParser => $o, ); @@ -4735,7 +4736,7 @@ sub main { $dst = Cxn->new( dsn => $dst_dsn, prev_dsn => ($src ? $src->dsn : undef), - parent => $o->get('daemonize'), + preserve_dbh => $o->get('daemonize'), DSNParser => $dp, OptionParser => $o, set => $set_tz, @@ -4806,13 +4807,13 @@ sub main { # If we daemonized, the parent has already exited and we're the child. # We shared a copy of every Cxn with the parent, and the parent's copies - # were destroyed but the dbhs were not disconnected because the parent + # were destroyed but the dbhs were not disconnected because the preserve_dbh # attrib was true. Now, as the child, set it false so the dbhs will be # disconnected when our Cxn copies are destroyed. If we didn't daemonize, # then we're not really a parent (since we have no children), so set it # false to auto-disconnect the dbhs when our Cxns are destroyed. - $src->{parent} = 0; - $dst->{parent} = 0 if $dst; + $src->{preserve_dbh} = 0; + $dst->{preserve_dbh} = 0 if $dst; # ######################################################################## # Do the version-check diff --git a/bin/pt-fk-error-logger b/bin/pt-fk-error-logger index a758d766d..9985136e4 100755 --- a/bin/pt-fk-error-logger +++ b/bin/pt-fk-error-logger @@ -1869,6 +1869,7 @@ sub new { DSNParser => $dp, is_cluster_node => undef, parent => $args{parent}, + preserve_dbh => $args{preserve_dbh}, }; return bless $self, $class; @@ -1927,7 +1928,7 @@ sub set_dbh { $self->{hostname} = $hostname; } - if ( $self->{parent} ) { + if ( $self->{preserve_dbh} ) { PTDEBUG && _d($dbh, 'Setting InactiveDestroy=1 in parent'); $dbh->{InactiveDestroy} = 1; } @@ -2050,7 +2051,7 @@ sub DESTROY { PTDEBUG && _d('Destroying cxn'); - if ( $self->{parent} ) { + if ( $self->{preserve_dbh} ) { PTDEBUG && _d($self->{dbh}, 'Not disconnecting dbh in parent'); } elsif ( $self->{dbh} @@ -4177,7 +4178,7 @@ sub main { if ( my $src_dsn_string = shift @ARGV ) { $src = Cxn->new( dsn_string => $src_dsn_string, - parent => $o->get('daemonize'), + preserve_dbh => $o->get('daemonize'), DSNParser => $dp, OptionParser => $o, ); @@ -4188,7 +4189,7 @@ sub main { $dst = Cxn->new( dsn => $dst_dsn, prev_dsn => ($src ? $src->dsn : undef), - parent => $o->get('daemonize'), + preserve_dbh => $o->get('daemonize'), DSNParser => $dp, OptionParser => $o, ); @@ -4244,13 +4245,13 @@ sub main { # If we daemonized, the parent has already exited and we're the child. # We shared a copy of every Cxn with the parent, and the parent's copies - # were destroyed but the dbhs were not disconnected because the parent + # were destroyed but the dbhs were not disconnected because the preserve_dbh # attrib was true. Now, as the child, set it false so the dbhs will be # disconnected when our Cxn copies are destroyed. If we didn't daemonize, # then we're not really a parent (since we have no children), so set it # false to auto-disconnect the dbhs when our Cxns are destroyed. - $src->{parent} = 0; - $dst->{parent} = 0 if $dst; + $src->{preserve_dbh} = 0; + $dst->{preserve_dbh} = 0 if $dst; # ######################################################################## # Do the version-check diff --git a/bin/pt-kill b/bin/pt-kill index 43d41b92d..26aeb112f 100755 --- a/bin/pt-kill +++ b/bin/pt-kill @@ -5557,6 +5557,7 @@ sub new { DSNParser => $dp, is_cluster_node => undef, parent => $args{parent}, + preserve_dbh => $args{preserve_dbh}, }; return bless $self, $class; @@ -5615,7 +5616,7 @@ sub set_dbh { $self->{hostname} = $hostname; } - if ( $self->{parent} ) { + if ( $self->{preserve_dbh} ) { PTDEBUG && _d($dbh, 'Setting InactiveDestroy=1 in parent'); $dbh->{InactiveDestroy} = 1; } @@ -5738,7 +5739,7 @@ sub DESTROY { PTDEBUG && _d('Destroying cxn'); - if ( $self->{parent} ) { + if ( $self->{preserve_dbh} ) { PTDEBUG && _d($self->{dbh}, 'Not disconnecting dbh in parent'); } elsif ( $self->{dbh} @@ -7245,7 +7246,7 @@ sub main { $cxn = Cxn->new( dsn_string => shift @ARGV, NAME_lc => 0, - parent => $o->get('daemonize'), + preserve_dbh => $o->get('daemonize'), DSNParser => $dp, OptionParser => $o, ); @@ -7444,12 +7445,12 @@ sub main { # If we daemonized, the parent has already exited and we're the child. # We shared a copy of every Cxn with the parent, and the parent's copies - # were destroyed but the dbhs were not disconnected because the parent + # were destroyed but the dbhs were not disconnected because the preserve_dbh # attrib was true. Now, as the child, set it false so the dbhs will be # disconnected when our Cxn copies are destroyed. If we didn't daemonize, # then we're not really a parent (since we have no children), so set it # false to auto-disconnect the dbhs when our Cxns are destroyed. - $cxn->{parent} = 0 if $cxn; + $cxn->{preserve_dbh} = 0 if $cxn; # ######################################################################## # Do the version-check diff --git a/bin/pt-online-schema-change b/bin/pt-online-schema-change index 3293048a7..2813f72cc 100755 --- a/bin/pt-online-schema-change +++ b/bin/pt-online-schema-change @@ -3984,6 +3984,7 @@ sub new { DSNParser => $dp, is_cluster_node => undef, parent => $args{parent}, + preserve_dbh => $args{preserve_dbh}, }; return bless $self, $class; @@ -4042,7 +4043,7 @@ sub set_dbh { $self->{hostname} = $hostname; } - if ( $self->{parent} ) { + if ( $self->{preserve_dbh} ) { PTDEBUG && _d($dbh, 'Setting InactiveDestroy=1 in parent'); $dbh->{InactiveDestroy} = 1; } @@ -4165,7 +4166,7 @@ sub DESTROY { PTDEBUG && _d('Destroying cxn'); - if ( $self->{parent} ) { + if ( $self->{preserve_dbh} ) { PTDEBUG && _d($self->{dbh}, 'Not disconnecting dbh in parent'); } elsif ( $self->{dbh} diff --git a/bin/pt-table-checksum b/bin/pt-table-checksum index 1fed83eec..e18af2b12 100755 --- a/bin/pt-table-checksum +++ b/bin/pt-table-checksum @@ -3676,6 +3676,7 @@ sub new { DSNParser => $dp, is_cluster_node => undef, parent => $args{parent}, + preserve_dbh => $args{preserve_dbh}, }; return bless $self, $class; @@ -3734,7 +3735,7 @@ sub set_dbh { $self->{hostname} = $hostname; } - if ( $self->{parent} ) { + if ( $self->{preserve_dbh} ) { PTDEBUG && _d($dbh, 'Setting InactiveDestroy=1 in parent'); $dbh->{InactiveDestroy} = 1; } @@ -3857,7 +3858,7 @@ sub DESTROY { PTDEBUG && _d('Destroying cxn'); - if ( $self->{parent} ) { + if ( $self->{preserve_dbh} ) { PTDEBUG && _d($self->{dbh}, 'Not disconnecting dbh in parent'); } elsif ( $self->{dbh} diff --git a/bin/pt-upgrade b/bin/pt-upgrade index dc69194a7..e8b5b6036 100755 --- a/bin/pt-upgrade +++ b/bin/pt-upgrade @@ -2542,6 +2542,7 @@ sub new { DSNParser => $dp, is_cluster_node => undef, parent => $args{parent}, + preserve_dbh => $args{preserve_dbh}, }; return bless $self, $class; @@ -2600,7 +2601,7 @@ sub set_dbh { $self->{hostname} = $hostname; } - if ( $self->{parent} ) { + if ( $self->{preserve_dbh} ) { PTDEBUG && _d($dbh, 'Setting InactiveDestroy=1 in parent'); $dbh->{InactiveDestroy} = 1; } @@ -2723,7 +2724,7 @@ sub DESTROY { PTDEBUG && _d('Destroying cxn'); - if ( $self->{parent} ) { + if ( $self->{preserve_dbh} ) { PTDEBUG && _d($self->{dbh}, 'Not disconnecting dbh in parent'); } elsif ( $self->{dbh} diff --git a/lib/Cxn.pm b/lib/Cxn.pm index fd3cfe82c..2e4b13a8a 100644 --- a/lib/Cxn.pm +++ b/lib/Cxn.pm @@ -111,6 +111,7 @@ sub new { DSNParser => $dp, is_cluster_node => undef, parent => $args{parent}, + preserve_dbh => $args{preserve_dbh}, }; return bless $self, $class; @@ -184,7 +185,7 @@ sub set_dbh { $self->{hostname} = $hostname; } - if ( $self->{parent} ) { + if ( $self->{preserve_dbh} ) { PTDEBUG && _d($dbh, 'Setting InactiveDestroy=1 in parent'); $dbh->{InactiveDestroy} = 1; } @@ -334,7 +335,7 @@ sub DESTROY { PTDEBUG && _d('Destroying cxn'); - if ( $self->{parent} ) { + if ( $self->{preserve_dbh} ) { PTDEBUG && _d($self->{dbh}, 'Not disconnecting dbh in parent'); } elsif ( $self->{dbh} diff --git a/lib/PerconaTest.pm b/lib/PerconaTest.pm index b5638e8ec..124b861a0 100644 --- a/lib/PerconaTest.pm +++ b/lib/PerconaTest.pm @@ -52,6 +52,7 @@ our @EXPORT = qw( load_file slurp_file parse_file + touch_file wait_until wait_for wait_until_replica_running @@ -261,6 +262,13 @@ sub parse_file { return \@e; } +# Create empty file +sub touch_file { + my ($file) = @_; + open(my $t, '>', $file) or die "Cannot create $file: $!"; + close($t) or die "Cannot close $file: $!"; +} + # Wait until code returns true. sub wait_until { my ( $code, $t, $max_t ) = @_; diff --git a/t/lib/Cxn.t b/t/lib/Cxn.t index c02dc9c03..da636bff9 100644 --- a/t/lib/Cxn.t +++ b/t/lib/Cxn.t @@ -63,7 +63,7 @@ sub test_var_val { my ($dbh, $var, $val, %args) = @_; my @row; - if ( !$args{user_var} ) { + if ( !$args{user_var} ) { my $sql = "SHOW " . ($args{global} ? "GLOBAL" : "SESSION " ) . "VARIABLES LIKE '$var'"; @row = $dbh->selectrow_array($sql); @@ -271,7 +271,7 @@ is_deeply( $o->get_opts(); # ############################################################################# -# The parent of a forked Cxn should not disconnect the dbh in DESTORY +# The preserve_dbh of a forked Cxn should not disconnect the dbh in DESTORY # because the child still has access to it. # ############################################################################# @@ -281,8 +281,8 @@ my $outfile = "/tmp/pt-cxn-outfile.$PID"; my $pid; { my $parent_cxn = make_cxn( - dsn_string => 'h=127.1,P=12345,u=msandbox,p=msandbox', - parent => 1, + dsn_string => 'h=127.1,P=12345,u=msandbox,p=msandbox', + preserve_dbh => 1, ); $parent_cxn->connect(); @@ -292,7 +292,7 @@ my $pid; # Wait for the parent to leave this code block which will cause # the $parent_cxn to be destroyed. PerconaTest::wait_for_files($sync_file); - $parent_cxn->{parent} = 0; + $parent_cxn->{preserve_dbh} = 0; eval { $parent_cxn->dbh->do("SELECT 123 INTO OUTFILE '$outfile'"); $parent_cxn->dbh->disconnect(); @@ -305,7 +305,7 @@ my $pid; # Let the child know that we (the parent) have left that ^ code block, # so our copy of $parent_cxn has been destroyed, but hopefully the child's # copy is still alive, i.e. has an active/not-disconnected dbh. -diag(`touch $sync_file`); +PerconaTest::touch_file($sync_file); # Wait for the child. waitpid($pid, 0); @@ -315,7 +315,7 @@ ok( "Child created outfile" ); -my $output = `cat $outfile 2>/dev/null`; +my $output = PerconaTest::slurp_file($outfile); is( $output, @@ -326,6 +326,62 @@ is( unlink $sync_file if -f $sync_file; unlink $outfile if -f $outfile; +# ############################################################################# +# Ensure connection is removed even if parent id is provided but not preserve_dbh +# ############################################################################# + +my $children_sync_file = "/tmp/pt-cxn-children-sync.$PID"; +my $children_ping_file = "/tmp/pt-cxn-children-ping.$PID"; + +{ + + my $parent_cxn = make_cxn( + dsn_string => 'h=127.1,P=12346,u=msandbox,p=msandbox', + parent => 'h=127.1,P=12345,u=msandbox,p=msandbox' # fake parent id + ); + $parent_cxn->connect(); + $pid = fork(); + + if ( defined($pid) && $pid == 0 ) { + # I am the child. + # Wait for the parent to leave this code block which will cause + # the $parent_cxn to be destroyed. + open(my $fh, '>', $children_ping_file) or die "Cannot open $children_ping_file: $!"; + print($fh "Children ping: " . ($parent_cxn->dbh->ping() ? "OK": "FAIL") . "\n"); + + PerconaTest::touch_file($children_sync_file); + + # we wait parent exit this code-block so DESTROY actually disconnects the session + PerconaTest::wait_for_files($sync_file); + print($fh "Children ping after parent exited: " . ($parent_cxn->dbh->ping() ? "OK": "FAIL") . "\n"); + close($fh) or die "Cannot close $children_ping_file: $!"; + exit; + } + PerconaTest::wait_for_files($children_sync_file); # wait confirmation that children conn was ok + ok($parent_cxn->dbh->ping(), 'Parent connection was fine'); +} +# Let the child know that we (the parent) have left that ^ code block, +# so our copy of $parent_cxn has been destroyed, but hopefully the child's +# copy is still alive, i.e. has an active/not-disconnected dbh. +PerconaTest::touch_file($sync_file); + +# Wait for the child. +waitpid($pid, 0); + +my $children_output = PerconaTest::slurp_file($children_ping_file); + +is( + $children_output, + "Children ping: OK\nChildren ping after parent exited: FAIL\n", + 'Expected children to succeed on first ping but fail when parent exited' +); + + +unlink $sync_file if -f $sync_file; +unlink $children_sync_file if -f $children_sync_file; +unlink $children_ping_file if -f $children_ping_file; + + # ############################################################################# # Re-connect with new DSN. # ############################################################################# diff --git a/t/lib/MasterSlave.t b/t/lib/MasterSlave.t index 2eee175f8..0243471db 100644 --- a/t/lib/MasterSlave.t +++ b/t/lib/MasterSlave.t @@ -56,7 +56,7 @@ PXC_SKIP: { local @ARGV = (); $o->get_opts(); - + my $slaves = $ms->get_replicas( dbh => $master_dbh, dsn => $master_dsn, @@ -133,7 +133,7 @@ PXC_SKIP: { # Create percona.checksums to make the privs happy. diag(`/tmp/12345/use -e "create database if not exists percona"`); diag(`/tmp/12345/use -e "create table if not exists percona.checksums (id int)"`); - + # Create a read-only checksum user that can't SHOW SLAVES HOSTS or much else. diag(`/tmp/12345/use -u root < $trunk/t/lib/samples/ro-checksum-user.sql`); @@ -205,7 +205,135 @@ PXC_SKIP: { ); $ro_dbh->disconnect(); - diag(`/tmp/12345/use -u root -e "drop user 'ro_checksum_user'\@'%'"`); + diag(`/tmp/12345/use -u root -e "drop user 'ro_checksum_user'\@'%'"`); + + # ########################################################################## + # Test that get_replicas doesn't leak connections when called multiple times + # This verifies that Cxn objects without preserve_dbh properly clean up + # ########################################################################## + + @ARGV = ('--recursion-method', 'hosts'); + $o->get_opts(); + + # Track connection thread IDs we create so we can verify cleanup + my @disposable_thread_ids; + my $replicas = $ms->get_replicas( + dbh => $master_dbh, + dsn => $master_dsn, + make_cxn => sub { + my $cxn = new Cxn( + @_, + DSNParser => $dp, + OptionParser => $o, + preserve_dbh => 1, # there will be a fork in this test, we need this persistent + ); + $cxn->connect(); + return $cxn; + }, + ); + + # Call get_replicas multiple times in a loop + # Each iteration should create Cxn objects and then clean them up + # As soon as object loses reference, perl should DESTROY Cxn and related connections + for my $iteration (1..5) { + my $tmp_replicas = $ms->get_replicas( + dbh => $master_dbh, + dsn => $master_dsn, + make_cxn => sub { + my $cxn = new Cxn( + @_, + DSNParser => $dp, + OptionParser => $o, + # NOT setting preserve_dbh, so connections should be cleaned up + ); + $cxn->connect(); + return $cxn; + }, + ); + + # Verify we got replicas + ok( + scalar(@$tmp_replicas) > 0, + "get_replicas iteration $iteration returned replicas" + ); + + # Collect thread IDs from the replica connections + foreach my $cxn (@$tmp_replicas) { + if ($cxn && $cxn->dbh) { + my $thread_id = $cxn->dbh->{mysql_thread_id}; + push @disposable_thread_ids, $thread_id; + } + } + } + + my $placeholders = join(',', map { '?' } @disposable_thread_ids); + for my $cxn (@$replicas) { + my ($leaked_connections,) = $cxn->dbh->selectrow_array( + "SELECT COUNT(*) + FROM information_schema.processlist + WHERE id IN ($placeholders)", + { Slice => {} }, + @disposable_thread_ids + ); + + ok($leaked_connections eq 0, 'No remaining connection from get_replicas'); + } + + # Test the opposite now: with preserve_dbh=1, read-replicas should not close connection when going out of scope + # like in a fork scenario + my @preserved_thread_ids; + { + my $replicas_preserved = $ms->get_replicas( + dbh => $master_dbh, + dsn => $master_dsn, + make_cxn => sub { + my $cxn = new Cxn( + @_, + DSNParser => $dp, + OptionParser => $o, + preserve_dbh => 1, + ); + $cxn->connect(); + return $cxn; + }, + ); + + my $pid = fork(); + if ( defined($pid) && $pid == 0 ) { + # We are in the child process + # this would trigger destruction of read replicas in child process + # as preserve_dbh was set to 1, the connection will remain + exit; + } + for my $cxn (@$replicas_preserved) { + if ($cxn && $cxn->dbh) { + push @preserved_thread_ids, $cxn->dbh->{mysql_thread_id}; + } + } + } + + $placeholders = join(',', map { '?' } @preserved_thread_ids); + for my $cxn (@$replicas) { + $cxn->{preserve_dbh} = 0; # enable deletion of our probe replica connection + my $leaked_connections = $cxn->dbh->selectall_arrayref( + "SELECT ID + FROM information_schema.processlist + WHERE id IN ($placeholders)", + { Slice => {} }, + @preserved_thread_ids + ); + + my $count_leaked_conn = scalar @$leaked_connections; + ok($count_leaked_conn > 0, "Had $count_leaked_conn remaining connection from get_replicas"); + + # Test done, let's now kill connections to prevent a leak in the test database + for my $thread (@$leaked_connections) { + eval { + $cxn->dbh->do("KILL $thread->{id}"); + }; + warn "Failed to kill thread $thread->{id}: $@" if $@; + } + } } } # #############################################################################