maria-developers team mailing list archive

[Kristian Nielsen <knielsen@xxxxxxxxxxxxxxx>]

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

> 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).