Hi Rafal,
Comments below.
/Matz
Rafal Somla wrote:
> Hi Mats,
>
> Here are more replies to your comments. I save the issue of using
> "gotos" for a separate, last email.
>
> Mats Kindahl wrote:
>> * You are passing in a THD instance that is already available as a
>> member variable in the class.
>
> I'm doing this because I saw in the code that you wrote the following
> declaration:
>
> virtual int do_prepare_row(THD*, Relay_log_info const*, TABLE*,
> uchar const *row_start,
> uchar const **row_end) = 0;
>
> inside Write_rows_log_event class. You added THD parameter despite the
> fact that the class contains a thd member. I thought there was some
> reason for that and played it safe by following your example. Now, if
> you think that Log_event::thd is the correct value to be used here, it
> would be better to use it instead of passing thd as method argument.
> I'll change the code then.
Yes, I think the code I wrote is wrong in this aspect (I think it is a
result of some of the functions not being member functions earlier).
Just to be sure, remove the parameter from the declaration/definition,
and then go though each error to make sure that it's passing the value
of Log_event::thd directly or indirectly.
>
>
>> * Add tests to check the result for both fewer and more columns on
>> the slave's table. The "fewer columns on the slave" tests will
>> fail now, but when you merge you have WL#3228 to handle, so you
>> can just as well add those tests right now.
>
> I'm bit reluctant to do that as it will introduce dependency of my
> patch on WL#3228. That is, after doing it, if one pushes my patch to a
> tree which doesn't contain WL#3228, the tree will get broken (failing
> tests). I think it is better if tests for "less columns on slave" are
> introduced in the patch for WL#3228 or in a separate patch to come
> after both WL#3228 and BUG#21842 fix. Actually I can prepare such a
> patch to add "less slave colums" case to my rpl_ndb_2other test.
OK. That is fine as well, just don't forget to test both cases after the
merge.
>
>> * Avoid qualifying parameters with 'const' in the definition and
>> without in the declaration. This is correct according to the
>> standard, but we have compilers that require the signature to be
>> exactly the same, even the cv-qualifiers for the parameters [sic].
>
> Ok, I'll check that declarations and definitions agree. You also wrote:
>
> >> +int Write_rows_log_event::do_exec_row(THD *thd,
> >> + const
> >> Slave_reporting_capability *const log)
> >>
> >
> > See [1] above.
> >
> > Also, there is a bug in some compiler (HP-UX, what else?) that requires
> > the const qualifier on the parameter to be identical to what it is in
> > the definition, which is not required. I suggest removing the 'const'
> > that is just before 'log'. For some reason, they got the idea that the
> > cv-qualification is part of the signature, even though the standard
> > clearly states that this cv-qualifier is *not* part of the function
> > type. [ISO 14882:2003, section 8.3.5].
>
> I don't understand why you want me to remove const qualifier here.
> Wouldn't it be enough to include it in both declaration and
> definition. After all, I'm not going to modify this pointer and I want
> to inform compiler about that.
Either way, as long as the qualifier for the declaration and definition
is the same.
>
>>> @@ -6028,7 +6031,9 @@ int Rows_log_event::do_apply_event(RELAY
>>>
>>> DBUG_ASSERT(rli->tables_to_lock == NULL &&
>>> rli->tables_to_lock_count == 0);
>>>
>>> - TABLE* table=
>>>
> const_cast<RELAY_LOG_INFO*>(rli)->m_table_map.get_table(m_table_id);
>>> + TABLE* + table= + m_table=
>>>
> const_cast<RELAY_LOG_INFO*>(rli)->m_table_map.get_table(m_table_id);
>>>
>>
>> Why not handle each assignment on a separate line? That will be much
>> easier to read.
>>
>> m_table= const_cast....
>> TABLE *table= m_table;
>>
>
> Hmm, is really
>
> int a= 0;
> int b= a;
>
> that much easier to read than
>
> int a= b= 0; ?
No, but the line I commented on is easier to read in the first form.
>
>
>>> @@ -7044,58 +6968,89 @@ is_duplicate_key_error(int errcode)
>>> return false;
>>> }
>>>
>>> +/**
>>> + Write the current row into @c m_table.
>>>
>>
>> What does it mean to write the current row into m_table? m_table =
>> m_curr_row? If you could elaborate a little (just a little, this is
>> the brief description, so it shouldn't be too long). Something like:
>> "Unpack the contents of the current row and store it into
>> m_table->record[0]".
>>
>
> m_table of type TABLE* represents the table on which we operate. The
> function writes a row into this table (using its handler methods). It
> is not just storing it in m_table->record[0]. Do you still think that
> my description is not adequate? Then please elaborate a bit more.
This is not important, but m_table is variable holding a pointer, so I
just find it unclear on what it means to write into a variable holding a
pointer.
>
>>> + error= unpack_current_row(); // TODO: how to handle errors?
>>> +
>>> + DBUG_DUMP("record[0]", m_table->record[0],
> m_table->s->reclength);
>>> + DBUG_PRINT_BITSET("debug", "write_set = %s", m_table->write_set);
>>> + DBUG_PRINT_BITSET("debug", "read_set = %s", m_table->read_set);
>>>
>>
>> Maybe remove these debug print-outs since you've got it working.
>>
>
> I'm not sure - this code is quite tricky and Andrei is working on
> conflict detection where this type of info can be useful. If you don't
> object I'd leave it here (and other similar places), especially that
> the original code contained such debug info.
OK.
>
>>> @@ -7166,37 +7141,71 @@ replace_record(THD *thd, TABLE *table,
>>> fail, so we're better off just deleting the row and inserting
>>> the correct row.
>>> */
>>> - if (last_uniq_key(table, keynum) &&
>>> - !table->file->referenced_by_foreign_key())
>>> + if (last_uniq_key(m_table, keynum) &&
>>> + !m_table->file->referenced_by_foreign_key())
>>> {
>>> - error=table->file->ha_update_row(table->record[1],
>>> - table->record[0]);
>>> - if (error && error != HA_ERR_RECORD_IS_THE_SAME)
>>> - table->file->print_error(error, MYF(0));
>>> - else
>>> + DBUG_PRINT("info",("Updating row using ha_update_row()"));
>>> + error= m_table->file->ha_update_row(m_table->record[1],
>>> + m_table->record[0]);
>>> + if (error == HA_ERR_RECORD_IS_THE_SAME)
>>> + {
>>> + DBUG_PRINT("info",("ignoring HA_ERR_RECORD_IS_THE_SAME ret.
>>> value from"
>>> + " ha_update_row()"));
>>> error= 0;
>>>
>>
>> Might be an idea to use a switch here: I suspect that there will be a
>> few more error codes to ignore. Not important, though.
>>
>
> Ok, sounds reasonable - I'll change it.
>
>>> @@ -7289,50 +7298,79 @@ record_compare_exit:
>>> return result;
>>> }
>>>
>>> +/**
>>> + Locate the current row in @c m_table.
>>>
>>> -/*
>>> - Find the row given by 'key', if the table has keys, or else use a
>>> table scan
>>> - to find (and fetch) the row.
>>> + The current row is stored in event's row buffer, pointed by @c
>>> m_curr_row + member. Member @c m_width tells how many columns are
>>> there in the row (this
>> s/are there/there are/
>>
>>> + number can be smaller than the number of columns in the table).
>>> It is assumed
>>
>> With the introduction of WL#3228: s/smaller/smaller or larger/
>>
>> I think you will run into this when merging, so better write some
>> tests for it now and see how they are affected by the merge. No need
>> to write any code to handle it right now, though. Better to see how
>> the resulting merged code looks, and fix it there.
>>
>
> Ok, I'll add this as a separate patch.
>
>>> + that event's table is already open and pointed by @c m_table.
>>> +
>>> + If a corresponding record is found in the table it is stored in
>>> + @c m_table->record[0]. Note that when record is located based on
>>> a primary + key, it is possible that the record found differs from
>>> the row being located.
>>> +
>>> + If no key is specified or table does not have keys, a table scan
>>> is used to + find the row. In that case the row should be complete
>>> and contain values for
>>> + all columns. However, it can still be shorter than the table,
>>> i.e. the table + can contain extra columns not present in the row. +
>>> + @returns Error code on failure, 0 on success. If success @c
>>> m_table->record[0] + contains the record found. Also, the internal
>>> "cursor" of the table is + positioned at the record found.
>>>
>>
>> It seems that @retval is used for "enumerated" return values, and
>> @return for more descriptive cases. I think this would be a @retval
>> case.
>>
>> @retval 0 Success
>> @retval errno Error number
>>
>> @post On success, @c m_table->record[0]....
>>
>
> OK, nice suggestion.
>
>>> + /* Select best method for locating the row based on handler
>>> capabilities */
>>> +
>>> + enum { TABLE_SCAN, INDEX, POSITION } method= TABLE_SCAN;
>>> +
>>> + if ((m_table->file->ha_table_flags() &
>>> HA_PRIMARY_KEY_REQUIRED_FOR_POSITION) &&
>>> + m_table->s->primary_key < MAX_KEY)
>>> + method= POSITION;
>>> + else if (m_table->s->keys > 0)
>>> + method= INDEX;
>>>
>>
>> I still don't understand why you insist on this structure.
>>
>
> What is nice about it, in my opinion, is that all the logic deciding
> about the search method is put in one place, making it easy to
> understand how decision is made and also easy to modify the logic.
> Before, the code had structure:
>
> if(<conditions for first method>)
> {
> <use first method>
> }
> else if(<conditions for second method>)
> {
> <use second method>
> }
> else
> {
> <use third method>
> }
>
> Given that the blocks of code are several screens long, it was
> difficult to see when each method is selected.
Define separate functions for each method. I don't think turning it into
a switch makes it easier to read (and yes, the original code was just as
bad in this respect).
>
>>> + DBUG_ASSERT(m_table != NULL);
>>> + DBUG_ASSERT(m_table->s->fields >= m_width);
>>>
>>
>> Good, but this will fire when you merge. No need to do anything right
>> now (the assertion is correct), just hinting that you should remove
>> it after you've made the merge.
>>
>
> Good to know - thanks.
>
>>> @@ -23,6 +23,10 @@
>>>
>>> #include <my_bitmap.h>
>>> #include "rpl_constants.h"
>>> +#ifndef MYSQL_CLIENT
>>> +#include "rpl_record.h"
>>> +#include "rpl_reporting.h"
>>> +#endif
>>>
>>> #define LOG_READ_EOF -1
>>> #define LOG_READ_BOGUS -2
>>> @@ -2135,7 +2139,13 @@ public:
>>> NO_FOREIGN_KEY_CHECKS_F = (1U << 1),
>>>
>>> /* Value of the OPTION_RELAXED_UNIQUE_CHECKS flag in
>>> thd->options */
>>> - RELAXED_UNIQUE_CHECKS_F = (1U << 2)
>>> + RELAXED_UNIQUE_CHECKS_F = (1U << 2),
>>> +
>>> + /*
>> /**
>>
>> This is an excellent comment for Doxygen.
>>
>>> + Indicates that rows in this event are complete, that is contain
>>> + values for all columns of the table.
>>> + */
>>> + COMPLETE_ROWS_F = (1U << 3)
>>> };
>>>
>
> Ok, will do.
>
>
>>> @@ -1,9 +1,961 @@
>>>
>>> #include "mysql_priv.h"
>>> +#ifndef MYSQL_CLIENT
>>> +#include "rpl_rli.h"
>>> +#include "rpl_utility.h"
>>> +#endif
>>> #include "log_event_old.h"
>>> #include "rpl_record_old.h"
>>>
>>> +#define DBUG_PRINT_BITSET(N,FRM,BS) \
>>> + do { \
>>> + char buf[256]; \
>>> + for (uint i = 0 ; i < (BS)->n_bits ; ++i) \
>>> + buf[i] = bitmap_is_set((BS), i) ? '1' : '0'; \
>>> + buf[(BS)->n_bits] = '\0'; \
>>> + DBUG_PRINT((N), ((FRM), buf)); \
>>> + } while (0)
>>>
>>
>> Inline function?
>>
>
> This is just existing code pasted here.
OK.
>
>>> +int Write_rows_log_event_old::do_after_row_operations(TABLE *table,
>>> int error)
>>> +{
>>> + int local_error= 0;
>>> + table->file->extra(HA_EXTRA_NO_IGNORE_DUP_KEY);
>>> + table->file->extra(HA_EXTRA_WRITE_CANNOT_REPLACE);
>>> + /*
>>> + reseting the extra with +
>>> table->file->extra(HA_EXTRA_NO_IGNORE_NO_KEY); + fires bug#27077
>>> + todo: explain or fix
>>> + */
>>> + if ((local_error= table->file->ha_end_bulk_insert()))
>>> + {
>>> + table->file->print_error(local_error, MYF(0));
>>> + }
>>> + return error? error : local_error;
>>> +}
>>>
>>
>> I assume this is just a cut-and-paste of the old code. If not, please
>> tell me and I'll look closer at it.
>>
>
> Yes, you are right. All old row event application code was
> cut-and-pasted from log_event.cc to log_event_old.cc
OK. Then I won't look closer at it.
/Matz
>
> Rafal
--
Mats Kindahl
Lead Software Developer
Replication Team
MySQL AB, www.mysql.com
