diff --git a/bin/pt-table-checksum b/bin/pt-table-checksum index 7af47372..970562a4 100755 --- a/bin/pt-table-checksum +++ b/bin/pt-table-checksum @@ -1417,18 +1417,19 @@ sub new { foreach my $arg ( @required_args ) { die "I need a $arg argument" unless $args{$arg}; }; - die "I need a dsn or dsn_string argument" - unless $args{dsn} || $args{dsn_string}; my ($dp, $o) = @args{@required_args}; + my $dsn_defaults = $dp->parse_options($o); + my $dsn = $args{dsn}; if ( !$dsn ) { + $args{dsn_string} ||= 'h=' . ($dsn_defaults->{h} || 'localhost'); + $dsn = $dp->parse( - $args{dsn_string}, $args{prev_dsn}, $dp->parse_options($o)); + $args{dsn_string}, $args{prev_dsn}, $dsn_defaults); } my $self = { - dsn_string => $args{dsn_string}, dsn => $dsn, dbh => $args{dbh}, set => $args{set}, @@ -7073,6 +7074,12 @@ group: Help Show help and exit. +=item --host + +short form: -h; type: string; default: localhost; group: Connection + +Host to connect to. + =item --ignore-columns type: Hash; group: Filter @@ -7466,21 +7473,15 @@ dsn: charset; copy: yes Default character set. -=item * D - -dsn: database; copy: yes - -Default database. - =item * F -dsn: mysql_read_default_file; copy: yes +dsn: mysql_read_default_file; copy: no Only read default options from the given file =item * h -dsn: host; copy: yes +dsn: host; copy: no Connect to host. @@ -7498,16 +7499,10 @@ Port number to use for connection. =item * S -dsn: mysql_socket; copy: yes +dsn: mysql_socket; copy: no Socket file to use for connection. -=item * t - -dsn: table; copy: no - -Table for DSN L<"--recursion-method">. - =item * u dsn: user; copy: yes diff --git a/lib/Cxn.pm b/lib/Cxn.pm index 7161465d..5b517bfa 100644 --- a/lib/Cxn.pm +++ b/lib/Cxn.pm @@ -57,18 +57,29 @@ sub new { foreach my $arg ( @required_args ) { die "I need a $arg argument" unless $args{$arg}; }; - die "I need a dsn or dsn_string argument" - unless $args{dsn} || $args{dsn_string}; my ($dp, $o) = @args{@required_args}; + # Any tool that connects to MySQL should have a standard set of + # connection options like --host, --port, --user, etc. These + # are default values; they're used in the DSN if the DSN doesn't + # explicate the corresponding part (h=--host, P=--port, etc.). + my $dsn_defaults = $dp->parse_options($o); + my $dsn = $args{dsn}; if ( !$dsn ) { + # If there's no DSN and no DSN string, then the user probably ran + # the tool without specifying a DSN or any default connection options. + # They're probably relying on DBI/DBD::mysql to do the right thing + # by connecting to localhost. On many systems, connecting just to + # localhost causes DBI to use a built-in socket, i.e. it doesn't + # always equate to 'h=127.0.0.1,P=3306'. + $args{dsn_string} ||= 'h=' . ($dsn_defaults->{h} || 'localhost'); + $dsn = $dp->parse( - $args{dsn_string}, $args{prev_dsn}, $dp->parse_options($o)); + $args{dsn_string}, $args{prev_dsn}, $dsn_defaults); } my $self = { - dsn_string => $args{dsn_string}, dsn => $dsn, dbh => $args{dbh}, set => $args{set}, diff --git a/t/lib/Cxn.t b/t/lib/Cxn.t index 95e814f3..93b8040b 100644 --- a/t/lib/Cxn.t +++ b/t/lib/Cxn.t @@ -9,7 +9,7 @@ BEGIN { use strict; use warnings FATAL => 'all'; use English qw(-no_match_vars); -use Test::More tests => 14; +use Test::More tests => 16; use Sandbox; use OptionParser; @@ -36,6 +36,7 @@ $dp->prop('set-vars', $o->get('set-vars')); sub make_cxn { my (%args) = @_; + $o->get_opts(); return new Cxn( OptionParser => $o, DSNParser => $dp, @@ -197,6 +198,50 @@ is_deeply( "cxn->dsn()" ); +# ############################################################################ +# Default cxn, should be equivalent to 'h=localhost'. +# ############################################################################ +my $default_cxn = make_cxn(); +is_deeply( + $default_cxn->dsn(), + { + h => 'localhost', + P => undef, + u => undef, + p => undef, + A => undef, + F => undef, + S => undef, + D => undef, + t => undef, + n => 'h=localhost', + }, + "Defaults to h=localhost" +); + +# But now test if it will inherit just a few standard connection options. +@ARGV = qw(--port 12345); +$default_cxn = make_cxn(); +is_deeply( + $default_cxn->dsn(), + { + h => 'localhost', + P => '12345', + u => undef, + p => undef, + A => undef, + F => undef, + S => undef, + D => undef, + t => undef, + n => 'h=localhost,P=12345', + }, + "Default cxn inherits default connection options" +); + +@ARGV = (); +$o->get_opts(); + # ############################################################################# # Done. # ############################################################################# diff --git a/t/pt-table-checksum/issue_947.t b/t/pt-table-checksum/issue_947.t deleted file mode 100644 index 6b273440..00000000 --- a/t/pt-table-checksum/issue_947.t +++ /dev/null @@ -1,49 +0,0 @@ -#!/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 Test::More; - -use PerconaTest; -use Sandbox; -require "$trunk/bin/pt-table-checksum"; - -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'; -} -else { - plan tests => 1; -} - -my $output; -my $cnf = '/tmp/12345/my.sandbox.cnf'; - -# ############################################################################# -# Issue 947: mk-table-checksum crashes if h DSN part is not given -# ############################################################################# - -$output = output( - sub { pt_table_checksum::main("F=$cnf", qw(-d mysql -t user)) }, - stderr => 1, -); -like( - $output, - qr/mysql\s+user\s+0/, - "Doesn't crash if no h DSN part (issue 947)" -); - -# ############################################################################# -# Done. -# ############################################################################# -exit; diff --git a/t/pt-table-checksum/standard_options.t b/t/pt-table-checksum/standard_options.t index f60e7635..e4a31806 100644 --- a/t/pt-table-checksum/standard_options.t +++ b/t/pt-table-checksum/standard_options.t @@ -9,32 +9,90 @@ BEGIN { use strict; use warnings FATAL => 'all'; use English qw(-no_match_vars); -use Test::More tests => 2; +use Test::More; use PerconaTest; +use Sandbox; require "$trunk/bin/pt-table-checksum"; +my $dp = new DSNParser(opts=>$dsn_opts); +my $sb = new Sandbox(basedir => '/tmp', DSNParser => $dp); +my $master_dbh = $sb->get_dbh_for('master'); +my $slave_dbh = $sb->get_dbh_for('slave1'); + +if ( !$master_dbh ) { + plan skip_all => 'Cannot connect to sandbox master'; +} +elsif ( !$slave_dbh ) { + plan skip_all => 'Cannot connect to sandbox slave1'; +} +else { + plan tests => 4; +} + +# 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. +my @args = (qw(--lock-wait-timeout 3 --explain --tables sakila.country)); +my $cnf = "/tmp/12345/my.sandbox.cnf"; +my $pid_file = "/tmp/mk-table-checksum-test.pid"; my $output; -# Test DSN value inheritance -$output = `$trunk/bin/pt-table-checksum h=127.1 h=127.2,P=12346 --port 12345 --explain-hosts`; +# ############################################################################ +# DSN should inherit connection options (--port, etc.) +# ############################################################################ + +$output = output( + sub { pt_table_checksum::main(@args, 'h=127.1', + qw(--port 12345 --user msandbox --password msandbox)) }, +); like( $output, - qr/^Server 127.1:\s+P=12345,h=127.1\s+Server 127.2:\s+P=12346,h=127.2/, - 'DSNs inherit values from --port, etc. (issue 248)' + qr/-- sakila\.country/, + 'DSN inherits values from --port, etc. (issue 248)' +); + +# Same test but this time intentionally use the wrong port so the connection +# fails so we know that the previous test didn't work because your system +# has a .my.cnf file or something. +eval { + pt_table_checksum::main(@args, 'h=127.1', + qw(--port 4 --user msandbox --password msandbox)); +}; +like( + $EVAL_ERROR, + qr/port=4.+failed/, + 'DSN truly inherits values from --port, etc. (issue 248)' +); + +# ############################################################################# +# Issue 947: mk-table-checksum crashes if h DSN part is not given +# ############################################################################# + +$output = output( + sub { pt_table_checksum::main(@args, "F=$cnf") }, +); +like( + $output, + qr/-- sakila\.country/, + "Doesn't crash if no h DSN part (issue 947)" ); # ######################################################################### # Issue 391: Add --pid option to all scripts # ######################################################################### -`touch /tmp/mk-script.pid`; -$output = `$trunk/bin/pt-table-checksum h=127.1,P=12345,u=msandbox,p=msandbox -d test -t issue_122,issue_94 --pid /tmp/mk-script.pid 2>&1`; +diag(`rm -rf $pid_file >/dev/null 2>&1`); +diag(`touch $pid_file`); + +eval { + pt_table_checksum::main(@args, $cnf, '--pid', $pid_file); +}; like( - $output, - qr{PID file /tmp/mk-script.pid already exists}, + $EVAL_ERROR, + qr/PID file $pid_file already exists/, 'Dies if PID file already exists (issue 391)' ); -`rm -rf /tmp/mk-script.pid`; + +diag(`rm -rf $pid_file >/dev/null 2>&1`); # ############################################################################# # Done.