maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Julius,

On Dec 27, Julius Goryavsky wrote:

> commit 4d9f8a3c31e
> Author: Julius Goryavsky <julius.goryavsky@xxxxxxxxxxx>
> 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@xxxxxxxxxxx