Hi Mats,
Please check the comemnts inline.
On Wed, 2009-03-25 at 10:53 +0100, Mats Kindahl wrote:
> Luís Soares wrote:
> > Hi Mats,
> >
> > On Tue, 2009-03-24 at 13:16 +0100, Mats Kindahl wrote:
> >> Hi Luis,
> >>
> >> Excellent work!
> >
> > Thanks.
> >> I have some comments though, see them inline below.
> >>
> >> Just my few cents,
> >> Mats Kindahl
> >>
> >> Luis Soares wrote:
>
> [snip]
>
> >>> === 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
>
> [snip]
>
> >>> + connection master;
> >>> + DELETE FROM t WHERE c1 = 1;
> >>> + DELETE FROM t WHERE c1 = 2;
> >>> + DELETE FROM t;
> >> You need a sync and a compare between each of these statements, otherwise
> you
> >> will only make sure that the table is empty after executing the delete.
> >
> > Hmm... The purpose for these deletes + sync + diff_tables is two-fold:
> >
> > i) check that replication does not break because an event would not
> > find the record for deletion on slave. I think this does not need sync
> > between statements, just one at the end of all. If replication breaks,
> > it will break in that sync anyway; This differs from updates, because
> > update events might hit a similar row (partial match) and so replication
> > would not break - that's why I diffed tables between each update. On
> > deletes, if the event does a partial match for a row and deletes it
> > eventually replication will break because a subsequent event will not
> > find the row previoulsy deleted and SQL thread reports error - causing
> > the test to fail.
>
> OK.
Good. ;)
> [snip]
>
> >>> === added file 'mysql-test/suite/rpl/r/rpl_row_record_find.result'
> >>> --- a/mysql-test/suite/rpl/r/rpl_row_record_find.result 1970-01-01
> 00:00:00 +0000
> >>> +++ b/mysql-test/suite/rpl/r/rpl_row_record_find.result 2009-03-20
> 13:17:10 +0000
> >>> @@ -0,0 +1,1080 @@
> >>> +stop slave;
> >>> +drop table if exists t1,t2,t3,t4,t5,t6,t7,t8,t9;
> >>> +reset master;
> >>> +reset slave;
> >>> +drop table if exists t1,t2,t3,t4,t5,t6,t7,t8,t9;
> >>> +start slave;
> >>> +CREATE TABLE t1 ( i1 int, c1 char(1), key ( i1 ));
> >>> +INSERT IGNORE INTO t1 VALUES (1, 'a');
> >>> +UPDATE t1 SET c1 = 'b' WHERE i1 = 1;
> >>> +DROP TABLE t1;
> >>> +CREATE TABLE table1_myisam ( `bit_key` bit, `int_key` int, key
> (`bit_key` ), key (`int_key` ));
> >>> +INSERT IGNORE INTO table1_myisam VALUES ('1', '-2146992385');
> >>> +Warnings:
> >>> +Warning 1264 Out of range value for column 'bit_key' at row 1
> >> Do you need this warning for the test to work correct? If not, I would
> suggest
> >> creating a statement that does not emit a warning (a result mismatch on
> this
> >> warning would indeed catch an error, but since this is not the objective of
> this
> >> test, I would leave that to the test that tests for that).
> >
> > This is the exact test case used for verifying BUG#40007. I did not
> > change it. However, I agree with you that the warning is by no means the
> > purpose of the test. I have two suggestions:
> >
> > i) disable warnings in the statement;
>
> I think this is good enough...
Agree ;).
> > ii) use an integer instead of bit field (as reported in the bug report
> > that it is also exhibits the same behavior).
>
> [snip]
>
> >>> === 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)
> >> You have the table already, why are you passing in the readset instead of
> >> reading it from table->read_set?
> >
> > The parameter is for checking fields against the read_set mask set on
> > the master and not against the read_set on the table reference on the
> > slave. Furthermore, the bitmask for the table->read_set is set to all
> > right before do_before_row_opertions and do_exec_row:
>
> OK.
Good.
> >>> @@ -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))
> >> Is it not possible to ensure that table->read_set == m_cols and just set
> it at
> >> the beginning of the execution of the event (or maybe even only when
> executing
> >> the table_map_event and opening the table)?
> >
> > Actually, you seem to have added something like (in log_event.cc -
> > judging from the Matz signature ;) ):
> >
> > 8743 // Temporary fix to find out why it fails [/Matz]
> > 8744 memcpy(m_table->read_set->bitmap, m_cols.bitmap,
> > (m_table->read_set->n_bits + 7) / 8);
> >
> > This is in find_rows. I am unsure whether there are side effects of
> > setting the read_set to m_cols while executing the table map event.
> > However, I just followed existing protocol. For example, prepare_record
> > takes the table as a parameter and the read_set - m_cols - as well.
>
> There are side effects. IIRC, I tried to do exactly what I suggested, but was
> short on time and did this instead.
>
> I need to have a look at that one later, you can ignore it for now.
I will.
> >>> === modified file 'sql/records.cc'
> >>> --- 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() */
> >> Use_hidden_primary_key() is virtual, so it depends on the storage engine
> whether
> >> this will call use_all_columns() or not. Now, the binary log *always*
> require
> >> *all* columns to be set, otherwise it will not work correctly, so you need
> to
> >> somehow make sure that use_all_columns() is called when the binary log is
> active
> >> (for that table).
> >>
> >> One way would be to add a non-virtual ha_use_hidden_primary_key() function
> that
> >> calls use_all_columns() if the binary log is active, and then calls the
> virtual
> >> use_hidden_primary_key(). The default use_hidden_primary_key() would then
> do
> >> nothing.
> >>
> >> This will require you to change all calls to use_hidden_primary_key(), so if
> you
> >> make the virtual function private, it will not compile unless you have fixed
> all
> >> the calls.
> >
> > What about just replacing the
> > table->file->use_hidden_primary_key();
> > for
> > table->use_all_columns();
> > in the above?
>
> No, that will not work if the use_hidden_primary_key() does something extra, so
> you need to call this function *and* be guaranteed that use_all_columns() is called.
>
> > We are already certain that the we are in RBR and binlog is active
> > (though it is missing the "!table->no_replicate" check).
> >
> > Is your concern that that call to use_hidden_primary_key() might be
> > called elsewhere messing the read_set bits after we set them here?
>
> No, my concern is that there are two things that need to be done: let the
> storage engine use the hidden key, if it has one (e.g., NDB), and set all
> columns if the engine does not support HA_PARTIAL_COLUMN_READ. These are
> orthogonal operations, and the current code will miss calling use_all_column()
> if the storage engine overrides the use_hidden_primary_key() function.
Ok I see your point, but allow me to disagree. That is not
responsibility of this patch. Calling the use_hidden_primary_key()
should be done by the implementation of mysql_update and mysql_delete
(which actually call it indirectly through calls to:
* TABLE::mark_columns_needed_for_update()
* TABLE::mark_columns_needed_for_delete()
respectively).
What we should worry about is to request that all columns are fetched so
that the BI image contains all columns. (Hidden primary key is not our
game. Imagine replicating between NDB and MyISAM whereas hidden primary
key semantics from NDB are meaningless to MyISAM). I may have started
this by using use_hidden_primary_key() instead of use_all_columns() in
the first place.
What do you say?
Btw, I will create a function mark_columns_needed_for_rbr containing the
code currently on records.cc and call this function only on mysql_delete
and mysql_update so that its usage is more self-contained and does not
harm other statements than updates and deletes.
>
> Just my few cents,
> Mats Kindahl
> --
> Mats Kindahl
> Senior Software Engineer
> Database Technology Group
> Sun Microsystems
>
--
Luís
