Hi Kristian, I've done more work on binlog row image and I think it is nearly ready to be merged with 10.1. I still need to import all mysql's test cases, however none of our rpl tests fail. There are 2 test cases: rpl.rpl_not_null_innodb rpl.rpl_not_null_myisam that do have a different output when run with minimal row image. This output however is normal, considering the slave and master have different table specifications. It is a corner case that is not treated in MySQL either.
I'd like a review from you for the whole patch, while I work on adding all the relevant tests from MySQL. To me, it seems like a pretty big change with lots of implications. I tried to cover all of them. Let me know if you have any questions or concerns. https://github.com/MariaDB/server/compare/10.1-MDEV-6877-binlog_row_image Regards, Vicențiu On Wed, 29 Apr 2015 at 11:37 Kristian Nielsen <[email protected]> wrote: > Vicențiu Ciorbaru <[email protected]> writes: > > > I've gotten to a "sort of" stable state with binlog_row_image. It's not > > 100% complete as I did not add any tests for the new use cases. > > Also there are a couple of changes that I am not 100% sure if they are > > complete or correct. > > > > I would like some input on these changes: > > > https://github.com/MariaDB/server/compare/10.1-MDEV-6877-binlog_row_image > > I read through the changes (10 commits). Generally, it seems ok. I assume > you have taken the code from the MySQL tree (5.6 or 5.7?), and only changed > it as necessary to work with any local MariaDB changes? > > > The unpack_current_row being a very iffy change for me. I understand why > it > > is needed in the Update_rows_event::do_exec_row code, as the after image > > has other columns that get written as opposed to the before image. Thing > is > > I've just made it in an attempt to fix a HA_ERR_SAME_KEY error that I > kept > > getting on the slave and it seems to have worked. > > I don't understand here. > You wrote "I've just made it" - why could you not just use the same code > that MySQL uses? > But I think you meant something else. Maybe this is caused by some changes > done in one of the trees but not the other, and that needs to be > merged/adapted? > > One thing done in MariaDB is that most code now uses rpl_group_info rather > than Relay_log_info, due to parallel replication. MySQL may have done > something conceptually similar, but different implementation. > > In general, it would seem best to follow MySQL/Oracle code as closely as > possible. We should not have changed the code for applying row events much > compared to MySQL. However, there may of course be other things that MySQL > did that we have not yet merged. > > > Do you think the implementation that MySQL chose is the correct one? The > > I suppose the MySQL one should be correct. In general, there does not seem > to be much going on in the patches (though I'm sure digging up exactly > which > code to carry over has taken a lot of effort on your part). It seems most > of > the required functionality is already in the code. Though it is hard for me > to see if anything might be missing. > > Maybe there are some test cases from MySQL that can be taken over? > Or maybe they run the existing test suite with > --mysqld=--binlog-row-image={NOBLOB,MINIMAL,FULL}. This is something that > we > would benefit a lot from doing also, I think... > > > changes are mostly the same as the MySQL variant, except they have V2 ROW > > LOG EVENTS which seem to pass some extra info which we don't use (need?). > > I think the extra info in V2 rows is only used by NDB, something we do not > have in our tree. And I think reading V2 events were ported by Serg to > MariaDB already (just ignoring the extra info). So I think generally the V2 > can just be ignored, except to be able to read those events... > > Some answers to specific comments you put on github: > > > This code is not used and prevents compilation when changing the > prototype > > of binlog_write_row. As discussed with Sergei Golubchik, this whole code > > is up for the chopping block after this feature is complete. > > Yes. Binlog injector is for NDB, which we do not have in our tree. > > > Same of old log events, but I don't know exactly where these are used. I > > could use a bit of help with this part. > > I think log_event_old.{h,cc} is for backwards compatibility with really old > versions of row-based replication. It may even be for event format that was > never released (only alpha/beta). I think just updating the code without > further action should be ok. I am not aware of a way to test this part of > the code. > > > I actually don't understand why we need this. This was copy pasted from > > MySQL and is on my TODO list to figure out why. > > Well, THD::binlog_prepare_row_images() changes the table->read_set to point > to table->tmp_set, so I suppose that is why they restore it... > > > This doesn't seem to make any sense for me. This is what MySQL does but > > does ha_update_row care? > > Yeah, I don't understand either, but I suppose the comment "Temporary fix > to > find out why it fails [/Matz]" is a clue that it is some debugging code, > possibly forgotten (Mats Kindahl is one of the main replication > developers). You could try asking him in a mail ([email protected]). > > > There are some things that need to be fixed within that patch, but I'm > > looking to know if it's going in the right direction or not, before > > investing any more time into it. > > It seems ok to me. > > Hope this helps, > > - Kristian. > >
_______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
