Merge lp:~sergei.glushchenko/percona-server/mysqlbinlog-stdin into lp:percona-server/5.5

[Sergei Glushchenko]

Bug #933969: mysqlbinlog doesn't accept stdin
mysqlbinlog can't handle stdin when "|" used.
mysqlbinlog fails after making an attempt to execute seek on pipe handle.
Buffered read used in mysqlbinlog and it's possible to seek inside buffer
window. But in check_header routine seek(0) made before any read
operation, when buffer is empty. Due to it actual seek is performed.
Solution is to avoid seek(0) operation in check_header before any read is
performed, e.g. when we are at the beginning of the file.

[Sergei Glushchenko (sergei.glushchenko)]

Jenkins build:
http://jenkins.percona.com/view/Percona%20Server%205.5/job/percona-server-5.5-param/280/

[Alexey Kopytov (akopytov)]

Sergei,

- Wouldn't the following be a more correct fix?

--- mysys/mf_iocache2.c 2011-06-30 15:37:13 +0000
+++ mysys/mf_iocache2.c 2012-02-21 10:56:04 +0000
@@ -145,7 +145,8 @@ void my_b_seek(IO_CACHE *info,my_off_t p
   if (info->type == READ_CACHE || info->type == SEQ_READ_APPEND)
   {
     /* TODO: explain why this works if pos < info->pos_in_file */
- if ((ulonglong) offset < (ulonglong) (info->read_end - info->buffer))
+ if (offset == 0 ||
+ (ulonglong) offset < (ulonglong) (info->read_end - info->buffer))
     {
       /* The read is in the current buffer; Reuse it */
       info->read_pos = info->buffer + offset;

As in, if pos == pos_in_file, we literally don't have to do anything, including forcing a seek on next read.

- We usually prefix our test names with "percona_", and it's sometimes handy to have a bug number in the corresponding test name, so "percona_bug933969.{test,result}" would probably be better, but it's optional.

- Similarly, "bug933969.patch" looks a bit more descriptive than "mysqlbinlog.patch" to me. At least I can look up the bug without even opening the patch.

- The test case should also have "--source include/not_windows.inc" and "--source include/not_embedded.inc". We support neither Windows nor embedded, but it would be good to at least try not to break them even further. :)

- Please replace "--exec /bin/bash -c" with "--system". In which case you also don't need --short-form or "> /dev/null".

- Tests usually have SQL statements in uppercase. It's a minor thing, just to make sure all tests use a consistent style.

review: Needs Fixing

[Sergei Glushchenko (sergei.glushchenko)]

Alexey,

> - Wouldn't the following be a more correct fix?
>
> --- mysys/mf_iocache2.c 2011-06-30 15:37:13 +0000
> +++ mysys/mf_iocache2.c 2012-02-21 10:56:04 +0000
> @@ -145,7 +145,8 @@ void my_b_seek(IO_CACHE *info,my_off_t p
> if (info->type == READ_CACHE || info->type == SEQ_READ_APPEND)
> {
> /* TODO: explain why this works if pos < info->pos_in_file */
> - if ((ulonglong) offset < (ulonglong) (info->read_end - info->buffer))
> + if (offset == 0 ||
> + (ulonglong) offset < (ulonglong) (info->read_end - info->buffer))
> {
> /* The read is in the current buffer; Reuse it */
> info->read_pos = info->buffer + offset;
>
> As in, if pos == pos_in_file, we literally don't have to do anything,
> including forcing a seek on next read.
>

I thought about fixing this bug in similar way. Unfortunately both fixes
are not actually fix the root problem and are kind of workaround.
Drawback of your approach is when we do seek(3) instead of seek(0) it will
not work, however seek(0) was working! I mean that it creates a special case
when actually not working solution looks like working.

I thought also of calling my_b_fill() right after stdin buffer created.
The drawback of this approach is that it's not obvious why we are calling
my_b_fill() in one branch of "if" statement and not doing the same in other :)

Please comment this.

As for other things which you have mentioned, I will fix them. Thanks.
However I've seen many testcases having statements in lowercase and even
in mixed case :)

[Alexey Kopytov (akopytov)]

On 21.02.12 16:16, Sergei Glushchenko wrote:
> Alexey,
>
>> - Wouldn't the following be a more correct fix?
>>
>> --- mysys/mf_iocache2.c 2011-06-30 15:37:13 +0000
>> +++ mysys/mf_iocache2.c 2012-02-21 10:56:04 +0000
>> @@ -145,7 +145,8 @@ void my_b_seek(IO_CACHE *info,my_off_t p
>> if (info->type == READ_CACHE || info->type == SEQ_READ_APPEND)
>> {
>> /* TODO: explain why this works if pos < info->pos_in_file */
>> - if ((ulonglong) offset < (ulonglong) (info->read_end - info->buffer))
>> + if (offset == 0 ||
>> + (ulonglong) offset < (ulonglong) (info->read_end - info->buffer))
>> {
>> /* The read is in the current buffer; Reuse it */
>> info->read_pos = info->buffer + offset;
>>
>> As in, if pos == pos_in_file, we literally don't have to do anything,
>> including forcing a seek on next read.
>>
>
> I thought about fixing this bug in similar way. Unfortunately both fixes
> are not actually fix the root problem and are kind of workaround.

Sure, the problem problem is that mysqlbinlog doesn't have to use seek().

> Drawback of your approach is when we do seek(3) instead of seek(0) it will
> not work, however seek(0) was working! I mean that it creates a special case
> when actually not working solution looks like working.
>

There's no way to make my_b_seek(file, x) work in a general case for
pipes, because that might require a real seek anyway. The point is to
avoid doing a real seek when we don't really have to do it. In
particular, my_b_seek(file, 0) before accessing the file becomes a no-op
with the fix, as it should be. Again, in particular, it solves the
problem with unnecessary my_b_seek() in check_header().

An alternative approach would be just replace my_b_seek(file, 0) with an
assertion that the IO cache is positioned at the start of the file when
check_header() is called.

> I thought also of calling my_b_fill() right after stdin buffer created.
> The drawback of this approach is that it's not obvious why we are calling
> my_b_fill() in one branch of "if" statement and not doing the same in other :)
>
> Please comment this.
>

I agree, it would be yet another way to fix it, but the code would
become even less obvious :)

> As for other things which you have mentioned, I will fix them. Thanks.
> However I've seen many testcases having statements in lowercase and even
> in mixed case :)

Well, it's a real world, not everyone follows recommendations and common
conventions :) Some recommendations on writing good tests can be found
here: http://forge.mysql.com/wiki/How_to_Create_Good_Tests

[Sergei Glushchenko (sergei.glushchenko)]

>
> An alternative approach would be just replace my_b_seek(file, 0) with an
> assertion that the IO cache is positioned at the start of the file when
> check_header() is called.
>

As for me, it's the most straight and natural way to fix this particular issue.
It would be not obvious to just remove my_b_seek(file, 0) and could cause some issues
in case of changes. But put an assertion makes things clear.

If you don't mind I would prefer this fix to others.

> Well, it's a real world, not everyone follows recommendations and common
> conventions :) Some recommendations on writing good tests can be found
> here: http://forge.mysql.com/wiki/How_to_Create_Good_Tests

Thanks for that link! I've seen it once, but couldn't remember where.

[Alexey Kopytov (akopytov)]

On 21.02.12 17:18, Sergei Glushchenko wrote:
> As for me, it's the most straight and natural way to fix this particular issue.
> It would be not obvious to just remove my_b_seek(file, 0) and could cause some issues
> in case of changes. But put an assertion makes things clear.
>
> If you don't mind I would prefer this fix to others.

Sure, fine by me.

[Alexey Kopytov (akopytov)]

OK to merge, thanks!

review: Approve

[Stewart Smith (stewart)]

broke rpl_auto_increment test when trying to merge:
http://jenkins.percona.com/view/Percona%20Server%205.5/job/percona-server-5.5-param/281/

review: Needs Fixing

[Sergei Glushchenko (sergei.glushchenko)]

The new fix makes mysqlbinlog not to seek to start pos before header checked.
Jenkins:
http://jenkins.percona.com/view/Percona%20Server%205.5/job/percona-server-5.5-param/285/

Not all configurations are finished yed, but tendencies are good.

Fix the same as for 5.1.

(my_off_t)0 was copied from couple of lines below to keep symmetry, and yes, I don't really need typecast there :)

[Alexey Kopytov (akopytov)]

But the typecast below correctly has a space after it ;)

