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.
> * 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.
> * 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.
>> @@ -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; ?
>> @@ -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.
>> + 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.
>> @@ -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.
>> + 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.
>> +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
Rafal
