maria-developers team mailing list archive
Mailing list archive
Re: MDEV-6877 - binlog_row_image implementation advice
Vicențiu Ciorbaru <vicentiu@xxxxxxxxxxx>
Kristian Nielsen <knielsen@xxxxxxxxxxxxxxx>
Thu, 25 Jun 2015 09:36:41 +0200
<CACzaYTMr-q-S9m=mYyx7nc7oXCYzywatq1TbPJnT4Oo67_shiA@mail.gmail.com> ("Vicențiu Ciorbaru"'s message of "Mon, 22 Jun 2015 07:41:57 +0000")
Gnus/5.13 (Gnus v5.13) Emacs/24.4 (gnu/linux)
Vicențiu Ciorbaru <vicentiu@xxxxxxxxxxx> 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
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,
> + 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, 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).