[Sergei Glushchenko (sergei.glushchenko)]

Oh... really... my fault :)

[Alexey Kopytov (akopytov)]

review: Approve

=== added file 'patches/bug933969.patch'
--- patches/bug933969.patch	1970-01-01 00:00:00 +0000
+++ patches/bug933969.patch	2012-02-23 09:11:18 +0000
@@ -0,0 +1,92 @@
+--- /dev/null
++++ b/mysql-test/r/percona_bug933969.result
+@@ -0,0 +1,16 @@
++RESET MASTER;
++DROP TABLE IF EXISTS t1;
++CREATE TABLE t1 (word VARCHAR(20));
++INSERT INTO t1 VALUES ("hamite");
++INSERT INTO t1 VALUES ("hoho");
++INSERT INTO t1 VALUES ("znamenito");
++INSERT INTO t1 VALUES ("mrachny");
++INSERT INTO t1 VALUES ("mrak");
++INSERT INTO t1 VALUES ("zhut");
++INSERT INTO t1 VALUES ("parnisha");
++INSERT INTO t1 VALUES ("krrasota!");
++INSERT INTO t1 VALUES ("podumayesh");
++INSERT INTO t1 VALUES ("ogo!");
++FLUSH LOGS;
++DROP TABLE t1;
++RESET MASTER;
+--- /dev/null
++++ b/mysql-test/t/percona_bug933969.test
+@@ -0,0 +1,42 @@
++###################### percona_bug933969.test ########################
++# Bug #933969: mysqlbinlog doesn't accept stdin                      #
++#                                                                    #
++# The goal of this testcase is to test that mysqlbinlog handle       #
++# stdin correctly when stdin is pipe.                                #
++# i.e. "cat log | mysqlbinlog -" don't cause mysqlbinlog failure     #
++######################################################################
++-- source include/have_log_bin.inc
++-- source include/not_windows.inc
++-- source include/not_embedded.inc
++
++# deletes all the binary logs
++RESET MASTER;
++
++--disable_warnings
++DROP TABLE IF EXISTS t1;
++--enable_warnings
++
++# produce some statements for binlog
++
++CREATE TABLE t1 (word VARCHAR(20));
++
++INSERT INTO t1 VALUES ("hamite");
++INSERT INTO t1 VALUES ("hoho");
++INSERT INTO t1 VALUES ("znamenito");
++INSERT INTO t1 VALUES ("mrachny");
++INSERT INTO t1 VALUES ("mrak");
++INSERT INTO t1 VALUES ("zhut");
++INSERT INTO t1 VALUES ("parnisha");
++INSERT INTO t1 VALUES ("krrasota!");
++INSERT INTO t1 VALUES ("podumayesh");
++INSERT INTO t1 VALUES ("ogo!");
++
++FLUSH LOGS;
++
++# run mysqlbinlog and make sure it ends normally
++
++let $MYSQLD_DATADIR= `SELECT @@datadir`;
++--system cat $MYSQLD_DATADIR/master-bin.000001 | $MYSQL_BINLOG - >/dev/null
++
++DROP TABLE t1;
++RESET MASTER;
+--- a/client/mysqlbinlog.cc
++++ b/client/mysqlbinlog.cc
+@@ -1760,7 +1760,7 @@
+   }
+ 
+   pos= my_b_tell(file);
+-  my_b_seek(file, (my_off_t)0);
++  DBUG_ASSERT(pos == 0);
+   if (my_b_read(file, header, sizeof(header)))
+   {
+     error("Failed reading header; probably an empty file.");
+@@ -1920,7 +1920,7 @@
+     /* read from normal file */
+     if ((fd = my_open(logname, O_RDONLY | O_BINARY, MYF(MY_WME))) < 0)
+       return ERROR_STOP;
+-    if (init_io_cache(file, fd, 0, READ_CACHE, start_position_mot, 0,
++    if (init_io_cache(file, fd, 0, READ_CACHE, (my_off_t) 0, 0,
+ 		      MYF(MY_WME | MY_NABP)))
+     {
+       my_close(fd, MYF(MY_WME));
+@@ -1928,6 +1928,7 @@
+     }
+     if ((retval= check_header(file, print_event_info, logname)) != OK_CONTINUE)
+       goto end;
++    my_b_seek(file, start_position_mot);
+   }
+   else
+   {

=== modified file 'patches/series'
--- patches/series	2012-01-19 08:42:23 +0000
+++ patches/series	2012-02-23 09:11:18 +0000
@@ -1,3 +1,4 @@
+bug933969.patch
 microsec_process.patch
 optimizer_fix.patch
 mysql_dump_ignore_ct.patch