Merge lp:~percona-toolkit-dev/percona-toolkit/fix-950294-ptc-if-not-exists into lp:percona-toolkit/2.1

[Brian Fraser]

[Daniel Nichter (daniel-nichter)]

1) "--no-create-replicate-table was passed in." -> ""--no-create-replicate-table was specified."

2)

29 + print STDERR "CREATE DATABASE IF NOT EXISTS $db failed, but the "
30 + . "db already exists. This is harmless unless the "
31 + . "slaves don't have the database. $EVAL_ERROR"; # kb link?

I think we need to make this fatal because we must avoid breaking replication. So:

die "Error executing $sql: $EVAL_ERROR\nThe database exists on the master, but replication will break if it does not also exist on all replicas. If the --replicate database and table already exist on the master and all replicas, or if you create them manually, then specify --no-create-replicate-table to avoid this error."

How does that ^ sound? Better to force the user to think/do a little manual work than break their replication.

3) 49 +<<<<<<< TREE -- why is there a merge conflict?

4) 63 + . "--no-create-replicate-table was passed in. " -- "passed id" -> "specified"

5) 73 + print STDERR "CREATE TABLE IF NOT EXISTS $args{repl_table} " -- Same as above with the db: make fatal, change error message, etc.

6) 142 +isnt($exit_status, 0, "");

i) Let's never use isnt() because 1) it amounts to "yes we have no bananas" ("yes, $got is not $bar") and 2) it's dangerously close to isn't() which is just crazy.

ii) Test has no name.

review: Needs Fixing

[lp:~percona-toolkit-dev/percona-toolkit/fix-950294-ptc-if-not-exists]

333. By Brian Fraser <email address hidden> on 2012-07-31

Resolved a pending merge conflict.

The fix for https://bugs.launchpad.net/percona-toolkit/+bug/916168
was affecting the same code as this fix.
This commit mimicks that fix, and removes the privs checks.

[Daniel Nichter (daniel-nichter)]

review: Approve

=== modified file 'bin/pt-table-checksum'
--- bin/pt-table-checksum	2012-07-30 14:30:05 +0000
+++ bin/pt-table-checksum	2012-07-31 15:30:27 +0000
@@ -8152,17 +8152,38 @@
    my $sql = "SHOW DATABASES LIKE '$db'";
    PTDEBUG && _d($sql);
    my @db_exists = $dbh->selectrow_array($sql);
-   if ( !@db_exists && $o->get('create-replicate-table') ) {
-      $sql = "CREATE DATABASE " . $q->quote($db) . " /* pt-table-checksum */";
+   if ( !@db_exists && !$o->get('create-replicate-table') ) {
+      die "--replicate database $db does not exist and "
+         . "--no-create-replicate-table was specified.  You need "
+         . "to create the database.\n";
+   }
+
+   if ( $o->get('create-replicate-table') ) {
+      $sql = "CREATE DATABASE IF NOT EXISTS "
+            . $q->quote($db)
+            . " /* pt-table-checksum */";
+      local $@;
+      PTDEBUG && _d($sql);
       eval {
          $dbh->do($sql);
       };
       if ( $EVAL_ERROR ) {
-         die "--replicate database $db does not exist and it cannot be "
-            . "created automatically.  You need to create the database.\n";
+         if ( @db_exists ) {
+            die "Error executing $sql: $EVAL_ERROR\n"
+              . "The database exists on the master, but replication will "
+              . "break if it does not also exist on all replicas. If the "
+              . "--replicate database and table already exist on the master "
+              . "and all replicas, or if you created them manually, then "
+              . "specify --no-create-replicate-table to avoid this error."
+         }
+         else {
+            die "--replicate database $db does not exist and it cannot be "
+               . "created automatically.  You need to create the database. "
+               . $EVAL_ERROR;
+         }
       }
    }
-
+   
    # USE the correct db (probably the repl db, but maybe --replicate-database).
    use_repl_db(%args);
 
@@ -8172,20 +8193,47 @@
       db  => $db,
       tbl => $tbl,
    );
