Hi Alfranio,
On Tue, 2009-03-24 at 16:46 +0000, Alfranio Correia wrote:
> Excellent work. Sorry for the delay.
>
Thank you.
> See some comments in-line.
>
> Luis Soares wrote:
> > #At file:///home/lsoares/Workspace/mysql-server/bugfix/40045/6.0-rpl/ based on
> revid:serge.kozlov@stripped
> >
> > 2831 Luis Soares 2009-03-20
> > BUG#40045: Replication failure on RBR + no PK + UPDATE + integer key +
> > char with no key
> >
> > Row based replication would break if updating a row and using a key
> > for searching for the correct row to update. This was observed when
> > using MyISAM storage engine on the slave. Since MyISAM does not
> > provide partial row fetch (HA_PARTIAL_COLUMN_READ) , the comparison
> > between the row fetched on the slave and the before image (BI)
> > replicated from master would fail. This happened because the BI (which
> > sometimes was only part of the row) would always be compared against a
> > complete row.
> >
> > Furthermore, the read set mask was not being properly set when using
> > keys (not unique nor primary) which would lead to partial row matches,
> > thence changes might end up in the wrong row.
> >
> > This patch addresses this by changing the record_compare function for
> > taking into consideration the fields set on the readset and the engine
> > used on the slave. Furthermore, it also changes the construction of
> > the read set on the master by forcing the setting of the appropriate
> > fields (the ones that uniquely identify the row).
> > @ mysql-test/extra/rpl_tests/rpl_row_record_find.test
> > Shared part of the test battery while testing with different engines.
> > @ mysql-test/suite/rpl/r/rpl_row_record_find.result
> > Result file.
> > @ mysql-test/suite/rpl/t/rpl_row_record_find.test
> > Test case that checks for different combinations of engines and
> > updates/deletes.
> > @ sql/log_event.cc
> > Changed the record_compare function to compare the entire record
> > when possible, or just the fields marked in the read_set otherwise.
> > @ sql/log_event_old.cc
> > Changed the record_compare function to compare the entire record
> > when possible, or just the fields marked in the read_set otherwise.
> > @ sql/records.cc
> > Added changes for forcing marking the correct read_set when binlogging.
> >
> > added:
> > mysql-test/extra/rpl_tests/rpl_row_record_find.test
> > mysql-test/suite/rpl/r/rpl_row_record_find.result
> > mysql-test/suite/rpl/t/rpl_row_record_find.test
> > modified:
> > sql/log_event.cc
> > sql/log_event_old.cc
> > sql/records.cc
> > === added file 'mysql-test/extra/rpl_tests/rpl_row_record_find.test'
> > --- a/mysql-test/extra/rpl_tests/rpl_row_record_find.test 1970-01-01 00:00:00
> +0000
> > +++ b/mysql-test/extra/rpl_tests/rpl_row_record_find.test 2009-03-20 13:17:10
> +0000
> >
>
> I would put this in suite/rpl/include and rename it to *.inc.
I would keep it as it is. It is for a specific test only, so I will keep
the MO that already exists for some replication tests.
> > @@ -0,0 +1,161 @@
> > +# USAGE:
> > +# Before including this file the following variables should be set:
> > +# * $master_engine
> > +# * slave_engine
> > +#
> > +# Example:
> > +#
> > +# let $master_engine= Falcon;
> > +# let $slave_engine= MyISAM;
> > +#
> > +# -- source extra/rpl_tests/rpl_row_record_find.test
> > +#
> > +
> > +
> > +connection master;
> > +
> > +--disable_warnings
> > +DROP TABLE IF EXISTS t;
> > +--enable_warnings
> > +
> > +sync_slave_with_master;
> > +connection master;
> > +
> > +let $i= 7;
> > +while($i)
> > +{
> > + connection master;
> > + SET SQL_LOG_BIN=0;
> > +
> > + if (`SELECT $i=1`) {
> >
> This is not compliant to the code guide-lines.
> Just a joke ;).
Eheh.
> > + --echo ******* TEST: No keys ($master_engine -> $slave_engine)
> > + --eval CREATE TABLE t (c1 int, c2 char(1), c3 char(1)) engine=
> $master_engine;
> > + connection slave;
> > + --eval CREATE TABLE t (c1 int, c2 char(1), c3 char(1)) engine=
> $slave_engine;
> > + }
> > + if (`SELECT $i=2`)
> > + {
> > + --echo ******* TEST: One Key ($master_engine -> $slave_engine)
> > + --eval CREATE TABLE t (c1 int, c2 char(1), c3 char(1), key(c1)) engine=
> $master_engine;
> > + connection slave;
> > + --eval CREATE TABLE t (c1 int, c2 char(1), c3 char(1), key(c1)) engine=
> $slave_engine;
> > + }
> > + if (`SELECT $i=3`)
> > + {
> > + --echo ****** TEST: One Composite Key ($master_engine -> $slave_engine)
> > + --eval CREATE TABLE t (c1 int, c2 char(1), c3 char(1), key(c1,c2)) engine=
> $master_engine;
> > + connection slave;
> > + --eval CREATE TABLE t (c1 int, c2 char(1), c3 char(1), key(c1,c2)) engine=
> $slave_engine;
> > + }
> > + if (`SELECT $i=4`)
> > + {
> > + --echo ****** TEST: One Unique Key ($master_engine -> $slave_engine)
> > + --eval CREATE TABLE t (c1 int, c2 char(1), c3 char(1), unique key(c1))
> engine= $master_engine;
> > + connection slave;
> > + --eval CREATE TABLE t (c1 int, c2 char(1), c3 char(1), unique key(c1))
> engine= $slave_engine;
> > + }
> > + if (`SELECT $i=5`)
> > + {
> > + --echo ****** TEST: One Composite Unique Key ($master_engine ->
> $slave_engine)
> > + --eval CREATE TABLE t (c1 int, c2 char(1), c3 char(1), unique key(c1,c2))
> engine= $master_engine;
> > + connection slave;
> > + --eval CREATE TABLE t (c1 int, c2 char(1), c3 char(1), unique key(c1,c2))
> engine= $slave_engine;
> > + }
> > + if (`SELECT $i=6`)
> > + {
> > + --echo ****** TEST: One Primary Key ($master_engine -> $slave_engine)
> > + --eval CREATE TABLE t (c1 int, c2 char(1), c3 char(1), primary key(c1))
> engine= $master_engine;
> > + connection slave;
> > + --eval CREATE TABLE t (c1 int, c2 char(1), c3 char(1), primary key(c1))
> engine= $slave_engine;
> > + }
> > + if (`SELECT $i=7`)
> > + {
> > + --echo ****** TEST: One Composite Primary Key ($master_engine ->
> $slave_engine)
> > + --eval CREATE TABLE t (c1 int, c2 char(1), c3 char(1), primary key(c1,c2))
> engine= $master_engine;
> > + connection slave;
> > + --eval CREATE TABLE t (c1 int, c2 char(1), c3 char(1), primary key(c1,c2))
> engine= $slave_engine;
> > + }
> > +
> > + connection master;
> > + SET SQL_LOG_BIN=1;
> > +
> > + connection master;
> > + INSERT INTO t VALUES (1, '1', '3' );
> > + INSERT INTO t VALUES (2, '2', '3' );
> > + INSERT INTO t VALUES (3, '9', '9' );
> > +
> > + sync_slave_with_master;
> > +
> > + --echo ** WITHOUT KEY
> > + connection master;
> > + UPDATE t SET c3 = '4';
> > +
> > + sync_slave_with_master;
> > +
> > + let $diff_table_1=master:test.t;
> > + let $diff_table_2=slave:test.t;
> > + source include/diff_tables.inc;
> > +
> >
> I would remove the ifs and execute all the statements regardless the table.
> Most likely this will slow down a bit the execution but this is not a
> big deal.
>
> INSERT INTO t VALUES (1, '1', '1' );
> INSERT INTO t VALUES (4, '4', '4' );
> INSERT INTO t VALUES (7, '7', '7' );
>
>
> 1, 1, 1 1, 1, 7 1, 1, 7
> 4, 4, 4 --> 4, 4, 7 --> 4, 4, 7 --> ...
> 7, 7, 7 7, 7, 7 7, 7, 7
>
>
> UPDATE t SET c3 = '7';
>
> UPDATE t SET c3 = '5' WHERE c1 = '1';
> UPDATE t SET c2 = '5' WHERE c1 = '1';
> UPDATE t SET c1 = '5' WHERE c1 = '1';
>
> UPDATE t SET c3 = '8' WHERE c2 = '4';
> UPDATE t SET c1 = '8' WHERE c2 = '4';
> UPDATE t SET c2 = '8' WHERE c2 = '4';
>
> DELETE FROM t where c1=7;
> DELETE FROM t where c2=8;
> DELETE FROM t;
>
> Your patch is changing/fixing how null and not null columns are handled
> by the replication. I think you should test this case too.
I'll check.
>
> > === modified file 'sql/log_event.cc'
> > --- a/sql/log_event.cc 2009-03-04 13:33:47 +0000
> > +++ b/sql/log_event.cc 2009-03-20 13:17:10 +0000
> > @@ -8611,7 +8611,7 @@ void Write_rows_log_event::print(FILE *f
> >
> > Returns TRUE if different.
> > */
> > -static bool record_compare(TABLE *table)
> > +static bool record_compare(TABLE *table, const MY_BITMAP *cols_in_readset)
> > {
> > /*
> > Need to set the X bit and the filler bits in both records since
> > @@ -8643,14 +8643,23 @@ static bool record_compare(TABLE *table)
> > }
> > }
> >
> > - if (table->s->blob_fields + table->s->varchar_fields == 0)
> > + /*
> > + We can compare the record straight away if:
> > + i) there are no blob and varchar fields
> > + ii) all columns were read or the slave SE provides partial reads
> > + */
> > + if ((table->s->blob_fields + table->s->varchar_fields == 0)
> &&
> > + (bitmap_is_set_all(cols_in_readset) ||
> > + (table->file->ha_table_flags() & HA_PARTIAL_COLUMN_READ)))
> > {
> > result= cmp_record(table,record[1]);
> > goto record_compare_exit;
> > }
> >
> > - /* Compare null bits */
> > - if (memcmp(table->null_flags,
> > + /* Compare null bits only if all fields were read or slave SE has partial
> reads */
> > + if ((bitmap_is_set_all(cols_in_readset) ||
> > + (table->file->ha_table_flags() & HA_PARTIAL_COLUMN_READ))
> &&
> > + memcmp(table->null_flags,
> > table->null_flags+table->s->rec_buff_length,
> > table->s->null_bytes))
> > {
> > @@ -8661,10 +8670,14 @@ static bool record_compare(TABLE *table)
> > /* Compare updated fields */
> > for (Field **ptr=table->field ; *ptr ; ptr++)
> > {
> > - if ((*ptr)->cmp_binary_offset(table->s->rec_buff_length))
> > - {
> > - result= TRUE;
> > - goto record_compare_exit;
> > + /* compare field if it is set */
> > + if (bitmap_is_set(cols_in_readset, (*ptr)->field_index))
> > + {
> > + if ((*ptr)->cmp_binary_offset(table->s->rec_buff_length))
> > + {
> > + result= TRUE;
> > + goto record_compare_exit;
> > + }
> > }
> > }
> >
> > @@ -8875,7 +8888,7 @@ int Rows_log_event::find_row(const Relay
> > */
> > DBUG_PRINT("info",("non-unique index, scanning it to find matching
> record"));
> >
> > - while (record_compare(table))
> > + while (record_compare(table, &m_cols))
> > {
> > /*
> > We need to set the null bytes to ensure that the filler bit
> > @@ -8956,7 +8969,7 @@ int Rows_log_event::find_row(const Relay
> > goto err;
> > }
> > }
> > - while (restart_count < 2 && record_compare(table));
> > + while (restart_count < 2 && record_compare(table,
> &m_cols));
> >
> > /*
> > Note: above record_compare will take into accout all record fields
> >
> > === modified file 'sql/log_event_old.cc'
> > --- a/sql/log_event_old.cc 2008-12-10 14:30:52 +0000
> > +++ b/sql/log_event_old.cc 2009-03-20 13:17:10 +0000
> > @@ -313,7 +313,7 @@ last_uniq_key(TABLE *table, uint keyno)
> >
> > Returns TRUE if different.
> > */
> > -static bool record_compare(TABLE *table)
> > +static bool record_compare(TABLE *table, const MY_BITMAP *cols_in_readset)
> > {
> > /*
> > Need to set the X bit and the filler bits in both records since
> > @@ -342,16 +342,25 @@ static bool record_compare(TABLE *table)
> > }
> > }
> >
> > - if (table->s->blob_fields + table->s->varchar_fields == 0)
> > + /*
> > + We can compare the record straight away if:
> > + i) there are no blob and varchar fields
> > + ii) all columns were read or the slave SE provides partial reads
> > + */
> > + if ((table->s->blob_fields + table->s->varchar_fields == 0)
> &&
> > + (bitmap_is_set_all(cols_in_readset) ||
> > + (table->file->ha_table_flags() & HA_PARTIAL_COLUMN_READ)))
> > {
> > result= cmp_record(table,record[1]);
> > goto record_compare_exit;
> > }
> >
> > - /* Compare null bits */
> > - if (memcmp(table->null_flags,
> > - table->null_flags+table->s->rec_buff_length,
> > - table->s->null_bytes))
> > + /* Compare null bits only if all fields were read or slave SE has partial
> reads */
> > + if ((bitmap_is_set_all(cols_in_readset) ||
> > + (table->file->ha_table_flags() & HA_PARTIAL_COLUMN_READ))
> &&
> > + memcmp(table->null_flags,
> > + table->null_flags+table->s->rec_buff_length,
> > + table->s->null_bytes))
> > {
> > result= TRUE; // Diff in NULL value
> > goto record_compare_exit;
> > @@ -360,10 +369,14 @@ static bool record_compare(TABLE *table)
> > /* Compare updated fields */
> > for (Field **ptr=table->field ; *ptr ; ptr++)
> > {
> > - if ((*ptr)->cmp_binary_offset(table->s->rec_buff_length))
> > - {
> > - result= TRUE;
> > - goto record_compare_exit;
> > + /* compare field if it is set */
> > + if (bitmap_is_set(cols_in_readset, (*ptr)->field_index))
> > + {
> > + if ((*ptr)->cmp_binary_offset(table->s->rec_buff_length))
> > + {
> > + result= TRUE;
> > + goto record_compare_exit;
> > + }
> > }
> > }
> >
> > @@ -761,7 +774,7 @@ static int find_and_fetch_row(TABLE *tab
> > DBUG_RETURN(0);
> > }
> >
> > - while (record_compare(table))
> > + while (record_compare(table, table->read_set))
> > {
> > int error;
> >
> > @@ -837,7 +850,7 @@ static int find_and_fetch_row(TABLE *tab
> > DBUG_RETURN(error);
> > }
> > }
> > - while (restart_count < 2 && record_compare(table));
> > + while (restart_count < 2 && record_compare(table,
> table->read_set));
> >
> > /*
> > Have to restart the scan to be able to fetch the next row.
> > @@ -2387,7 +2400,7 @@ int Old_rows_log_event::find_row(const R
> > */
> > DBUG_PRINT("info",("non-unique index, scanning it to find matching
> record"));
> >
> > - while (record_compare(table))
> > + while (record_compare(table, &m_cols))
> > {
> > /*
> > We need to set the null bytes to ensure that the filler bit
> > @@ -2463,7 +2476,7 @@ int Old_rows_log_event::find_row(const R
> > DBUG_RETURN(error);
> > }
> > }
> > - while (restart_count < 2 && record_compare(table));
> > + while (restart_count < 2 && record_compare(table,
> &m_cols));
> >
> > /*
> > Note: above record_compare will take into accout all record fields
> >
> > === modified file 'sql/records.cc'
> >
>
> The code below is duplicate, please put it in a function.
> And I think you should remove the test
> if (thd->current_stmt_binlog_row_based && mysql_bin_log.is_open()) and
> always execute the code. Theoretically, you can do it.
I will put it in a function, but I don't think removing the check is
good idea. read_set handling is already taken care by mysql_update and
mysql_delete functions. I will be moving the code in records.cc into a
function and call that inside mysql_update and mysql_delete, instead of
records.cc. The execution flow should become more clear and the side
effects should be self-contained.
> You need to check this patch with someone from the optimizer/runtime
> team. It may happen that the changed flags below are used during the
> optimization
> phase. I don't think this should have impact on the correctness though.
Ok.
> Please, not as part of this bug but either leave a comment in some
> place or ask someone to
> test selects + combined with updates and deletes.
>
> > --- a/sql/records.cc 2008-08-11 12:40:09 +0000
> > +++ b/sql/records.cc 2009-03-20 13:17:10 +0000
> > @@ -67,6 +67,27 @@ void init_read_record_idx(READ_RECORD *i
> > info->record= table->record[0];
> > info->print_error= print_error;
> >
> > + /* sets read set needed for RBR */
> > + if (thd->current_stmt_binlog_row_based &&
> mysql_bin_log.is_open())
> > + {
> > + /*
> > + If the handler has no cursor capabilites, or we have row-based
> > + logging active for the current statement, we have to read either
> > + the primary key, the hidden primary key or all columns to be
> > + able to do an update
> > + */
> > + if (table->s->primary_key == MAX_KEY ||
> > + !(table->file->ha_table_flags() & HA_PARTIAL_COLUMN_READ))
> > + table->file->use_hidden_primary_key(); /* alias for
> use_all_columns() */
> > + else
> > + {
> > + /* mark the primary key */
> > +
> table->mark_columns_used_by_index_no_reset(table->s->primary_key,
> > + table->read_set);
> > + table->file->column_bitmaps_signal();
> > + }
> > + }
> > +
> > table->status=0; /* And it's always found */
> > if (!table->file->inited)
> > table->file->ha_index_init(idx, 1);
> > @@ -174,6 +195,27 @@ void init_read_record(READ_RECORD *info,
> > info->file= table->file;
> > info->forms= &info->table; /* Only one table */
> >
> > + /* sets read set needed for RBR */
> > + if (thd->current_stmt_binlog_row_based &&
> mysql_bin_log.is_open())
> > + {
> > + /*
> > + If the handler has no cursor capabilites, or we have row-based
> > + logging active for the current statement, we have to read either
> > + the primary key, the hidden primary key or all columns to be
> > + able to do an update
> > + */
> > + if (table->s->primary_key == MAX_KEY ||
> > + !(table->file->ha_table_flags() & HA_PARTIAL_COLUMN_READ))
> > + table->file->use_hidden_primary_key(); /* alias for
> use_all_columns() */
> > + else
> > + {
> > + /* mark the primary key */
> > +
> table->mark_columns_used_by_index_no_reset(table->s->primary_key,
> > + table->read_set);
> > + table->file->column_bitmaps_signal();
> > + }
> > + }
> > +
> > if (table->s->tmp_table == NON_TRANSACTIONAL_TMP_TABLE &&
> > !table->sort.addon_field)
> > (void) table->file->extra(HA_EXTRA_MMAP);
> >
> > ------------------------------------------------------------------------
> >
> > This body part will be downloaded on demand.
>
> Cheers.
Regards,
Luís
--
Luís
