Luís Soares wrote:
> Hi Mats,
> Please check the comemnts inline.
>
> On Wed, 2009-03-25 at 10:53 +0100, Mats Kindahl wrote:
>> Luís Soares wrote:
>>> On Tue, 2009-03-24 at 13:16 +0100, Mats Kindahl wrote:
[snip]
>>>>> === 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?
OK. I read your code as if it was necessary to get the hidden primary key. If
that is done elsewhere (where needed), then it is sufficient to call
use_all_columns() here.
> 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.
That's fine.
/Matz
--
Mats Kindahl
Senior Software Engineer
Database Technology Group
Sun Microsystems
