Request for review: bk commit into 5.1 tree (aelkin:1.2578) BUG#28597

[Andrei Elkin]

Serg, Magnus, Lars, hello.

I re-committed a patch for bug#28597 which rolls back some of changes for
a former bug#20166. That part was discussed with Magnus who was saying
that rolling-back is safer than my first workaround, which tried to
preserve those changes (usage of pidfile_name instead of glob_hostname
for generating binlog file name).
Naturally, I am fine with rolling back, as long as it does not
break logics of the former bug - which is also Magnus' and Serg's concern.

Serg, your review is necessary as bug#28597 relates to
failure in upgrading from a GA to another GA.

So the essence of fixes of bug#28597 is unrolling a wrong hunk for bug#20166.

There is another side of bug#28597 concerning with upgrading from a
post-bug#20166, i.e contaminated, releases to a future post-bug#28597
with fixes.

I and Lars stated discussion about that on Friday.
Could we save such way upgrading users from the same risk of stopping
slave as in bug#28597?
After spending some time I have changed my earlier opinion from
"that's impossible" towards it might be rather hard and risky to do it
for a GA.

Master binlog index file can collect an arbitrary combination of names
in different format. Consider --log-bin-index is staying but --log-bin
changes from one value to another during master's several restarts.
In effect a similar to binlog index file of bug#28597 report can be
produced merely by means of legal manipulation of the two options,
also on  a bugless pre-bug#20166 server.

That has couple of implications.

1. Although the different path formats list in the index file is displayable by
   SHOW BINARY LOGS there is no real access to the content of those
   files whose names are in a relative format but
   incompatible with the current mater startup options
   (e.g mysql_data_dir changed).

   One can not see from SHOW BINLOG 'SUCH_FILE', PURGE it, a slave can not 
   pull events from it because the master can not locate the file.

   I wonder if it's worth to report a bug about those issues. I have not
   done that yet.

2. the same applies to a file name recored in the
   index in the absolate format if the master started with --log-bin's
   relative path. This must be indeed just a technical issue, caused
   by implementation, as the abolute path should be reachable always.


Clearly, to fix my bug in the most comfortable for users way means to
have a *new* feature that I think should

 -  stop allowing mixture of absolute and
    relavite path names in the binlog index file - which as is pointed out
    still may be confusing and useless
 -  store names in the abs path format in the binlog index always
 -  be able to compare an abs path format name with a relative path
    format name.

Unfortunately, there is no such notion as the absolute path in
the server code. As far as I have tried to find, there are no functions 
to constuct from a relative path its absolute value - which is what
the feature's implementation should provide. Basicly we need: 

 - to find out which format a name from the binlog index is to compare
 - to be able to build  an absolute path made of
   mysql_real_data_home + binlog file name supplied by slave

   Comparison - with no check of the type! - of two homogeneous either
   way paths that is that the current implemention does essentially
   when it tries locating a binlog file requested by a slave.

All in all, it'd be a new feature which a separate purpose whose
implementation does not seem simple. Absolute path handling functions
are platform specific. Potential changes most probably would affect
other log types because of common interfaces with the binary log
type. There might be side effects that could affect some other parts
like ndb's binlog index.

Please tell me your ideas and concerns.

regards,

Andrei


========================================================================

Attached is the beginning of discussion. I clarified some statements.

> Lars,
>
>> It needs to be able to replicate 5.0-with-20166-patch -> 5.0-new,
>> since those are both GA releases.
>>
>> /L
>
> It would be the ideal way out.
> However, it's impossible.
>
> A fact of the matter is that the user can supply binlog name in either
> format - i.e with the absolute path format
>
> --log-bin=/tmp/andrei/5.1.16/mysql-test/var/log/master-bin
>
>   or with the relative path format
>
> --log-bin=my_log
>
>   or simply
>
> --log-bin
>
> which is a sub-variation of the relative path format.
>
    or with yet another form of relative path format

   --log-bin=../../x_log

> For the absolute path format the master server writes into the index
> file absolute path format file names.
> For the relative path format the master server writes into the index
> file relative path format file names.
>
> Since bug#20166 fixes the master started to write *only* with the absolute
> path format.
>

> The mixture of formats leads to bug#28597.

Not just mixture but one combined with master is staying with the same
relative path in its --log-bin.


>
> So if the user supplies the relative path format file names its master
> creates for 
>
>   1. pre-bug20166 lines like
>
>      ./my_log-bin.000001
>      ./my_log-bin.000002
>
>   2. upgraded from pre-bug20166 to bug20166-fixed
>      
>      ./my_log-bin.000001                 # the line before upgrade
>      /var/log/.../my_log-bin.000002
>
>     i.e this is the current bug#28597 
>   
>   3. your suggestion  upgraded bug20166-fixed to bug28597-fixed
>      
>      /var/log/.../my_log-bin.000001   # the line before upgrade
>      ./my_log-bin.000002
>
> Notice, the line before upgrade does not say which format it was meant
> to. This fact is lost but it is cruicial to resolve your requirement.

the summary stays:

