Hi, Julius, On Dec 27, Julius Goryavsky wrote:
commit 4d9f8a3c31e Author: Julius Goryavsky <julius.goryavsky@mariadb.com> Date: Mon Dec 19 10:29:55 2022 +0100
diff --git a/mysql-test/mysql-test-run.pl b/mysql-test/mysql-test-run.pl index 12afab4d28c..8df8198fc99 100755 --- a/mysql-test/mysql-test-run.pl +++ b/mysql-test/mysql-test-run.pl @@ -313,7 +313,7 @@ my %mysqld_logs; my $opt_debug_sync_timeout= 300; # Default timeout for WAIT_FOR actions. my $warn_seconds = 60;
-my $rebootstrap_re= '--innodb[-_](?:page[-_]size|checksum[-_]algorithm|undo[-_]tablespaces|log[-_]group[-_]home[-_]dir|data[-_]home[-_]dir)|data[-_]file[-_]path|force_rebootstrap'; +my $rebootstrap_re= '--(?:loose[-_])?(?:innodb[-_](?:page[-_]size|file(?:[-_]format|per[-_]table)|compression[-_](?:default|algorithm)|checksum(?:s|[-_]algorithm)|undo[-_](?:directory|tablespaces)|(?:data|log[-_]group)[-_]home[-_]dir|buffer[-_]pool[-_]filename|data[-_]file[-_]path|default[-_](?:encryption[-_]key[-_]id|page[-_]encryption)|sys[-_]|(?:index|table)[-_]stats)|aria[-_]log[-_](?:dir[-_]path|file[-_]size))|force_rebootstrap';
1. drop force_rebootstrap please 2. this code was already not very readable, but got fairly ridiculous now. Let's change the approach. E.g. put them all in a hash, like my %rebootstrap_check = map { $_ => 1 } qw( innodb-page-size innodb-file-format ... ); you'll need to normalize an option before looking it up in the hash
sub testcase_timeout ($) { return $opt_testcase_timeout * 60; } sub check_timeout ($) { return testcase_timeout($_[0]); } @@ -2762,7 +2762,29 @@ sub mysql_server_start($) { return; }
- my $datadir= $mysqld->value('datadir'); + my $extra_opts= get_extra_opts($mysqld, $tinfo); + + # The test options can contain the --datadir parameter, but + # the bootstrap function can only read datadir location from + # a .cnf file. So we need to parse the options to get the + # actual location of the data directory, considering both + # the options and the .cnf file, and then store the resulting + # value in a "$mysqld" hash - to use later, when we need to + # know the actual location of the data directory: + my $datadir=""; + foreach my $opt ( @$extra_opts ) + { + if ($opt =~ /^--datadir=/) { + $datadir = substr($opt, 10); + last;
strictly speaking, `last` is incorrect, there can be more than one --datadir
+ } + } + # If datadir is not in the options, then read it from .cnf: + if (! $datadir) { + $datadir = $mysqld->value('datadir'); + } + $mysqld->{'datadir'} = $datadir; + if (not $opt_start_dirty) {
@@ -2785,17 +2807,59 @@ sub mysql_server_start($) { } }
+ # Run <tname>-master.sh + if ($mysqld->option('#!run-master-sh') and + defined $tinfo->{master_sh} and + run_system('/bin/sh ' . $tinfo->{master_sh}) ) + { + $tinfo->{'comment'}= "Failed to execute '$tinfo->{master_sh}'"; + return 1; + } + + # Run <tname>-slave.sh + if ($mysqld->option('#!run-slave-sh') and + defined $tinfo->{slave_sh} and + run_system('/bin/sh ' . $tinfo->{slave_sh})) + { + $tinfo->{'comment'}= "Failed to execute '$tinfo->{slave_sh}'"; + return 1; + } + my $mysqld_basedir= $mysqld->value('basedir'); - my $extra_opts= get_extra_opts($mysqld, $tinfo);
if ( $basedir eq $mysqld_basedir ) { if (! $opt_start_dirty) # If dirty, keep possibly grown system db { + # Find the name of the current section in the configuration + # file and its suffix: + my $section = $mysqld->{name}; + my $server_name; + my $suffix = ""; + if (index($section, '.') != -1) { + ($server_name, $suffix) = $section =~ /^\s*([^\s.]+)\s*\.\s*([^\s]+)/; + } + else { + $server_name = $section =~ /^\s*([^\s]+)/; + }
Replace all 11 lines above with: my ($suffix) = ($mysqld->{name} =~ /\.(.*)/);
# Some InnoDB options are incompatible with the default bootstrap. # If they are used, re-bootstrap my @rebootstrap_opts; @rebootstrap_opts = grep {/$rebootstrap_re/o} @$extra_opts if $extra_opts; + # Let's store the additional bootstrap options in + # the environment variable - they may be used later + # in the mtr tests - for re-bootstrap or run the + # recovery, etc: + my $extra_text = "--datadir=$datadir";
that's rather random. Why do you add only this one option, that doesn't make sense. Better handle any additional requirements in the test.
+ if (@rebootstrap_opts) {
why do you need an if() ?
+ $extra_text .= ' '.join(' ', @rebootstrap_opts); + } + if ($suffix) { + $ENV{'MTR_BOOTSTRAP_OPTS_'.$suffix} = $extra_text; + } + else { + $ENV{'MTR_BOOTSTRAP_OPTS'} = $extra_text;
Is that even possible?
+ } if (@rebootstrap_opts) { mtr_verbose("Re-bootstrap with @rebootstrap_opts"); diff --git a/mysql-test/suite/galera/disabled.def b/mysql-test/suite/galera/disabled.def index 84babda2fa0..18e7c074059 100644 --- a/mysql-test/suite/galera/disabled.def +++ b/mysql-test/suite/galera/disabled.def @@ -28,3 +28,4 @@ query_cache: MDEV-15805 Test failure on galera.query_cache versioning_trx_id: MDEV-18590: galera.versioning_trx_id: Test failure: mysqltest: Result content mismatch galera_wsrep_provider_unset_set: wsrep_provider is read-only for security reasons pxc-421: wsrep_provider is read-only for security reasons +galera_sst_rsync_innodb_nest : MDEV-29591 nesting innodb subdirectories in datadir causes SST to fail
Why do you disable it if the whole commit is about fixing the test? How can I see that the test works?
diff --git a/mysql-test/suite/galera/r/galera_sst_rsync_innodb__innodb_dir,debug.rdiff b/mysql-test/suite/galera/r/galera_sst_rsync_innodb__innodb_dir,debug.rdiff new file mode 100644 index 00000000000..027f4860a39 --- /dev/null +++ b/mysql-test/suite/galera/r/galera_sst_rsync_innodb__innodb_dir,debug.rdiff @@ -0,0 +1,114 @@
looks like a wrong file name?
+--- galera_sst_rsync_innodb_nest.result ++++ galera_sst_rsync_innodb_nest.reject +@@ -286,3 +286,111 @@ + DROP TABLE t1; + COMMIT;
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org