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

[Andrei Elkin]

Serg, Magnus, Lars, hello.

A new patch is committed.

Many thanks to Serg whose explanations helped to identify the
functions I was looking for implementing the safest for users
fix (Lars' concern).

Since the existed paths comparison algorithm was limited we
eliminate that obstacle.
The patch has not become much longer still it requires many pairs and
single eyes.

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

Magnus, I have to ask you to re-review yet another two-hunk patch. We
had to leave `pidfile_name' to be used further but strip off its
directory name part (my very first patch's hunk).
As Serg pointed out just reverting does not help! Because it would be
conflicting with logics of another bug#20166 hunk in mysqld.cc

  if (gethostname(glob_hostname,sizeof(glob_hostname)) < 0)
  {
    strmake(glob_hostname, STRING_WITH_LEN("localhost"));
    sql_print_warning("gethostname failed, using '%s' as hostname",
                      glob_hostname);
    strmake(pidfile_name, STRING_WITH_LEN("mysql"));
  }
  else
    strmake(pidfile_name, glob_hostname, sizeof(pidfile_name)-5);
  strmov(fn_ext(pidfile_name),".pid");		// Add proper extension

(Notice, `mysql' is assigned to `pidfile_name' not to `glob_hostname'
since that bug#20166).

> 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 was not.



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

It appeared that there are function providing the necessary service!

Particularly this

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

and that ^ is refined.

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

I have not found any use of fn_format() with the flag to construct the
abs path in the server code. For that reason I am still anxious about
passing this patch on different platform.


regards,

Andrei

PS

the patch covers three more artifacts gathered during this work.