Andrei Elkin wrote:
> Mats, hej.
>
> The patch is good. Actually, it's even very good :)
>
> I have only some minor suggestions.
>
>
>
>> Below is the list of changes that have just been committed into a local
>> 6.0 repository of mats. When mats does a push these changes will
>> be propagated to the main repository and, within 24 hours after the
>> push, to the public repository.
>> For information on how to access the public repository
>> see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html
>>
>> ChangeSet@stripped, 2007-12-18 18:26:54+01:00, mats@stripped +10
> -0
>> BUG#33055 (Replication fails for UPDATE when using falcon Storage engine):
>>
>> The read- and write-sets were not respected when deciding what fields to
>> transfer to the slave, nor when writing the row to the handler on the sla-
>> ve side.
>>
>
> How about to add this paragraph that clarifies what is "to respect".
>
> It was assumed that all fields are presented in the record which is
> not true anymore since replication of falcon (ndb actually consults
> the sets but that's via a specific injector interface).
>
Hmmm... Falcon has dire need for the patch, so it is already pushed to
their tree.
>
>>
>> This patch adds code that only writes the columns actually needed to be
>> able to replicate row-based. This means that the columns in the write set
>> of the table is used for ha_write_row(), the columns in the read set for
>> ha_delete_row(), and for ha_update_row() the columns in the read set is
>> used for the before image, and the columns of the write set for the after
>> image.
>>
>> In addition, for cursor-based delete and update, the necessary bits are
>> set to be able to get enough information to locate the affected row on
>> the slave side, which means that it behaves as if HA_PRIMARY_KEY_REQUIRED_
>> FOR_DELETE was set for the table when row-based logging is active.
>>
>> mysql-test/suite/rpl/r/rpl_temporary_errors.result@stripped, 2007-12-18
> 18:26:43+01:00, mats@stripped +2 -2
>> Result change.
>>
>> sql/handler.cc@stripped, 2007-12-18 18:26:44+01:00,
> mats@stripped +6 -23
>> The function binlog_row_logging_function() does not need column
>> information any more: that information is fetched directly from
>> the read- and write-sets of the table instead. Hence, an unused
>> initialization of a bitmap is removed, as well as error handling
>> code for that initialization.
>>
>> sql/log_event.cc@stripped, 2007-12-18 18:26:44+01:00,
> mats@stripped +15 -25
>> Passing columns to new unpack_current_row() function: it now
>> takes a column bitmap with the columns to unpack. When preparing
>> the record before filling it in, the column vector now has to be
>> passed so that the function knows what columns to initialize.
>>
>> The constructors for Write_rows, Delete_rows, and Update_rows
>> now use either the read- or write-set of the table when packing
>> rows for transfer, so the column vector parameter was removed.
>>
>
>
>> As a consequence, an overloaded version of the Update_rows
>> constructor was removed.
>>
>>
>
> I see that hunk. Still, why do you call it "overloaded"?
>
The term "overloaded" in C++ means that there are several functions with
the same name, but with different signatures (i.e., different number of
parameters, or different types of at least one parameter).
>
>
>> sql/log_event.h@stripped, 2007-12-18 18:26:44+01:00,
> mats@stripped +8 -21
>> The function unpack_current_row() now takes an extra column
>> vector giving the columns to unpack and passes that to
>> ::unpack_row() instead of m_cols previously (it can now be
>> either m_cols or m_cols_ai, depending on situation).
>>
>> Removing redundant column vector parameter from constructors
>> for Write_rows, Delete_rows, and Update_rows and removing
>> a redundant constructor.
>>
>> Removing column vector parameters to binlog_row_logging_
>> function() since they are no longer needed.
>>
>> sql/log_event_old.h@stripped, 2007-12-18 18:26:45+01:00,
> mats@stripped +6 -6
>> Removing redundant column vector parameter from constructors
>> for Write_rows, Delete_rows, and Update_rows and removing
>> a redundant constructor.
>>
>> sql/rpl_record.cc@stripped, 2007-12-18 18:26:45+01:00,
> mats@stripped +55 -21
>> Adding debug printouts to be able to debug packing and unpacking of rows.
>>
>> Adding column vector to prepare_record(). The function prepare_record()
>> now initializes all fields of the record, not just the "extra" ones that
>> could be present on a slave with more columns. The function still prints
>> an error for extra fields that does not have a default value (in the same
>> manner as the old function), but does not print anything for columns that
>> are present on the master and the slave.
>>
>>
>
> Wrt to prepare_record() i did not find the heading comment of the
> function have been changed, and they should have done.
>
Ah, OK. Will fix that. Will push it as a separate patch though.
>
>
>> sql/rpl_record.h@stripped, 2007-12-18 18:26:45+01:00,
> mats@stripped +1 -1
>> Adding column vector parameter to prepare_record().
>>
>> sql/sql_class.cc@stripped, 2007-12-18 18:26:45+01:00,
> mats@stripped +13 -24
>> Columns bitmap and number of columns no longer needed for binlog_*_row().
>> The correct columns are fetched from the read- and write-set of the table
>> instead. Consequence: binlog_prepare_pending_rows_event() does not need
>> this info either.
>>
>> The function pack_row() is called with either the read- or the write-set
>> of the table.
>>
>> sql/sql_class.h@stripped, 2007-12-18 18:26:46+01:00,
> mats@stripped +2 -7
>> Columns bitmap and number of columns no longer needed for binlog_*_row().
>> The correct columns are fetched from the read- and write-set of the table
>> instead. Consequence: binlog_prepare_pending_rows_event() does not need
>> this info either.
>>
>> sql/table.cc@stripped, 2007-12-18 18:26:46+01:00, mats@stripped
> +12 -6
>> Columns marked for delete are marked in the same way as if
>> HA_PRIMARY_KEY_REQUIRED_FOR_DELETE when row-based replication
>> is active since the primary key (or entire row) has to be
>> transfered to the slave. There is no way that the slave can
>> know what row the master's cursor is positioned on when del-
>> eting a row. This is done for both DELETE and UPDATE.
>>
>> diff -Nrup a/mysql-test/suite/rpl/r/rpl_temporary_errors.result
> b/mysql-test/suite/rpl/r/rpl_temporary_errors.result
>> --- a/mysql-test/suite/rpl/r/rpl_temporary_errors.result 2007-10-24 16:02:32
> +02:00
>> +++ b/mysql-test/suite/rpl/r/rpl_temporary_errors.result 2007-12-18 18:26:43
> +01:00
>> @@ -44,7 +44,7 @@ Master_User root
>> Master_Port MASTER_PORT
>> Connect_Retry 1
>> Master_Log_File master-bin.000001
>> -Read_Master_Log_Pos 408
>> +Read_Master_Log_Pos 404
>> Relay_Log_File #
>> Relay_Log_Pos #
>> Relay_Master_Log_File master-bin.000001
>>
>
> It's not obvious why, please add a comment to the file's change
> summary (as i see it's because the size of the cols map is changed).
>
Yes.
>
>> @@ -59,7 +59,7 @@ Replicate_Wild_Ignore_Table
>> Last_Errno 0
>> Last_Error
>> Skip_Counter 0
>> -Exec_Master_Log_Pos 408
>> +Exec_Master_Log_Pos 404
>> Relay_Log_Space #
>> Until_Condition None
>> Until_Log_File
>> diff -Nrup a/sql/handler.cc b/sql/handler.cc
>> --- a/sql/handler.cc 2007-12-05 20:44:35 +01:00
>> +++ b/sql/handler.cc 2007-12-18 18:26:44 +01:00
>> @@ -4516,35 +4516,18 @@ namespace
>>
>> if (check_table_binlog_row_based(thd, table))
>> {
>> - MY_BITMAP cols;
>> - /* Potential buffer on the stack for the bitmap */
>> - uint32 bitbuf[BITMAP_STACKBUF_SIZE/sizeof(uint32)];
>> - uint n_fields= table->s->fields;
>> - my_bool use_bitbuf= n_fields <= sizeof(bitbuf)*8;
>> -
>> + DBUG_DUMP("read_set 10", (uchar*) table->read_set->bitmap,
> (table->s->fields + 7) / 8);
>>
This one should go. I though I had removed all of them...
Just my few cents,
Mats Kindahl
--
Mats Kindahl
Lead Software Developer
Replication Team
MySQL AB, www.mysql.com
