maria-developers team mailing list archive

[Sergey Petrunya <psergey@xxxxxxxxxxxx>]

Hi Alexander,

Please find the rest of the review below:

On Tue, Oct 20, 2009 at 10:46:08AM +0400, Alexi1952 wrote:
> The diff is due to run-dependent data in log-events (e.g. in Table map table_id = 23 in my test run and = 60 in your test run).
> It's me got a kink that --replace_regex will correct this in BINLOG :) Actually to correct this, mysqlbinlog should be called with
> --base64-output=decode-rows options.
> 
> Attached is corrected version of the test and result.


Overall comments:

--help message
==============

The new syntax is not fully documented in --help --verbose output. It says:

  --rewrite-db=name   Updates to a database with a different name than the
                      original.

and doesn't specify what is the syntax of the 'name'. I think ideally we would
want to have it print

  --rewrite-db='from->to'

but this seems to be difficult to achieve within the option parsing framework,
so we could follow the approach used by the server's counterpart:

  --replicate-rewrite-db=name 
                      Updates to a database with a different name than the
                      original. Example:
                      replicate-rewrite-db=master_db_name->slave_db_name.

Coding style
============
The patch violates adopted coding style in several places (We follow MySQL's
coding conventions). To speed things up, I've made the fixes myself.

Testing
=======
Please add a checks for

1. that text form of RBR events shows rewritten database names
2. that everything works when mysqlbinlog reads events from the server.

(they both seem to work, we just need test coverage).

Memory errors
=============
When I tried to get events from the server, using a command line like this:

  mysqlbinlog -uroot  --verbose --base64-output=decode-rows -R -t \
     --start-position=0 --host=localhost --rewrite-db='trepl->xx' \
     --debug pslp2-bin.000020 

I got these errors among the output:

Error: Freeing wrong aligned pointer at line 1022, 'log_event.h'
Error: Freeing wrong aligned pointer at line 1022, 'log_event.h'
Error: Freeing wrong aligned pointer at line 1022, 'log_event.h'  --(1)
...
Warning: Memory that was not free'ed (456 bytes):
           456 bytes at 0x86f84d8, allocated at line  201 in 'my_alloc.c' --(2)

Problem (2) is repeatable with the mainline, so I've only reported it to MySQL
as BUG#48281.

Problem (1) is not repeatable on mainline or MariaDB. I've investigated it:

=== Wrong aligned pointer investigation ====
The messages are not produced when dumping local files. The message is
produced from Log_event::free_temp_buf() which is called from 
Table_map_log_event::rewrite_db.
When dumping local log, it calls dump_local_log_entries() which calls 

Log_event* Log_event::read_log_event(IO_CACHE* file,
                                     const Format_description_log_event
                                     *description_event)

which has this code:
  // some events use the extra byte to null-terminate strings
  if (!(buf = (char*) my_malloc(data_len+1, MYF(MY_WME))))
  {
    error = "Out of memory";
    goto err;
  }
  buf[data_len] = 0;
  memcpy(buf, head, header_size);
  if (my_b_read(file, (uchar*) buf + header_size, data_len - header_size))
  {
    error = "read error";
    goto err;
  }
  if ((res= read_log_event(buf, data_len, &error, description_event)))

'buf' is what eventually will be passed to Table_map_log_event constructor. We
can see that buf points to a start of chunk of my_malloc'ed memory.

For the case when we read the remote log, it runs this code:

static Exit_status dump_remote_log_entries(PRINT_EVENT_INFO *print_event_info,
                                           const char* logname)

{
  ...
  net= &mysql->net;
  ...
    if (!(ev= Log_event::read_log_event((const char*) net->read_pos + 1 ,
                                        len - 1, &error_msg,
                                        glob_description_event)))
    {
      error("Could not construct log event object: %s", error_msg);
      DBUG_RETURN(ERROR_STOP);
    }   
    /*
      If reading from a remote host, ensure the temp_buf for the
      Log_event class is pointing to the incoming stream.
    */
    ev->register_temp_buf((char *) net->read_pos + 1);

The first argument to Log_event::read_log_event() will get passed
as buffer pointer Table_map_log_event constructor. It points to 
NET::read_pos + 1, which is not an address we've got from my_malloc and
hence it's not something we should be calling my_free() for.

I've fixed thos problem too.

== Comments to the code ==

> --- maria-5.1-wl36-orig/client/mysqlbinlog.cc	2009-10-20 00:28:26.000000000 +0400
> +++ maria-5.1-wl36/client/mysqlbinlog.cc	2009-10-20 00:25:52.000000000 +0400
...
> @@ -35,6 +35,18 @@
>  #include "log_event.h"
>  #include "sql_common.h"
>  
> +/* Needed for Rlp_filter */
> +struct TABLE_LIST;
> +
> +/* Needed for Rpl_filter */
> +CHARSET_INFO* system_charset_info= &my_charset_utf8_general_ci;
> +
> +#include "../sql/sql_string.h"  // needed for Rpl_filter

Why use server versions of sql_string when there are client/sql_string.*? 
I've tried switching to client/sql_string.* and it worked.

> @@ -2095,6 +2211,21 @@
>   DBUG_RETURN(retval == ERROR_STOP ? 1 : 0);
> }
> 
>
> +/* Redefinition for Rpl_filter::tables_ok() */
> +struct TABLE_LIST
> +{
> +  TABLE_LIST() {}
> +  TABLE_LIST *next_global, **prev_global;
> +  bool  updating;
> +  char* db;
> +  char* table_name;
> +};

I think it's a bad idea to add such "similar" and unused structures:
- it creates confusion
- Other than getting rpl_filter.* to compile, we don't have any use for it
  (and if we implement per-table filtering later on, there is no warranty
   that representation for list of used tables will be linked list of
   TABLE_LIST)

I've tried to #ifdef-away Rpl_filter::tables_ok() from the client and then 
this stopped to be needed.


As mentioned above, I've had to actually apply some of the fixes to make sure
they work. I've pushed the result into
 
  lp:~maria-captains/maria/maria-5.1-wl36

and added that tree into buildbot:
  
  http://askmonty.org/buildbot/grid?branch=maria-5.1-wl36

The remaining issues (need for testcases, help message) are obvious, so I can
tell that I'll be happy with the patch if buildbot doesn't reveal any problems.

BR
 Sergey
-- 
Sergey Petrunia, Software Developer
Monty Program AB, http://askmonty.org
Blog: http://s.petrunia.net/blog