>
> Alas, i don't see anything but to address the issue of
> `upgraded bug20166-fixed to bug28597-fixed'
> in docs plus adding a warning like `be prepared your slaves will disconnect because
> of ...; do this ...' by which bug28597-fixed would react on the relative
> path binlog file name supplied to the master startup options.
>



> cheers,
>
> Andrei
>
>>
>>
>> On Fri, Oct 19, 2007 at 03:32:53PM +0300, Andrei Elkin wrote:
>>> Lars, hi.
>>> 
>>> > Hi Andrei,
>>> >
>>> > This patch is only fixing what is written to the index file.
>>> >
>>> > I don't think this is enough, since BUG#28597 is about an index file
>>> > that already exists on the master.  With the exact same file as given
>>> > in the bug report, the replication should still work.
>>> >
>>> 
>>> The problem of the bug is in that \cite{cset_comments}
>>>  
>>>   bug@20166 that replaced the binlog file name generating to favor
>>>   pidfile_name instead of the previous glob_hostname generated names
>>>   started to be written in the absolute path format.
>>> 
>>> That bug20166 changes forced writing the names of generated binlog file
> names
>>> in the absolute path format whereas they had been written in the
>>> relative path format previously, which was correct.
>>> 
>>> The patch that cures from this artifact definitely has to strip the dir
>>> name part to get the prior-bug20166 behaviour.
>>> 
>>> Stripping could be done either by reverting the changes of bug#20166
>>> or the way I did which also address a pending todo.
>>> 
>>> Anything wrong with this still?
>>> 
>>> regards,
>>> 
>>> Andrei
>>> 
>>> 
>>> > Best wishes,
>>> > Lars
>>> >
>>> >
>>> > On Fri, Oct 19, 2007 at 03:02:19PM +0300, Andrei Elkin wrote:
>>> >> Below is the list of changes that have just been committed into a
> local
>>> >> 5.1 repository of elkin. When elkin does a push these changes will
>>> >> be propagated to the main repository and, within 24 hours after the
>>> >> push, to the public repository.
>>> >> For information on how to access the public repository
>>> >> see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html
>>> >> 
>>> >> ChangeSet@stripped, 2007-10-19 15:02:10+03:00,
> aelkin@stripped +1 -0
>>> >>   Bug #28597 Replication doesn't start after upgrading to 5.1.18
>>> >>   
>>> >>   Since bug@20166 that replaced the binlog file name generating to
> favor pidfile_name instead of 
>>> >>   the previous glob_hostname generated names started to be written
> in the absolute path format.
>>> >>   
>>> >>   Fixed with stripping off the directory part of a generated name
> that also terminates a pending TODO.
>>> >>   
>>> >>    
>>> >> 
>>> >>   sql/log.cc@stripped, 2007-10-19 15:02:04+03:00,
> aelkin@stripped +6 -5
>>> >>     generating bin-log file name as relative path. Using
> make_default_log_name pattern to eliminate
>>> >>     a pending TODO in MYSQL_LOG::generate_name.
>>> >> 
>>> >> diff -Nrup a/sql/log.cc b/sql/log.cc
>>> >> --- a/sql/log.cc	2007-10-12 10:31:59 +03:00
>>> >> +++ b/sql/log.cc	2007-10-19 15:02:04 +03:00
>>> >> @@ -2166,13 +2166,14 @@ const char *MYSQL_LOG::generate_name(con
>>> >>  {
>>> >>    if (!log_name || !log_name[0])
>>> >>    {
>>> >> +    DBUG_ASSERT(strlen(suffix) <= 4);     // avoids overrun
>>> >>      /*
>>> >> -      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.
>>> >> +      generating in style of make_default_log_name
>>> >>      */
>>> >> -    strmake(buff, pidfile_name, FN_REFLEN - 5);
>>> >> -    strmov(fn_ext(buff), suffix);
>>> >> -    return (const char *)buff;
>>> >> +    strmake(buff, pidfile_name, FN_REFLEN-5);
>>> >> +    return (const char *)
>>> >> +      fn_format(buff, buff, "", suffix,
>>> >> +               
> MYF(MY_UNPACK_FILENAME|MY_REPLACE_EXT|MY_REPLACE_DIR));
>>> >>    }
>>> >>    // get rid of extension if the log is binary to avoid problems
>>> >>    if (strip_ext)
>>> >> 
>>> >> -- 
>>> >> MySQL Code Commits Mailing List
>>> >> For list archives: http://lists.mysql.com/commits
>>> >> To unsubscribe:    http://lists.mysql.com/commits?unsub=1
>>> >
>>> > -- 
>>> > Dr. Lars Thalmann
>>> > Replication and Clustering Technology
>>> > MySQL AB, www.mysql.com
>>> -- 
>>> Andrei Elkin, Software Developer, PhD
>>> MySQL Finland Oy, Helsinki, Finland, www.mysql.com
>>> 
>>> Are you MySQL certified?  www.mysql.com/certification
>>
>> -- 
>> Dr. Lars Thalmann
>> Replication and Clustering Technology
>> MySQL AB, www.mysql.com