pt-kill: Changes as per Daniel's review.

bin/pt-kill

@@ -1216,7 +1216,7 @@ sub parse {
    }
 
    foreach my $key ( keys %given_props ) {
-      die "Unknown DSN option '$key' in '$dsn'.  For more details, "
+      die "DSN option '$key' in '$dsn'.  For more details, "
             . "please use the --help option, or try 'perldoc $PROGRAM_NAME' "
             . "for complete documentation."
          unless exists $opts->{$key};
@@ -3125,6 +3125,125 @@ sub _d {
 # End MasterSlave package
 # ###########################################################################
 
+# ###########################################################################
+# Quoter package
+# This package is a copy without comments from the original.  The original
+# with comments and its test file can be found in the Bazaar repository at,
+#   lib/Quoter.pm
+#   t/lib/Quoter.t
+# See https://launchpad.net/percona-toolkit for more information.
+# ###########################################################################
+{
+package Quoter;
+
+use strict;
+use warnings FATAL => 'all';
+use English qw(-no_match_vars);
+use constant PTDEBUG => $ENV{PTDEBUG} || 0;
+
+sub new {
+   my ( $class, %args ) = @_;
+   return bless {}, $class;
+}
+
+sub quote {
+   my ( $self, @vals ) = @_;
+   foreach my $val ( @vals ) {
+      $val =~ s/`/``/g;
+   }
+   return join('.', map { '`' . $_ . '`' } @vals);
+}
+
+sub quote_val {
+   my ( $self, $val ) = @_;
+
+   return 'NULL' unless defined $val;          # undef = NULL
+   return "''" if $val eq '';                  # blank string = ''
+   return $val if $val =~ m/^0x[0-9a-fA-F]+$/;  # hex data
+
+   $val =~ s/(['\\])/\\$1/g;
+   return "'$val'";
+}
+
+sub split_unquote {
+   my ( $self, $db_tbl, $default_db ) = @_;
+   $db_tbl =~ s/`//g;
+   my ( $db, $tbl ) = split(/[.]/, $db_tbl);
+   if ( !$tbl ) {
+      $tbl = $db;
+      $db  = $default_db;
+   }
+   return ($db, $tbl);
+}
+
+sub literal_like {
+   my ( $self, $like ) = @_;
+   return unless $like;
+   $like =~ s/([%_])/\\$1/g;
+   return "'$like'";
+}
+
+sub join_quote {
+   my ( $self, $default_db, $db_tbl ) = @_;
+   return unless $db_tbl;
+   my ($db, $tbl) = split(/[.]/, $db_tbl);
+   if ( !$tbl ) {
+      $tbl = $db;
+      $db  = $default_db;
+   }
+   $db  = "`$db`"  if $db  && $db  !~ m/^`/;
+   $tbl = "`$tbl`" if $tbl && $tbl !~ m/^`/;
+   return $db ? "$db.$tbl" : $tbl;
+}
+
+sub serialize_list {
+   my ( $self, @args ) = @_;
+   return unless @args;
+
+   return $args[0] if @args == 1 && !defined $args[0];
+
+   die "Cannot serialize multiple values with undef/NULL"
+      if grep { !defined $_ } @args;
+
+   return join ',', map { quotemeta } @args;
+}
+
+sub deserialize_list {
+   my ( $self, $string ) = @_;
+   return $string unless defined $string;
+   my @escaped_parts = $string =~ /
+         \G             # Start of string, or end of previous match.
+         (              # Each of these is an element in the original list.
+            [^\\,]*     # Anything not a backslash or a comma
+            (?:         # When we get here, we found one of the above.
+               \\.      # A backslash followed by something so we can continue
+               [^\\,]*  # Same as above.
+            )*          # Repeat zero of more times.
+         )
+         ,              # Comma dividing elements
+      /sxgc;
+
+   push @escaped_parts, pos($string) ? substr( $string, pos($string) ) : $string;
+
+   my @unescaped_parts = map {
+      my $part = $_;
+
+      my $char_class = utf8::is_utf8($part)  # If it's a UTF-8 string,
+                     ? qr/(?=\p{ASCII})\W/   # We only care about non-word
+                     : qr/(?=\p{ASCII})\W|[\x{80}-\x{FF}]/; # Otherwise,
+      $part =~ s/\\($char_class)/$1/g;
+      $part;
+   } @escaped_parts;
+
+   return @unescaped_parts;
+}
+
+1;
+}
+# ###########################################################################
+# End Quoter package
+# ###########################################################################
+
 # ###########################################################################
 # QueryRewriter package
 # This package is a copy without comments from the original.  The original
@@ -3665,38 +3784,57 @@ sub main {
       };
    }
 
-   my ($log_dbh, $log_sth, @processlist_columns);
+   my ($log_sth, @processlist_columns);
 
    if ( my $log_dsn = $o->get('log-dsn') ) {
-      my $db    = $log_dsn->{d};
+      my $db    = $log_dsn->{D};
       my $table = $log_dsn->{t};
-      die "The DSN passed in for --log-dsn must have a database and table set"
+      die "--log-dsn does not specify a database (D) "
+        . "or a database-qualified table (t)"
          unless defined $table && defined $db;
-      $log_dbh = get_cxn($dp, $log_dsn, 0);
+      my $log_dbh = get_cxn($dp, $log_dsn, 0);
+      my $quoted_db = Quoter->quote($db);
+      my $log_table = Quoter->quote($db, $table);
 
-      my $sql = $o->read_para_after(
+      my $sql          = "SHOW TABLES FROM $quoted_db LIKE "
-         __FILE__, qr/MAGIC_create_log_table/);
+                       . Quoter->quote_val($table);
-      $sql =~ s/kill_log/IF NOT EXISTS `$db`.`$table`/;
+      PTDEBUG && _d($sql);
+      my @tables = $log_dbh->selectrow_array($sql);
 
       # Create the log-table table if desired
-      if ( $o->get('create-log-table') ) {
+      if ( @tables == 0 ) {
+         if ($o->get('create-log-table') ) {
+         $sql = $o->read_para_after(
+            __FILE__, qr/MAGIC_create_log_table/);
+         $sql =~ s/kill_log/IF NOT EXISTS $log_table/;
          PTDEBUG && _d($sql);
          $log_dbh->do($sql);
+         }
+         else {
+            die "--log-dsn table does not exist. Please create it or specify "
+            . "--create-log-table.";
+         }
       }
 
-      my $sth = $log_dbh->prepare("SHOW COLUMNS FROM `$db`.`$table`");
+      my @all_log_columns = qw(
-      $sth->execute();
+         server_id timestamp reason kill_error Id User Host
-      my (undef, @all_log_columns) = map @{$_}, @{$sth->fetchall_arrayref([0])};
+         db Command Time State Info Time_ms
-      $sth->finish();
-
-      local $LIST_SEPARATOR = ", ";
-      $log_sth = $log_dbh->prepare(
-         "INSERT INTO `$db`.`$table` (@all_log_columns) VALUES("
-       . join(", ", map { "?" } @all_log_columns)
-       . ")"
       );
+      # Grab the columns that come from processlist
+      @processlist_columns = @all_log_columns[4 .. $#all_log_columns];
 
-      @processlist_columns = @all_log_columns[3 .. $#all_log_columns];
+      $sql               = 'SELECT @@SERVER_ID';
+      PTDEBUG && _d($sql);
+      my ($server_id)    = $dbh->selectrow_array($sql);
+
+      $sql = do {
+         local $LIST_SEPARATOR = ", ";
+         "INSERT INTO $log_table (@all_log_columns) VALUES("
+         . join(", ", $server_id, ("?") x (@all_log_columns-1))
+         . ")"
+      };
+      PTDEBUG && _d($sql);
+      $log_sth = $log_dbh->prepare( $sql );
    }
 
    # ########################################################################
@@ -3902,19 +4040,16 @@ sub main {
                      . " seconds before kill");
                   sleep $o->get('wait-before-kill');
                }
+               local $@;
                eval { $kill_sth->execute($query->{Id}); };
                if ( $EVAL_ERROR ) {
+                  log_to_table($log_sth, $query, $pl, \@processlist_columns, $EVAL_ERROR)
+                     if $log_sth;
                   msg("Error killing $query->{Id}: $EVAL_ERROR");
                }
                else {
-                  if ( $log_sth ) {
+                  log_to_table($log_sth, $query, $pl, \@processlist_columns, $EVAL_ERROR)
-                     my $sql     = 'SELECT @@SERVER_ID';
+                     if $log_sth;
-                     PTDEBUG && _d($sql);
-                     my ($id)    = $dbh->selectrow_array($sql);
-                     my $ts      = Transformers::ts(localtime);
-                     my $reasons = join "\n", @{ $pl->{_reasons_for_matching}->{$query} };
-                     $log_sth->execute($id, $ts, $reasons, @{$query}{@processlist_columns} );
-                  }
                   msg("Killed $query->{Id}");
                }
             }
@@ -3988,6 +4123,16 @@ sub msg {
    return;
 }
 
+sub log_to_table {
+   my ($log_sth, $query, $pl, $processlist_columns, $error) = @_;
+
+   my $ts      = Transformers::ts(localtime);
+   my $reasons = join "\n", map {
+         defined($_) ? $_ : "Unkown reason"
+      } @{ $pl->{_reasons_for_matching}->{$query} };
+   $log_sth->execute($ts, $reasons, $error, @{$query}{@$processlist_columns} );
+}
+
 sub group_queries {
    my ( %args ) = @_;
    my ($proclist, $group_by, $qr) = @args{qw(proclist group_by QueryRewriter)};
@@ -4321,7 +4466,8 @@ type: DSN
 
 Store each query killed in this DSN.
 
-The argument specifies a table to store all killed queries.  The
+The argument specifies a table to store all killed queries.  The DSN
+passed in must have the databse (D) and table (t) options. The
 table must have at least the following columns.  You can add more columns for
 your own special purposes, but they won't be used by pt-kill.  The
 following CREATE TABLE definition is also used for L<"--create-log-table">.
@@ -4332,6 +4478,7 @@ MAGIC_create_log_table:
       server_id   bigint(4) NOT NULL DEFAULT '0',
       timestamp   DATETIME,
       reason      TEXT,
+      kill_error  TEXT,
       Id          bigint(4) NOT NULL DEFAULT '0',
       User        varchar(16) NOT NULL DEFAULT '',
       Host        varchar(64) NOT NULL DEFAULT '',
@@ -4853,10 +5000,6 @@ User for login if not current user.
 
 Table to log actions in, if passed through --log-dsn.
 
-=item * d
-
-Database to log actions in, if passed through --log-dsn.
-
 =back
 
 =head1 ENVIRONMENT

t/pt-kill/kill.t

@@ -139,7 +139,7 @@ $dbh->do($sql);
    eval {
       pt_kill::main('-F', $cnf, qw(--kill --run-time 1 --interval 1),
          "--match-info", 'select sleep\(4\)',
-         "--log-dsn", q!h=127.1,P=12345,u=msandbox,p=msandbox,d=kill_test,t=log_table!,
+         "--log-dsn", q!h=127.1,P=12345,u=msandbox,p=msandbox,D=kill_test,t=log_table!,
       )
    };
    ok !$@, "--log-dsn works if the table exists and --create-log-table wasn't passed in."
@@ -164,9 +164,9 @@ $dbh->do($sql);
    }
 
    my $result = shift @$results;
-   $result->[6] =~ s/localhost:[0-9]+/localhost/;
+   $result->[7] =~ s/localhost:[0-9]+/localhost/;
    is_deeply(
-      [ @{$result}[5..8, 10, 11] ],
+      [ @{$result}[6..9, 11, 12] ],
       [ 'msandbox', 'localhost', undef, 'Query', 'User sleep', 'select sleep(4)', ],
       "...and was populated as expected",
    );
@@ -177,7 +177,7 @@ $dbh->do($sql);
    eval {
       pt_kill::main('-F', $cnf, qw(--kill --run-time 1 --interval 1 --create-log-table),
          "--match-info", 'select sleep\(4\)',
-         "--log-dsn", q!h=127.1,P=12345,u=msandbox,p=msandbox,d=kill_test,t=log_table!,
+         "--log-dsn", q!h=127.1,P=12345,u=msandbox,p=msandbox,D=kill_test,t=log_table!,
       )
    };
    ok !$@, "--log-dsn works if the table exists and --create-log-table was passed in.";
@@ -192,7 +192,7 @@ $dbh->do($sql);
    eval {
       pt_kill::main('-F', $cnf, qw(--kill --run-time 1 --interval 1 --create-log-table),
          "--match-info", 'select sleep\(4\)',
-         "--log-dsn", q!h=127.1,P=12345,u=msandbox,p=msandbox,d=kill_test,t=log_table!,
+         "--log-dsn", q!h=127.1,P=12345,u=msandbox,p=msandbox,D=kill_test,t=log_table!,
       )
    };
    ok !$@, "--log-dsn works if the table doesn't exists and --create-log-table was passed in.";
@@ -205,17 +205,17 @@ $dbh->do($sql);
    eval {
       pt_kill::main('-F', $cnf, qw(--kill --run-time 1 --interval 1),
          "--match-info", 'select sleep\(4\)',
-         "--log-dsn", q!h=127.1,P=12345,u=msandbox,p=msandbox,d=kill_test,t=log_table!,
+         "--log-dsn", q!h=127.1,P=12345,u=msandbox,p=msandbox,D=kill_test,t=log_table!,
       )
    };
    like $@,
-      qr/\QTable 'kill_test.log_table' doesn't exist\E/,       #'
+      qr/\Q--log-dsn table does not exist. Please create it or specify\E/,
       "By default, --log-dsn doesn't autogenerate a table";
 }
 
 for my $dsn (
    q!h=127.1,P=12345,u=msandbox,p=msandbox,t=log_table!,
-   q!h=127.1,P=12345,u=msandbox,p=msandbox,d=kill_test!,
+   q!h=127.1,P=12345,u=msandbox,p=msandbox,D=kill_test!,
    q!h=127.1,P=12345,u=msandbox,p=msandbox!,
 ) {
    local $@;
@@ -226,8 +226,8 @@ for my $dsn (
       )
    };
    like $@,
-      qr/\QThe DSN passed in for --log-dsn must have a database and table set\E/,
+      qr/\Q--log-dsn does not specify a database (D) or a database-qualified table (t)\E/,
-      "--log-dsn croaks if t= or d= are absent";
+      "--log-dsn croaks if t= or D= are absent";
 }
 
 # Run it twice
@@ -236,7 +236,7 @@ for (1,2) {
    sleep 0.5;
    pt_kill::main('-F', $cnf, qw(--kill --run-time 1 --interval 1 --create-log-table),
       "--match-info", 'select sleep\(4\)',
-      "--log-dsn", q!h=127.1,P=12345,u=msandbox,p=msandbox,d=kill_test,t=log_table!,
+      "--log-dsn", q!h=127.1,P=12345,u=msandbox,p=msandbox,D=kill_test,t=log_table!,
    );
 }