Vicențiu Ciorbaru <vicentiu@mariadb.org> writes:
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
https://github.com/MariaDB/server/compare/10.1-MDEV-6877-binlog_row_image
It looks ok. A few comments below. Though generally, it seems best to stay as closely as possible to MySQL code, right? So you should take them only as suggestions, probably. - Kristian.
+ virtual bool read_write_bitmaps_cmp(TABLE *table)
+ case DELETE_ROWS_EVENT:
+ case UPDATE_ROWS_EVENT:
+ case WRITE_ROWS_EVENT:
+ default: + /* + We should just compare bitmaps for Delete, Write + or Update rows events. + */ + DBUG_ASSERT(0);
Why is this a virtual method? There is nothing in the patch that overrides it, at it asserts if called with arbitrary Log_event, so seems it should just be a normal method, maybe even in class Rows_log_event() with a static_cast<> at the call site?
+ // Unpack the current row into m_table->record[0], but with + // a different columns bitmap. + int unpack_current_row(rpl_group_info *rgi, MY_BITMAP const *cols)
The style guide uses /*\n ...\n*/ for multi-line comments I think.
@@ -6217,24 +6237,96 @@ int THD::binlog_delete_row(TABLE* table, bool is_trans,
uchar *row_data= memory.slot(0);
- size_t const len= pack_row(table, cols, row_data, record); + DBUG_DUMP("table->read_set", (uchar*) table->read_set->bitmap, (table->s->fields + 7) / 8);
Overlong line (style guide).