Re: bk commit into 5.0 tree (aelkin:1.2543) BUG#28597

[Andrei Elkin]

Sergei, hi.

> Hi!
>
> On Oct 25, Andrei Elkin wrote:
>> ChangeSet@stripped, 2007-10-25 12:21:46+03:00, aelkin@stripped +3 -0
>>   Bug #28597 Replication doesn't start after upgrading to 5.1.18
>>   
>>   Since bug@20166, which replaced the binlog file name generating to base on
> pidfile_name instead of 
>>   the previous glob_hostname, the binlog file name generating and its storing
> into the binlog index
>>   suddenly started to be solely in the absolute path format,
>>   including a case when --log-bin option meant a relative path.
>
> make comment lines shorter than 80 characters
>    

ok. (so am used to emacs' automatic formating which does not exist
in bk's widgets)


>>   An existed algorithm on master to find a requested binlog file for slave was
> limited to compare only
>>   homogenous paths either absolute or relative but never mixed. 
>>   In effect of two facts if --log-bin value means a relative path, comparison on
> the buggy 5.1.18 of an
>>   allways absolute value (read from the index) with a relave  was doomed.
>>   
>>   Fixed with leaving `pidfile_name' but stripping off its directory part that
> restores the original logics
>>   of storing the names in compatible with --log-bin option format.
>>   The comparison algorithm is refined to be able match any format two paths
> converting them to the absolute
>>   paths.
>>   
>>   Two side effects for this fix:
>>   

>>   correcting bug#27070;

Maybe I should have written the synopsis for you not to claim
something very controversal...

Bug #27070 server logs are created unrequested and in wrong directory

>>   ensuring no overrun for buff can happen anymore (Bug#31836 
>>   insufficient space reserved for the suffix of relay log file name);
>>   bug#31837  --remove_file $MYSQLTEST_VARDIR/tmp/bug14157.sql missed in
> rpl_temporary.test;
>> 
>>   mysql-test/t/rpl_dual_pos_advance.test@stripped, 2007-10-25 12:21:44+03:00,
> aelkin@stripped +0 -6
>>     After correcting the logics of log file name composing workaround for
>>     
>>       Bug #27070 server logs are created unrequested and in wrong directory
>>     
>>     is removed.
>> 
>>   mysql-test/t/rpl_temporary.test@stripped, 2007-10-25 12:21:44+03:00,
> aelkin@stripped +2 -0
>>     remaining temp file of the test removed
>> 
>>   sql/log.cc@stripped, 2007-10-25 12:21:44+03:00, aelkin@stripped +36 -10
>>     stripping off the directory part of `pidfile_name' for binlog name
> generating;
>>     a new function static bool equalp_paths() compares two paths;
>>     ensuring no overrun for buff can happen anymore (Bug #31836 
>>     insufficient space reserved for the suffix of relay log file name)
>> 
>> diff -Nrup a/sql/log.cc b/sql/log.cc
>> --- a/sql/log.cc	2007-07-30 18:27:30 +03:00
>> +++ b/sql/log.cc	2007-10-25 12:21:44 +03:00
>> @@ -448,13 +448,11 @@ const char *MYSQL_LOG::generate_name(con
>>  {
>>    if (!log_name || !log_name[0])
>>    {
>> -    /*
>> -      TODO: The following should be using fn_format();  We just need to
>> -      first change fn_format() to cut the file name if it's too long.
>> -    */
>> -    strmake(buff, pidfile_name,FN_REFLEN-5);
>> -    strmov(fn_ext(buff),suffix);
>> -    return (const char *)buff;
>> +    strmake(buff, pidfile_name, FN_REFLEN - strlen(suffix) - 1);
>> +    return (const char *)
>> +      fn_format(buff, buff, "", suffix,
>> +                MYF(MY_UNPACK_FILENAME|MY_REPLACE_EXT|MY_REPLACE_DIR));

>
> Why do you remove directory here ? It's wrong, binlogs should be where
> pidfile is (by default), not in the datadir.
>

Sorry, it's not. Besides bug#27070 there is <5.1 manual>

   When started with the `--log-bin[=BASE_NAME]' option, `mysqld'
   writes a log file containing all SQL commands that update data. If
   no BASE_NAME value is given, the default name is the name of the
   host machine followed by `-bin'. If the basename is given, but not
   as an absolute pathname, the server writes the file in the data
   directory.

>>    }
>>    // get rid of extension if the log is binary to avoid problems
>>    if (strip_ext)
>> @@ -796,6 +794,33 @@ err:
>>  #endif /* HAVE_REPLICATION */
>>  
>>  /*
>> +  Compare two arbitrary paths represented as null-terminated strings.
>> +  If a path arg is relative then `abs_root' absolute path provides the
>> +  necessary contribution to build the absolute path out of the path arg.
>> +
>> +  SYNOPSIS
>> +    equalp_paths()
>> +    
>> +    path_a, path_b  being compared paths
>> +    abs_root        an absolute root path
>> +
>> +  RETURN VALUES
>> +  TRUE if the paths are equal, otherwise FALSE
>> + */
>> +static bool equalp_paths(const char *path_a,  const char *path_b,
>> +                         const char *abs_root= mysql_real_data_home)
>> +{
>> +  size_t len;
>> +  uint flag= MYF(MY_RETURN_REAL_PATH|MY_RESOLVE_SYMLINKS);
>
> remove MY_RETURN_REAL_PATH and MY_RESOLVE_SYMLINKS.

MY_RESOLVE_SYMLINKS might be really unnecessary.
What to do mean to remove MY_RETURN_REAL_PATH?

The idea we have discussed is to convert both paths into comparable
absolute formats.

Sorry, I am lost.


> We only want to compare two strings, not query filesystem for directory
> hierarchy.

We do have to know the absolute paths - remember one of the paths in
my bug condition is proven to be absolute.


>
>> +  char abs_path_a[2*FN_REFLEN];
>> +  char abs_path_b[2*FN_REFLEN];
>> +  fn_format(abs_path_a, path_a, abs_root, "", flag);
>> +  fn_format(abs_path_b, path_b, abs_root, "", flag);
>> +  return ((len= strlen(abs_path_a)) == strlen(abs_path_b) && 
>> +          memcmp(abs_path_a, abs_path_b, len) == 0);
>> +}
>> +
>> +/*
>>    Find the position in the log-index-file for the given log name
>>  
>>    SYNOPSIS
>> @@ -852,11 +877,12 @@ int MYSQL_LOG::find_log_pos(LOG_INFO *li
>>  
>>      // if the log entry matches, null string matching anything
>>      if (!log_name ||
>> -	(log_name_len == length-1 && fname[log_name_len] == '\n' &&
>> -	 !memcmp(fname, log_name, log_name_len)))
>> +        (fname[length - 1] == '\n' &&
>> +         !(fname[length - 1]= 0)  &&
>> +         equalp_paths(log_name, fname)))

>
> remove equalp_paths function and do fn_format explicitly.
> fn_format(fname) - here
> fn_format(log_name) - *outside of the loop*
>

If you don't mind, I'd rather to make it inline and stay with the new
function. It might be useful.

>>      {
>>        DBUG_PRINT("info",("Found log file entry"));
>> -      fname[length-1]=0;			// remove last \n
>> +      fname[length - 1]= 0;
>>        linfo->index_file_start_offset= offset;
>>        linfo->index_file_offset = my_b_tell(&index_file);
>>        break;
>> 
> Regards / Mit vielen Grüssen,
> Sergei
>

cheers,

Andrei