+
+   # Always create the table, unless --no-create-replicate-table
+   # was passed in; see https://bugs.launchpad.net/percona-toolkit/+bug/950294
    if ( !$tbl_exists ) {
-      if ( $o->get('create-replicate-table') ) {
-         create_repl_table(%args)
-      }
-      else {
-         die "--replicate table $repl_table does not exist; "
-            . "read the documentation or use --create-replicate-table "
-            . "to create it.\n";
+      if ( !$o->get('create-replicate-table') ) {
+         die "--replicate table $repl_table does not exist and "
+         . "--no-create-replicate-table was specified.  "
+         . "You need to create the table.\n";
       }
    }
    else {
       PTDEBUG && _d('--replicate table', $repl_table, 'already exists');
-      # We used to check the table privs here, but:
-      # https://bugs.launchpad.net/percona-toolkit/+bug/916168
+<<<<<<< TREE
+      # We used to check the table privs here, but:
+      # https://bugs.launchpad.net/percona-toolkit/+bug/916168
+=======
+      # We used to check the table privs here, but:
+      # https://bugs.launchpad.net/percona-toolkit/+bug/916168
+   }
+
+   if ( $o->get('create-replicate-table') ) {
+      local $@;
+      eval {
+         create_repl_table(%args);
+      };
+      if ( $EVAL_ERROR ) {
+         if ( $tbl_exists ) {
+            die "Error executing $sql: $EVAL_ERROR\n"
+              . "The table exists on the master, but replication will "
+              . "break if it does not also exist on all replicas. If the "
+              . "--replicate database and table already exist on the master "
+              . "and all replicas, or if you created them manually, then "
+              . "specify --no-create-replicate-table to avoid this error."
+         }
+         else {
+            die "--replicate table $tbl does not exist and it cannot be "
+               . "created automatically.  You need to create the table. "
+               . $EVAL_ERROR;
+         }
+      }
+>>>>>>> MERGE-SOURCE
    }
 
    # Check and wait for the repl table to appear on all slaves.
@@ -8327,7 +8375,7 @@
    my ($dbh, $repl_table, $o) = @args{@required_args};
    PTDEBUG && _d('Creating --replicate table', $repl_table); 
    my $sql = $o->read_para_after(__FILE__, qr/MAGIC_create_replicate/);
-   $sql =~ s/CREATE TABLE checksums/CREATE TABLE $repl_table/;
+   $sql =~ s/CREATE TABLE checksums/CREATE TABLE IF NOT EXISTS $repl_table/;
    $sql =~ s/;$//;
    PTDEBUG && _d($dbh, $sql);
    eval {
 
=== modified file 't/pt-table-checksum/privs.t'
--- t/pt-table-checksum/privs.t	2012-07-20 23:58:44 +0000
+++ t/pt-table-checksum/privs.t	2012-07-31 15:30:27 +0000
@@ -41,9 +41,12 @@
 elsif ( !@{$master_dbh->selectall_arrayref("show databases like 'sakila'")} ) {
    plan skip_all => 'sakila database is not loaded';
 }
+<<<<<<< TREE
 else {
    plan tests => 4;
 }
+=======
+>>>>>>> MERGE-SOURCE
 
 # The sandbox servers run with lock_wait_timeout=3 and it's not dynamic
 # so we need to specify --lock-wait-timeout=3 else the tool will die.
@@ -55,6 +58,53 @@
 my $sample  = "t/pt-table-checksum/samples/";
 
 # ############################################################################
+# Should always create schema and tables with IF NOT EXISTS
+# https://bugs.launchpad.net/percona-toolkit/+bug/950294
+# ############################################################################
+
+$sb->wipe_clean($master_dbh);
+diag(`/tmp/12345/use -u root < $trunk/t/lib/samples/ro-checksum-user.sql 2>/dev/null`);
+PerconaTest::wait_for_table($slave1_dbh, "mysql.tables_priv", "user='ro_checksum_user'");
+
+($output, $exit_status) = full_output(
+   sub { pt_table_checksum::main(@args,
+      "$master_dsn,u=ro_checksum_user,p=msandbox",
+      qw(--recursion-method none)
+   ) },
+);
+
+ok(
+   $exit_status,
+   "Dies with an error status if it can't create the db/table"
+);
+
+like($output,
+   qr/\Q--replicate database percona does not exist and it cannot be created automatically/,
+   "fails if the percona db doesn't exist and the user can't create it",
+);
+
+($output, $exit_status) = full_output(
+   sub { pt_table_checksum::main(@args,
+      "$master_dsn,u=ro_checksum_user,p=msandbox",
+      qw(--recursion-method none --no-create-replicate-table)
+   ) },
+);
+
+like($output,
+   qr/\Q--replicate database percona does not exist and --no-create-replicate-table was/,
+   "fails if the percona db doesn't exist and --no-create-replicate-table",
+);
+
+diag(`/tmp/12345/use -u root -e "drop user 'ro_checksum_user'\@'%'"`);
+wait_until(
+   sub {
+      my $rows=$slave2_dbh->selectall_arrayref("SELECT user FROM mysql.user");
+      return !grep { ($_->[0] || '') ne 'ro_checksum_user' } @$rows;
+   }
+);
+$sb->wipe_clean($master_dbh);
+
+# ############################################################################
 # --recursion-method=none to avoid SHOW SLAVE HOSTS
 # https://bugs.launchpad.net/percona-toolkit/+bug/987694
 # ############################################################################
@@ -64,7 +114,7 @@
    "$master_dsn,u=msandbox,p=msandbox",
    qw(-t sakila.country --quiet --quiet));
 
-diag(`/tmp/12345/use -u root < $trunk/t/lib/samples/ro-checksum-user.sql`);
+diag(`/tmp/12345/use -u root < $trunk/t/lib/samples/ro-checksum-user.sql 2>/dev/null`);
 PerconaTest::wait_for_table($slave1_dbh, "mysql.tables_priv", "user='ro_checksum_user'");
 
 $output = output(
@@ -73,7 +123,7 @@
       # Comment out this line and the tests fail because ro_checksum_user
       # doesn't have privs to SHOW SLAVE HOSTS.  This proves that
       # --recursion-method none is working.
-      qw(--recursion-method none)
+      qw(--recursion-method none --no-create-replicate-table)
    ) },
    stderr => 1,
 );
