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

[Andrei Elkin]

Sergei, good morning.

>> >> @@ -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.

>
> The idea was to prepend mysql_real_data_home if a path is not absolute.
> That is, purely string concatenation.
>

Please see below.

> fn_format with MY_RETURN_REAL_PATH calls realpath() which in fact walks
> your path and resolves symlinks. E.g. if the path is
> /usr/local/mysql-5.0/var/mysql-log.bin, but /usr/local/mysql-5.0 is a
> symlink to /home/aelkin/devel/mysql-5.0 and /home is a symlink to
> /usr/home, then the path will be changed to
> /usr/home/aelkin/devel/mysql-5.0
>
> I meant that it's an overkill, we don't need it, it's enough to add
> mysql_real_data_home if the path it relative. We have a function to do
> it.

As I get the last paragraph, MY_RETURN_REAL_PATH should be a necessary
option for `fn_format' and its invocation is unavoidable, right?

I am asking because you are saying "purely string concatenation".
That's not always true.
`mysql_real_data_home' is generally not a canonical absolute path in
that it can constist of `..' tokens - like in this example -

  (gdb) p mysql_real_data_home

  /home/elkin/MySQL/TEAM/FIXES/5.0/bug28597/sql/../mysql-test/var/master-data/

and then its *string* concatanation with a name of the binlog file
would produce an absolute non-canonical path still be different as the
string from an equal absolute canonical path in the binlog index.

So fn_format(..., MY_RETURN_REAL_PATH) canonizes more to converting to
absolute.
It's not an overkill.

>
>> > 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.
>
> Yes. See above.
>  
>> >> +  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.

>
> I do mind. You're going to concatenate two strings in a loop, while both
> strings don't change in a loop - it is perfectly enough to do the
> operation once, outside of the loop.
>
> Please do it outside of the loop, unless you can prove (by showing
> generated assembly) that the compiler moves the concatenation out of the
> loop automatically.
>
> (inline makes no difference, compiler can figure out what functions are
> worth inlining. At high optimization levels it can even completely ignore
> your "inline" modifiers)

Thanks for spelling this out! (blindly overlooked "*outside of the
loop*" remark).
Indeed one of the paths is a constant.

Then let's have one argument to be a canonical absolute path -

 "fn_format(log_name) - *outside of the loop*".

cheers,

Andrei