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