@@ -90,6 +140,32 @@
    "Read-only user (bug 987694): checksummed rows"
 );
 
+($output) = full_output(
+   sub { $exit_status = pt_table_checksum::main(@args,
+      "$master_dsn,u=ro_checksum_user,p=msandbox",
+      qw(--recursion-method none)
+   ) }
+);
+
+like($output,
+   qr/\QThe database exists on the master, but replication will break/,
+   "dies if the db exists on the master but it can't CREATE DATABASE and --no-create-replicate-table was not specified",
+);
+
+diag(qx{/tmp/12345/use -u root -e 'DROP TABLE `percona`.`checksums`'});
+
+($output, $exit_status) = full_output(
+   sub { pt_table_checksum::main(@args,
+      "$master_dsn,u=ro_checksum_user,p=msandbox",
+      qw(--recursion-method none --no-create-replicate-table)
+   ) },
+);
+
+like($output,
+   qr/\Q--replicate table `percona`.`checksums` does not exist and --no/,
+   "fails if the checksums db doesn't exist and --no-create-replicate-table"
+);
+
 diag(`/tmp/12345/use -u root -e "drop user 'ro_checksum_user'\@'%'"`);
 wait_until(
    sub {
@@ -127,4 +203,5 @@
 # #############################################################################
 $sb->wipe_clean($master_dbh);
 ok($sb->ok(), "Sandbox servers") or BAIL_OUT(__FILE__ . " broke the sandbox");
-exit;
+
+done_testing;