From: Alfranio Correia
Date: October 5 2010 8:52am
Subject: Re: bzr commit into mysql-5.5 branch (mats.kindahl:3087) WL#5465
List-Archive: http://lists.mysql.com/commits/119961
Message-Id: <4CAAE730.2030400@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

Hi Mats,

Patch approved.
See minor comments in-line.

Cheers.

On 09/27/2010 06:30 PM, Mats Kindahl wrote:
> #At file:///home/bzr/mkindahl/w5465-5.5/ based on revid:joerg@stripped
>
>   3087 Mats Kindahl	2010-09-27
>        WL#5465: System variables: paths to relay log and binary log files
>
>        Adding new variables with paths to where the binary log files and
>        the relay log files are located. Three new global read-only variables
>        are added and one has changed contents.
>
>        log_bin_basename
>          This variable hold the full path and name used for the binary logs:
>          not including the extension of the files.
>
>        log_bin_index
>          This variable contain the full path to the binary log index file.
>
>        relay_log_basename
>          This variable hold the full path and name used for the relay logs:
>          not including the extension of the files.
>
>        relay_log_index
>          This variable now contain the full path to the relay log index file.
>          Previously, it just contained the value supplied to mysqld option
>          relay-log-index,  which was then used to compute the path to the
>          index file.
>       @ libmysqld/CMakeLists.txt
>          Adding file rpl_init.cc.
>       @ mysql-test/suite/rpl/t/rpl_variables.test
>          Extending test to print value of replication file variables.
>       @ sql/CMakeLists.txt
>          Adding new file rpl_init.cc.
>       @ sql/mysqld.cc
>          Exporting opt_binlog_index_name.
>       @ sql/rpl_init.cc
>          Initial revision.
>       @ sql/rpl_init.h
>          Initial revision.
>       @ sql/slave.h
>          Exporting variable storing the value of log-bin-index.
>       @ sql/sys_vars.cc
>          Adding variables relay_log_basename, relay_log_index, log_bin_basename,
>          and log_bin_index as non-option variables.
>
>      added:
>        sql/rpl_init.cc
>        sql/rpl_init.h
>      modified:
>        libmysqld/CMakeLists.txt
>        mysql-test/suite/rpl/r/rpl_variables.result
>        mysql-test/suite/rpl/t/rpl_variables.test
>        sql/CMakeLists.txt
>        sql/mysqld.cc
>        sql/slave.h
>        sql/sys_vars.cc
> === modified file 'libmysqld/CMakeLists.txt'
> --- a/libmysqld/CMakeLists.txt	2010-08-18 11:29:04 +0000
> +++ b/libmysqld/CMakeLists.txt	2010-09-27 17:30:05 +0000
> @@ -83,7 +83,7 @@ SET(SQL_EMBEDDED_SOURCES emb_qcache.cc l
>              ../sql/sql_alter.cc ../sql/sql_partition_admin.cc
>              ../sql/event_parse_data.cc
>              ../sql/sql_signal.cc ../sql/rpl_handler.cc
> -	       ../sql/rpl_utility.cc
> +	   ../sql/rpl_utility.cc ../sql/rpl_init.cc
>              ../sql/sys_vars.cc
>              ${CMAKE_BINARY_DIR}/sql/sql_builtin.cc
>              ../sql/mdl.cc ../sql/transaction.cc
>
> === modified file 'mysql-test/suite/rpl/r/rpl_variables.result'
> --- a/mysql-test/suite/rpl/r/rpl_variables.result	2008-08-14 09:38:22 +0000
> +++ b/mysql-test/suite/rpl/r/rpl_variables.result	2010-09-27 17:30:05 +0000
> @@ -47,6 +47,19 @@ include/start_slave.inc
>   [on slave]
>   SET @@global.init_slave = 'SELECT 1';
>   [on master]
> +SELECT @@pid_file, @@datadir;
> +@@pid_file	MYSQL_TEST_DIR/var/run/mysqld.1.pid
> +@@datadir	MYSQL_TEST_DIR/var/mysqld.1/data/
> +**** Relay log variables
> +SELECT @@relay_log, @@relay_log_index, @@relay_log_basename;
> +@@relay_log	NULL
> +@@relay_log_index	MYSQL_TEST_DIR/var/mysqld.1/data/mysqld-relay-bin.index
> +@@relay_log_basename	MYSQL_TEST_DIR/var/mysqld.1/data/mysqld-relay-bin
> +**** Binary log variables
> +SELECT @@log_bin, @@log_bin_index, @@log_bin_basename;
> +@@log_bin	1
> +@@log_bin_index	MYSQL_TEST_DIR/var/mysqld.1/data/master-bin-bin.index
> +@@log_bin_basename	MYSQL_TEST_DIR/var/mysqld.1/data/master-bin-bin
>   CREATE TABLE tstmt (id INT AUTO_INCREMENT PRIMARY KEY,
>   truth BOOLEAN,
>   num INT,
>
> === modified file 'mysql-test/suite/rpl/t/rpl_variables.test'
> --- a/mysql-test/suite/rpl/t/rpl_variables.test	2008-07-17 16:26:59 +0000
> +++ b/mysql-test/suite/rpl/t/rpl_variables.test	2010-09-27 17:30:05 +0000
> @@ -122,6 +122,15 @@ SET @@global.init_slave = 'SELECT 1';
>
>   --echo [on master]
>   connection master;
> +# checking values of read-only variables
> +--replace_result $MYSQL_TEST_DIR MYSQL_TEST_DIR
> +query_vertical SELECT @@pid_file, @@datadir;
> +--echo **** Relay log variables
> +--replace_result $MYSQL_TEST_DIR MYSQL_TEST_DIR
> +query_vertical SELECT @@relay_log, @@relay_log_index, @@relay_log_basename;
> +--echo **** Binary log variables
> +--replace_result $MYSQL_TEST_DIR MYSQL_TEST_DIR
> +query_vertical SELECT @@log_bin, @@log_bin_index, @@log_bin_basename;
>
>   # Tables where everything happens.
>   CREATE TABLE tstmt (id INT AUTO_INCREMENT PRIMARY KEY,
>
> === modified file 'sql/CMakeLists.txt'
> --- a/sql/CMakeLists.txt	2010-08-31 11:06:56 +0000
> +++ b/sql/CMakeLists.txt	2010-09-27 17:30:05 +0000
> @@ -69,7 +69,8 @@ SET (SQL_SOURCE
>                  event_queue.cc event_db_repository.cc
>                  sql_tablespace.cc events.cc ../sql-common/my_user.c
>                  partition_info.cc rpl_utility.cc rpl_injector.cc sql_locale.cc
> -               rpl_rli.cc rpl_mi.cc sql_servers.cc sql_audit.cc
> +               rpl_init.cc rpl_rli.cc rpl_mi.cc
> +               sql_servers.cc sql_audit.cc
>                  sql_connect.cc scheduler.cc sql_partition_admin.cc
>                  sql_profile.cc event_parse_data.cc sql_alter.cc
>                  sql_signal.cc rpl_handler.cc mdl.cc sql_admin.cc
>
> === modified file 'sql/mysqld.cc'
> --- a/sql/mysqld.cc	2010-08-30 14:07:40 +0000
> +++ b/sql/mysqld.cc	2010-09-27 17:30:05 +0000
> @@ -67,6 +67,7 @@
>   #include "scheduler.h"
>   #include "debug_sync.h"
>   #include "sql_callback.h"
> +#include "rpl_init.h"
>
>   #ifdef WITH_PERFSCHEMA_STORAGE_ENGINE
>   #include "../storage/perfschema/pfs_server.h"
> @@ -656,7 +657,8 @@ static my_bool opt_do_pstack;
>   static my_bool opt_bootstrap, opt_myisam_log;
>   static int cleanup_done;
>   static ulong opt_specialflag;
> -static char *opt_update_logname, *opt_binlog_index_name;
> +static char *opt_update_logname;
> +char *opt_binlog_index_name;
>   char *mysql_home_ptr, *pidfile_name_ptr;
>   /** Initial command line arguments (count), after load_defaults().*/
>   static int defaults_argc;
> @@ -3080,6 +3082,31 @@ static inline char *make_default_log_nam
>     return make_log_name(buff, default_logfile_name, log_ext);
>   }
>
> +/**
> +  Create a replication file name or base for file names.
> +
> +  Only the basename of this string will be used.
> +
> +  @param[in] opt Value of option, or NULL
> +  @param[in] def Default value if option value is not set.
> +  @param[in] ext Extension to use for the path
> + */
> +static inline const char *
> +make_replication_log_name(const char *opt,
> +                          const char *def,
> +                          const char *ext)
> +{
> +  DBUG_ENTER("make_replication_log_name");
> +  DBUG_PRINT("enter", ("opt: %s, def: %s, ext: %s", opt, def, ext));
> +  char buff[FN_REFLEN];
> +  const char *base= opt ? opt : def;
> +  DBUG_PRINT("debug", ("mysql_real_data_home_ptr: %s", mysql_real_data_home_ptr));
> +  fn_format(buff, base, mysql_real_data_home_ptr, ext,
> +            MY_REPLACE_DIR | MY_REPLACE_EXT | MY_UNPACK_FILENAME | MY_RETURN_REAL_PATH);

I think you should check if the function fails and also use MY_SAFE_PATH.
See mysys/mf_format.cc:

     /* To long path, return original or NULL */
     size_t tmp_length;
     if (flag & MY_SAFE_PATH)
       DBUG_RETURN(NullS);

> +  DBUG_RETURN(strdup(buff));

It is unlikely but strdup may also fail.

> +}
> +
> +
>   static int init_common_variables()
>   {
>     char buff[FN_REFLEN];
> @@ -3148,6 +3175,11 @@ static int init_common_variables()
>     strmov(fn_ext(pidfile_name),".pid");		// Add proper extension

>
>     /*
> +    Create the default names for binary and relay log files.
> +   */

I Think this comment is in the wrong place.

> +
> +
> +  /*
>       The default-storage-engine entry in my_long_options should have a
>       non-null default value. It was earlier intialized as
>       (longlong)"MyISAM" in my_long_options but this triggered a
> @@ -3433,7 +3465,7 @@ static int init_common_variables()
>                 make_default_log_name(buff, ".log"));
>     FIX_LOG_VAR(opt_slow_logname,
>                 make_default_log_name(buff, "-slow.log"));
> -
> +
>   #if defined(ENABLED_DEBUG_SYNC)
>     /* Initialize the debug sync facility. See debug_sync.cc. */
>     if (debug_sync_init())
> @@ -3782,6 +3814,12 @@ static int init_server_components()
>       unireg_abort(1);
>     }
>
> +  /*
> +    Replication Setup.
> +
> +    TODO: Most of this code could probably be moved to rpl_init.cc.
> +   */
> +
>     /* need to configure logging before initializing storage engines */
>     if (opt_log_slave_updates&&  !opt_bin_log)
>     {
> @@ -3854,6 +3892,7 @@ a file name for --log-bin-index option",
>                           "master and has his hostname changed!! Please "
>                           "use '--log-bin=%s' to avoid this problem.", ln);
>       }
> +    DBUG_PRINT("debug", ("mysql_bin_log.generate_name(): %s", ln));
>       if (ln == buf)
>       {
>         my_free(opt_bin_logname);
> @@ -3865,6 +3904,11 @@ a file name for --log-bin-index option",
>       }
>     }
>
> +  g_log_bin_basename= make_replication_log_name(opt_bin_logname, pidfile_name, "-bin");
> +  g_relay_log_basename= make_replication_log_name(opt_relay_logname, pidfile_name, "-relay-bin");
> +  g_log_bin_index= make_replication_log_name(opt_binlog_index_name, g_log_bin_basename, ".index");
> +  g_relay_log_index= make_replication_log_name(opt_relaylog_index_name, g_relay_log_basename, ".index");
> +

Please, consider the case that these calls may fail. Maybe:

if (!g_log_bin_basename || !g_relay_log_basename || !g_log_bin_index || !g_relay_log_index)
   exit(1);

>     /* call ha_init_key_cache() on all key caches to init them */
>     process_key_caches(&ha_init_key_cache);
>
> @@ -5778,9 +5822,13 @@ struct my_option my_long_options[]=
>      &opt_bin_logname,&opt_bin_logname, 0, GET_STR_ALLOC,
>      OPT_ARG, 0, 0, 0, 0, 0, 0},
>     {"log-bin-index", 0,
> -   "File that holds the names for last binary log files.",
> +   "File that holds the names for binary log files.",
>      &opt_binlog_index_name,&opt_binlog_index_name, 0, GET_STR,
>      REQUIRED_ARG, 0, 0, 0, 0, 0, 0},
> +  {"relay-log-index", 0,
> +   "File that holds the names for relay log files.",
> +&opt_relaylog_index_name,&opt_relaylog_index_name, 0, GET_STR,
> +   REQUIRED_ARG, 0, 0, 0, 0, 0, 0},
>     {"log-isam", OPT_ISAM_LOG, "Log all MyISAM changes to file.",
>      &myisam_log_filename,&myisam_log_filename, 0, GET_STR,
>      OPT_ARG, 0, 0, 0, 0, 0, 0},
>
> === added file 'sql/rpl_init.cc'
> --- a/sql/rpl_init.cc	1970-01-01 00:00:00 +0000
> +++ b/sql/rpl_init.cc	2010-09-27 17:30:05 +0000
> @@ -0,0 +1,7 @@
> +#include "my_global.h"
> +#include "rpl_init.h"
> +
> +const char *g_log_bin_index= 0;
> +const char *g_relay_log_index= 0;
> +const char *g_relay_log_basename= 0;
> +const char *g_log_bin_basename= 0;
>
> === added file 'sql/rpl_init.h'
> --- a/sql/rpl_init.h	1970-01-01 00:00:00 +0000
> +++ b/sql/rpl_init.h	2010-09-27 17:30:05 +0000
> @@ -0,0 +1,9 @@
> +#ifndef RPL_INIT_INCLUDED
> +#define RPL_INIT_INCLUDED
> +
> +extern const char *g_log_bin_index;
> +extern const char *g_relay_log_index;
> +extern const char *g_relay_log_basename;
> +extern const char *g_log_bin_basename;
> +
> +#endif /* RPL_INIT_INCLUDED */
>
> === modified file 'sql/slave.h'
> --- a/sql/slave.h	2010-03-31 14:05:33 +0000
> +++ b/sql/slave.h	2010-09-27 17:30:05 +0000
> @@ -113,6 +113,7 @@ extern bool use_slave_mask;
>   extern char *slave_load_tmpdir;
>   extern char *master_info_file, *relay_log_info_file;
>   extern char *opt_relay_logname, *opt_relaylog_index_name;
> +extern char *opt_binlog_index_name;
>   extern my_bool opt_skip_slave_start, opt_reckless_slave;
>   extern my_bool opt_log_slave_updates;
>   extern char *opt_slave_skip_errors;
>
> === modified file 'sql/sys_vars.cc'
> --- a/sql/sys_vars.cc	2010-08-30 14:07:40 +0000
> +++ b/sql/sys_vars.cc	2010-09-27 17:30:05 +0000
> @@ -38,6 +38,7 @@
>   #include "rpl_mi.h"
>   #include "transaction.h"
>   #include "mysqld.h"
> +#include "rpl_init.h"
>   #include "lock.h"
>   #include "sql_time.h"                       // known_date_time_formats
>   #include "sql_acl.h" // SUPER_ACL,
> @@ -2897,10 +2898,33 @@ static Sys_var_charptr Sys_relay_log(
>          READ_ONLY GLOBAL_VAR(opt_relay_logname), CMD_LINE(REQUIRED_ARG),
>          IN_FS_CHARSET, DEFAULT(0));
>
> +// Uses NO_CMD_LINE since the --relay-log-index option set
> +// opt_relaylog_index_name variable and computes a value for the
> +// g_relay_log_index variable.
>   static Sys_var_charptr Sys_relay_log_index(
>          "relay_log_index", "The location and name to use for the file "
>          "that keeps a list of the last relay logs",
> -       READ_ONLY GLOBAL_VAR(opt_relaylog_index_name), CMD_LINE(REQUIRED_ARG),
> +       READ_ONLY GLOBAL_VAR(g_relay_log_index), NO_CMD_LINE,
> +       IN_FS_CHARSET, DEFAULT(0));
> +

Please, use /* */.

> +// Uses NO_CMD_LINE since the --log-bin-index option set
> +// opt_binlog_index_name variable and computes a value for the
> +// g_log_bin_index variable.
> +static Sys_var_charptr Sys_binlog_index(
> +       "log_bin_index", "File that holds the names for last binary log files.",
> +       READ_ONLY GLOBAL_VAR(g_log_bin_index), NO_CMD_LINE,
> +       IN_FS_CHARSET, DEFAULT(0));

Please, use /* */.

> +
> +static Sys_var_charptr Sys_relay_log_basename(
> +       "relay_log_basename",
> +       "The full path of the relay log file names, excluding the extension.",
> +       READ_ONLY GLOBAL_VAR(g_relay_log_basename), NO_CMD_LINE,
> +       IN_FS_CHARSET, DEFAULT(0));
> +
> +static Sys_var_charptr Sys_log_bin_basename(
> +       "log_bin_basename",
> +       "The full path of the binary log file names, excluding the extension.",
> +       READ_ONLY GLOBAL_VAR(g_log_bin_basename), NO_CMD_LINE,
>          IN_FS_CHARSET, DEFAULT(0));
>
>   static Sys_var_charptr Sys_relay_log_info_file(
>
>
>
>
>