maria-developers team mailing list archive

[Kristian Nielsen <knielsen@xxxxxxxxxxxxxxx>]

Hi Alexi,

Thanks for writing up the low-level design. I read it through, and have a
couple of comments:

> 1.3. In mysqlbinlog.cc:
>
> - Add { "rewrite-db", OPT_REWRITE_DB, ...} record to my_long_options:
> - Add Rpl_filter object to mysqlbinlog.cc
>
>       Rpl_filter* binlog_filter;

Sharing code with similar replication options inside mysqld is a noble
goal. However, in this case I think it is a case of "the cure is worse than
the disease".

The Rpl_filter class has _so_ much mysqld server internals that we do not want
to mix into a client application. That is also why you need to do all these
modifications in sql_list, rpl_filter, etc.

So I think it is wrong to use the Rpl_filter class in mysqlbinlog.

To share code between the two, I think the better method is to move out the
needed functionality (add_db_rewrite() and get_rewrite_db()) in a separate
class, and have both the Rpl_filter class and mysqlbinlog use that.

Alternatively, if the shared functionality is really small (as it appears it
might be), just duplicating the functionality may be better.

> 2. Supporting rewrite-db for RBR events
> ---------------------------------------
>
> In binlog, each row operation event is preceded by Table map event(s) which maps
> table id(s) to database and table names. So, it's enough to support rewriting
> database name in a Table map.
>
> 2.1. Add rewrite_db() member to Table_map_log_event:
>
>       int Table_map_log_event::rewrite_db(
>         const char* new_db,
>         size_t new_db_len,
>         const Format_description_log_event* desc)
>       {
>         /* 1. In temp_buf member (possibly reallocating it) rewrite
>               event length, db length, and db parts
>            2. Change m_dblen and m_dbnam members

You need to be careful here to avoid a memory leak. The n_dbnam memory is part
of a my_multi_malloc(), so it will not be freed in destructor (and we must not
explicitly free the old pointer).

The way the existing code works is really not all that nice for what we are
trying to do here...

It would be cleaner perhaps to have a constructor or member that builds a new
event object, but I am not sure that will work well without major rewrites
that should not be part of this worklog. So what you suggest may work, just a
warning about handling the my_multi_malloc() issue properly.

> Note. write_event_header_and_base64() does not print use-statement. It
> produces BINLOG statement using ev->temp_buf content (i.e. the binary
> log representation of the event). We don't rewrite temp_buf here with
> db_to name (as we do it for Table map event) - this implies the
> limitation 3 mentioned above.
> Question: Is supporting of rewite_db + --base64-output really needed
> currently?

Probably it is not needed. But we should throw an error if --rewrite_db and
--base64-output=always are attempted used together.

> 4. Current status
> -----------------
>
> The outlined design (implemented for mysql-5.1.37) is tested for
> simple test-cases.
>
> TODO. 1. Check list of events which can emit use-statement.
>       2. Supporting of rewite_db + --base64-output ?

Apart from above mentioned points, the design looks ok to me.

 - Kristian.