From 738f9809f8653c2079c9f88a3d373eb01c3c3fd4 Mon Sep 17 00:00:00 2001 From: Carlos Salguero Date: Mon, 4 Sep 2017 12:03:36 -0300 Subject: [PATCH 1/2] PT-167 Still need to add tests Description here --- bin/pt-kill | 30 +++++++++++- bin/pt-query-digest | 14 +++++- lib/Processlist.pm | 17 ++++++- t/pt-kill/pt_167.t | 93 ++++++++++++++++++++++++++++++++++++ t/pt-kill/samples/pt_167.sql | 0 5 files changed, 150 insertions(+), 4 deletions(-) create mode 100644 t/pt-kill/pt_167.t create mode 100644 t/pt-kill/samples/pt_167.sql diff --git a/bin/pt-kill b/bin/pt-kill index 6d1fb47e..67e13b3b 100755 --- a/bin/pt-kill +++ b/bin/pt-kill @@ -3297,6 +3297,17 @@ sub new { foreach my $arg ( qw(MasterSlave) ) { die "I need a $arg argument" unless $args{$arg}; } + my $kill_busy_commands = {}; + if ($args{kill_busy_commands}) { + for my $command (split /,/,$args{kill_busy_commands}) { + $command =~ s/^\s+|\s+$//g; + $kill_busy_commands->{$command} = 1; + } + } else { + $kill_busy_commands->{Query} = 1; + } + $args{kill_busy_commands} = $kill_busy_commands; + my $self = { %args, polls => 0, @@ -3523,7 +3534,7 @@ sub find { next QUERY; } - if ( $find_spec{busy_time} && ($query->{Command} || '') eq 'Query' ) { + if ( $find_spec{busy_time} && exists($self->{kill_busy_commands}->{$query->{Command} || ''}) ) { next QUERY unless defined($query->{Time}); if ( $query->{Time} < $find_spec{busy_time} ) { PTDEBUG && _d("Query isn't running long enough"); @@ -6788,7 +6799,7 @@ sub main { DSNParser => $dp, Quoter => "Quoter", ); - my $pl = new Processlist(MasterSlave => $ms); + my $pl = new Processlist(MasterSlave => $ms, kill_busy_commands => $o->get('kill-busy-commands')); my $qr = new QueryRewriter(); my $cxn; @@ -8198,6 +8209,21 @@ that pt-kill matched and killed a query. See also L<"--wait-before-kill"> and L<"--wait-after-kill">. +=item --kill-busy-commands + +default: Query + +group: Actions + +Comma sepatated list of commands that will be watched/killed if they ran for +more than L<"--busy-time"> seconds. Default: C + +By default, L<"--busy-time"> kills only C commands but in some cases, it +is needed to make L<"--busy-time"> to watch and kill other commands. For example, +a prepared statement execution command is C instead of C. In this +case, specifying C<--kill-busy-commands=Query,Execute> will also kill the prepared +stamente execution. + =item --kill-query group: Actions diff --git a/bin/pt-query-digest b/bin/pt-query-digest index b90f3426..f01767a5 100755 --- a/bin/pt-query-digest +++ b/bin/pt-query-digest @@ -3251,6 +3251,17 @@ sub new { foreach my $arg ( qw(MasterSlave) ) { die "I need a $arg argument" unless $args{$arg}; } + my $kill_busy_commands = {}; + if ($args{kill_busy_commands}) { + for my $command (split /,/,$args{kill_busy_commands}) { + $command =~ s/^\s+|\s+$//g; + $kill_busy_commands->{$command} = 1; + } + } else { + $kill_busy_commands->{Query} = 1; + } + $args{kill_busy_commands} = $kill_busy_commands; + my $self = { %args, polls => 0, @@ -3465,6 +3476,7 @@ sub find { my $ms = $self->{MasterSlave}; my @matches; + $self->{_reasons_for_matching} = undef; QUERY: foreach my $query ( @$proclist ) { PTDEBUG && _d('Checking query', Dumper($query)); @@ -3476,7 +3488,7 @@ sub find { next QUERY; } - if ( $find_spec{busy_time} && ($query->{Command} || '') eq 'Query' ) { + if ( $find_spec{busy_time} && exists($self->{kill_busy_commands}->{$query->{Command} || ''}) ) { next QUERY unless defined($query->{Time}); if ( $query->{Time} < $find_spec{busy_time} ) { PTDEBUG && _d("Query isn't running long enough"); diff --git a/lib/Processlist.pm b/lib/Processlist.pm index 0546b1ce..a953a1c6 100644 --- a/lib/Processlist.pm +++ b/lib/Processlist.pm @@ -69,6 +69,19 @@ sub new { foreach my $arg ( qw(MasterSlave) ) { die "I need a $arg argument" unless $args{$arg}; } + # Convert the list of kill commands (Query, Execute, etc) to a hashref for + # faster check later + my $kill_busy_commands = {}; + if ($args{kill_busy_commands}) { + for my $command (split /,/,$args{kill_busy_commands}) { + $command =~ s/^\s+|\s+$//g; + $kill_busy_commands->{$command} = 1; + } + } else { + $kill_busy_commands->{Query} = 1; + } + $args{kill_busy_commands} = $kill_busy_commands; + my $self = { %args, polls => 0, @@ -471,6 +484,7 @@ sub find { my $ms = $self->{MasterSlave}; my @matches; + $self->{_reasons_for_matching} = undef; QUERY: foreach my $query ( @$proclist ) { PTDEBUG && _d('Checking query', Dumper($query)); @@ -484,7 +498,8 @@ sub find { } # Match special busy_time. - if ( $find_spec{busy_time} && ($query->{Command} || '') eq 'Query' ) { + #if ( $find_spec{busy_time} && ($query->{Command} || '') eq 'Query' ) { + if ( $find_spec{busy_time} && exists($self->{kill_busy_commands}->{$query->{Command} || ''}) ) { next QUERY unless defined($query->{Time}); if ( $query->{Time} < $find_spec{busy_time} ) { PTDEBUG && _d("Query isn't running long enough"); diff --git a/t/pt-kill/pt_167.t b/t/pt-kill/pt_167.t new file mode 100644 index 00000000..494c7283 --- /dev/null +++ b/t/pt-kill/pt_167.t @@ -0,0 +1,93 @@ +#!/usr/bin/env perl + +BEGIN { + die "The PERCONA_TOOLKIT_BRANCH environment variable is not set.\n" + unless $ENV{PERCONA_TOOLKIT_BRANCH} && -d $ENV{PERCONA_TOOLKIT_BRANCH}; + unshift @INC, "$ENV{PERCONA_TOOLKIT_BRANCH}/lib"; +}; + +use strict; +use warnings FATAL => 'all'; +use English qw(-no_match_vars); +use Time::HiRes qw(sleep); +use Test::More; + +use PerconaTest; +use Sandbox; +require "$trunk/bin/pt-kill"; + +use Data::Dumper; +$Data::Dumper::Indent = 1; +$Data::Dumper::Sortkeys = 1; +$Data::Dumper::Quotekeys = 0; + +my $dp = new DSNParser(opts=>$dsn_opts); +my $sb = new Sandbox(basedir => '/tmp', DSNParser => $dp); +my $dbh = $sb->get_dbh_for('master'); + +if ( !$dbh ) { + plan skip_all => 'Cannot connect to sandbox master'; +} + +my $output; +my $dsn = $sb->dsn_for('master'); +my $cnf = '/tmp/12345/my.sandbox.cnf'; + +# ############################################################################# +# Test that --kill-query only kills the query, not the connection. +# ############################################################################# + +# Here's how this works. This cmd is going to try 2 queries on the same +# connection: sleep5 and sleep3. --kill-query will kill sleep5 causing +# sleep3 to start using the same connection id (pid). +system("/tmp/12345/use -e 'select sleep(5); select sleep(2)' >/dev/null&"); +sleep 0.5; + +SKIP: { + + skip "TODO"; + my $iterations=99; + my $query = 'select benchmark(?, md5("when will it end?"))'; + my $ps = $dbh->prepare($query); + $ps->execute($iterations); + my $rows = $dbh->selectall_hashref('show processlist', 'id'); + my $pid = 0; # reuse, reset + map { $pid = $_->{id} } + grep { $_->{info} && $_->{info} =~ m/select sleep\(15\)/ } + values %$rows; + + ok( + $pid, + 'Got proc id of sleeping query' + ); + + $output = output( + sub { pt_kill::main('-F', $cnf, qw(--busy-time 4s --print --match-info "^(select|SELECT)")), }, + ); + like( + $output, + qr/KILL QUERY $pid /, + '--kill-query' + ); + + sleep 1; + $rows = $dbh->selectall_hashref('show processlist', 'id'); + my $con_alive = grep { $_->{id} eq $pid } values %$rows; + ok( + $con_alive, + 'Killed query, not connection' + ); + + is( + ($rows->{$pid}->{info} || ''), + 'select sleep(3)', + 'Connection is still alive' + ); +} + +# ############################################################################# +# Done. +# ############################################################################# +$sb->wipe_clean($dbh); +ok($sb->ok(), "Sandbox servers") or BAIL_OUT(__FILE__ . " broke the sandbox"); +done_testing; diff --git a/t/pt-kill/samples/pt_167.sql b/t/pt-kill/samples/pt_167.sql new file mode 100644 index 00000000..e69de29b From fec1a1f75883a20a96301fd4a2c9d7bb469447d7 Mon Sep 17 00:00:00 2001 From: Carlos Salguero Date: Wed, 6 Sep 2017 16:46:47 -0300 Subject: [PATCH 2/2] PT-167 pt-kill kills prepared statements w/o checking busy-time --- bin/pt-kill | 2 +- t/pt-kill/pt_167.t | 113 ++++++++++++++++++----------------- t/pt-kill/samples/pt_167.sql | 4 ++ 3 files changed, 64 insertions(+), 55 deletions(-) diff --git a/bin/pt-kill b/bin/pt-kill index 67e13b3b..11663760 100755 --- a/bin/pt-kill +++ b/bin/pt-kill @@ -47,7 +47,7 @@ BEGIN { { package Percona::Toolkit; -our $VERSION = '3.0.4'; +our $VERSION = '3.0.5'; use strict; use warnings FATAL => 'all'; diff --git a/t/pt-kill/pt_167.t b/t/pt-kill/pt_167.t index 494c7283..a133cd25 100644 --- a/t/pt-kill/pt_167.t +++ b/t/pt-kill/pt_167.t @@ -11,6 +11,7 @@ use warnings FATAL => 'all'; use English qw(-no_match_vars); use Time::HiRes qw(sleep); use Test::More; +use threads; use PerconaTest; use Sandbox; @@ -21,6 +22,13 @@ $Data::Dumper::Indent = 1; $Data::Dumper::Sortkeys = 1; $Data::Dumper::Quotekeys = 0; + +my $o = new OptionParser(description => 'Diskstats', + file => "$trunk/bin/pt-kill", +); +$o->get_specs("$trunk/bin/pt-table-checksum"); +$o->get_opts(); + my $dp = new DSNParser(opts=>$dsn_opts); my $sb = new Sandbox(basedir => '/tmp', DSNParser => $dp); my $dbh = $sb->get_dbh_for('master'); @@ -29,62 +37,59 @@ if ( !$dbh ) { plan skip_all => 'Cannot connect to sandbox master'; } -my $output; -my $dsn = $sb->dsn_for('master'); -my $cnf = '/tmp/12345/my.sandbox.cnf'; - -# ############################################################################# -# Test that --kill-query only kills the query, not the connection. -# ############################################################################# - -# Here's how this works. This cmd is going to try 2 queries on the same -# connection: sleep5 and sleep3. --kill-query will kill sleep5 causing -# sleep3 to start using the same connection id (pid). -system("/tmp/12345/use -e 'select sleep(5); select sleep(2)' >/dev/null&"); -sleep 0.5; - -SKIP: { - - skip "TODO"; - my $iterations=99; - my $query = 'select benchmark(?, md5("when will it end?"))'; - my $ps = $dbh->prepare($query); - $ps->execute($iterations); - my $rows = $dbh->selectall_hashref('show processlist', 'id'); - my $pid = 0; # reuse, reset - map { $pid = $_->{id} } - grep { $_->{info} && $_->{info} =~ m/select sleep\(15\)/ } - values %$rows; - - ok( - $pid, - 'Got proc id of sleeping query' - ); - - $output = output( - sub { pt_kill::main('-F', $cnf, qw(--busy-time 4s --print --match-info "^(select|SELECT)")), }, - ); - like( - $output, - qr/KILL QUERY $pid /, - '--kill-query' - ); - - sleep 1; - $rows = $dbh->selectall_hashref('show processlist', 'id'); - my $con_alive = grep { $_->{id} eq $pid } values %$rows; - ok( - $con_alive, - 'Killed query, not connection' - ); - - is( - ($rows->{$pid}->{info} || ''), - 'select sleep(3)', - 'Connection is still alive' - ); +$sb->load_file('master', "t/pt-kill/samples/pt_167.sql"); +my $ps = $dbh->prepare('INSERT INTO test.foo VALUES (?)'); +for (my $i=0; $i < 20000; $i++) { + $ps->execute($i); } +sub start_thread { + my ($dp, $o, $dsn, $sleep_time) = @_; + + diag("Thread started"); + + my $cxn = new Cxn(DSNParser => $dp, OptionParser => $o, dsn => $dsn); + $cxn->connect(); + my $dbh = $cxn->dbh(); + my $sth = $dbh->prepare('SELECT COUNT(*) FROM (SELECT a.id AS a_id, b.id AS b_id FROM foo AS a, foo AS b) AS c'); + # Since this query is going to be killed, wrap the execution in an eval to prevent + # displaying the error message. + eval { + $sth->execute(); + }; +} + +my $dsn = "h=127.1,P=12345,u=msandbox,p=msandbox,D=test;mysql_server_prepare=1"; +my $thr = threads->create('start_thread', $dp, $o, $dp->parse($dsn), 1); +$thr->detach(); +threads->yield(); + +sleep(1); + +my $rows = $dbh->selectall_hashref('show processlist', 'id'); +my $pid = 0; # reuse, reset +map { $pid = $_->{id} } +grep { $_->{info} && $_->{info} =~ m/SELECT COUNT\(\*\) FROM \(SELECT a.id/ } +values %$rows; + +ok( + $pid, + "Got proc id of sleeping query: $pid" +); + +$dsn = $sb->dsn_for('master'); +my $output = output( + sub { pt_kill::main($dsn, "--kill-busy-commands","Query,Execute", qw(--run-time 3s --kill --busy-time 2s --print --match-info), "^(select|SELECT)"), }, + stderr => 1, +); + +like( + $output, + qr/KILL $pid \(Execute/, + '--kill-query' +) or diag($output); + + # ############################################################################# # Done. # ############################################################################# diff --git a/t/pt-kill/samples/pt_167.sql b/t/pt-kill/samples/pt_167.sql index e69de29b..0b15466e 100644 --- a/t/pt-kill/samples/pt_167.sql +++ b/t/pt-kill/samples/pt_167.sql @@ -0,0 +1,4 @@ +DROP DATABASE IF EXISTS test; +CREATE DATABASE test; + +create table test.foo (id integer);