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

[Sergei Golubchik]

Hi!

On Oct 27, Andrei Elkin wrote:
> 
> >>   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

I remember :)
But in general, I agree, bug synopsys should be in the changeset
comment. Good catch.
 
> >> 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.

The difference is only visible when --pidfile is given but --log-bin is
without filename. In that case I thought binlog should be where pidfile
is (which is often in datadir, it's --pidfile default location).

But ok, it's questionable, and manual seems to support your code.
Let's keep it, scratch my comment.

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

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.

> > 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)
 
Regards / Mit vielen Grüssen,
Sergei

-- 
   __  ___     ___ ____  __
  /  |/  /_ __/ __/ __ \/ /   Sergei Golubchik <serg@stripped>
 / /|_/ / // /\ \/ /_/ / /__  Principal Software Developer
/_/  /_/\_, /___/\___\_\___/  MySQL GmbH, Dachauer Str. 37, D-80335 München
       <___/                  Geschäftsführer: Kaj Arnö - HRB
München 162140