Hi Mats,
Thank you for your comments. Since time is pressing, I'll try to explain the
issues with you one by one. I think the most serious is this one.
> * The use of Old_rows_log_event does not look right. Especially
> since you are using multiple inheritance from two classes that
> both implement virtual functions. You are using a qualified name
> to reference to a virtual function in that class, which defeats
> the purpose of using a virtual function in that case. In addition,
> you are passing the 'this' pointer to a member function defined in
> a superclass, which doesn't look right.
Let me explain my thinking here and let me know if I make any major mistakes.
Write_rows_log_event_old and descendants was introduced when you implemented the
"cannonical" row format to fix BUG#22583. Then you changed type numbers for row
events but kept their implementation in the same classes as before
(*_rows_log_event). To handle row events of old types, you introduced
*_rows_event_old classes which inherit from the original ones. E.g.,
class Write_rows_log_event_old: public Write_rows_log_event
This creates a bit peculiar situation: new class which handles old type of event
inherits from old class which implements new type of events.
The *_rows_log_event_old classes only redefine do_prepare_row() method and share
(inherit) all other methods with *_rows_log_event variants. This is because
methods like do_apply_event(), do_exec_row() and friends haven't been changed by
your patch.
Now, my changes affect do_apply_event() and do_exec_row() methods and are valid
only for row events of new type using the canonical format. The old type events
should use old versions of these methods.
If you remember, the original structure of event applying code was as follows.
- main method is do_apply_event() and it is defined in a base of all rows
events: Rows_log_event class.
- this method uses helper do_xxxx() functions which are virtual and redefined in
leaf classes *_rows_log_event.
Now consider, for example, Write_rows_log_event_old class which inherits from
Write_rows_log_event which inherits from Rows_log_event. The do_apply_event()
inherited from Rows_log_event is modified by my patch. Thus in
Write_rows_log_event_old this method should be overwritten with the old
implementation. The same goes for {Update,Delete}_rows_log_event_old.
Redefining do_apply_event() in each of the *_rows_log_event_old classes is not
an option because of code repetition. Therefore I put the old implementation of
do_apply_event() into a separate class Rows_log_event_old and make it additional
base of all *_rows_log_event_old classes. In the classes I have a trivial
redefinition of do_apply_event() which calls the version defined in
Rows_log_event_old.
Note that Rows_log_event_old doesn't inherit from Rows_log_event because then we
would have double inheritance like this:
Rows_log_event_old - Rows_log_event
/
Write_rows_log_event_old
\
Write_rows_log_event - Rows_log_event
Therefore I treat Rows_log_event as a mere container for the old implementations
of methods and don't put Rows_log_event as its base:
Rows_log_event_old
/
Write_rows_log_event_old
\
Write_rows_log_event - Rows_log_event
But do_apply_event() operates on instances of Rows_log_event class. Therefore I
must pass pointer to Rows_log_event instance to the method and make
Rows_log_event_old a friend of Rows_log_event so that it can access all members.
This leads to the following solutions:
Rows_log_event_old::do_apply_event(Rows_log_event *,...)
Write_rows_log_event_old::do_apply_event(...)
{
Rows_log_event_old::do_apply_event(this,...)
}
I agree that making this method virtual makes no sense. It should not be static
because it uses do_xxxx() primitives declared in Rows_log_event_old class and
implemented in *_rows_log_event_old classes.
And here we come to the most suspicious part of my code. For example let's take
do_exec_row() primitive redefined in Write_rows_log_event_old. The class
inherits this method from both Rows_log_event_old and Write_rows_log_event and
in both these classes the method is declared virtual. But, AFAIK, this should
not be a problem. After redefining do_exec_row() in Write_rows_log_event_old,
this definition will be always used. For example:
Write_rows_log_event_old ev;
ev->do_apply_event(...)
Rows_log_event_old::do_apply_event(ev,...)
ev->do_exec_row(...) // this will call version defined in
// Write_rows_log_event_old
Note also that the implementation of do_exec_row() in Write_rows_log_event_old
has access to all members of Write_rows_log_event as the latter class is a base
for the former.
Do you see any serious problems with this design which I have missed? Do you
have ideas how to do it better?
Rafal
