Hi,
OK. I'm fine with that.
/Matz
Rafal Somla wrote:
> Hi,
>
> This is a second issue which needs clarification. Tell me if you
> disagree. (there is more to come...).
>
> Mats Kindahl wrote:
>>> +int Rows_log_event::find_and_fetch_row()
>>> +{
>>> + DBUG_ENTER("find_and_fetch_row");
>>>
>>> - @pre <code>table->record[0]</code> shall contain the row to
> locate
>>> - and <code>key</code> shall contain a key to use for searching,
> if
>>> - the engine has a key.
>>> -
>>> - @post If the return value is zero,
> <code>table->record[1]</code>
>>> - will contain the fetched row and the internal "cursor" will refer to
>>> - the row. If the return value is non-zero,
>>> - <code>table->record[1]</code> is undefined. In either
> case,
>>> - <code>table->record[0]</code> is undefined.
>>> + DBUG_ASSERT(m_table && m_table->in_use != NULL);
>>>
>>> - @return Zero if the row was successfully fetched into
>>> - <code>table->record[1]</code>, error code otherwise.
>>> - */
>>> + int error;
>>>
>>> -static int find_and_fetch_row(TABLE *table, uchar *key)
>>> -{
>>> - DBUG_ENTER("find_and_fetch_row(TABLE *table, uchar *key, uchar
>>> *record)");
>>> - DBUG_PRINT("enter", ("table: 0x%lx, key: 0x%lx record: 0x%lx",
>>> - (long) table, (long) key, (long) table->record[1]));
>>> + /* unpack row - missing fields get default values */
>>> +
>>> + prepare_record(NULL,m_table,m_width,FALSE /* don't check errors
>>> */); // TODO: shall we check and report errors here?
>>>
>>
>> Yes, I think so. With some exceptions, I think we should try to
>> generate the error where it occurs. That will allow us to generate
>> better error messages. It will also be obvious if an error code is
>> checked or not. No need to do a major rework, though, if that turns
>> out to be the case.
>>
>
> I'm not so sure. Looking at the code before patch, what happened there
> was that inside ev->do_apply_event() for each row first
> ev->do_prepare_row() was called followed by ev->do_exec_row(). Row
> unpacking happened inside do_prepare_row(), e.g.:
>
> int Write_rows_log_event::do_prepare_row(THD *thd, Relay_log_info
> const *rli,
> TABLE *table,
> uchar const *const row_start,
> uchar const **const row_end)
> {
> DBUG_ASSERT(table != NULL);
> DBUG_ASSERT(row_start && row_end);
>
> if (int error= unpack_row(rli, table, m_width, row_start, &m_cols,
> row_end,
> &m_master_reclength, table->write_set,
> WRITE_ROWS_EVENT))
> {
> thd->net.last_errno= error;
> return error;
> }
> bitmap_copy(table->read_set, table->write_set);
> return 0;
> }
>
> The last argument to unpack_row, the event type, decided about error
> checking and reporting. Look at the old code of unpack_row():
>
> (...)
> /*
> Set properties for remaining columns, if there are any. We let the
> corresponding bit in the write_set be set, to write the value if
> it was not there already. We iterate over all remaining columns,
> even if there were an error, to get as many error messages as
> possible. We are still able to return a pointer to the next row,
> so redo that.
>
> This generation of error messages is only relevant when inserting
> new rows.
> */
> for ( ; *field_ptr ; ++field_ptr)
> {
> uint32 const mask= NOT_NULL_FLAG | NO_DEFAULT_VALUE_FLAG;
> Field *const f= *field_ptr;
>
> if (event_type == WRITE_ROWS_EVENT &&
> ((*field_ptr)->flags & mask) == mask)
> {
> rli->report(ERROR_LEVEL, ER_NO_DEFAULT_FOR_FIELD,
> "Field `%s` of table `%s`.`%s` "
> "has no default value and cannot be NULL",
> (*field_ptr)->field_name, table->s->db.str,
> table->s->table_name.str);
> error = ER_NO_DEFAULT_FOR_FIELD;
> }
> else
> f->set_default();
> }
>
> Thus error reporting happened only in case of Write_rows_log_event,
> not for Update_rows_log_event and Delete_rows_log_event. Only the two
> latter classes use find_and_fetch_row() hence to preserve old
> behaviour we should not check for errors here. Perhaps it was not
> correct from the beginning but I think that if this is the case we
> should make a separate bug report. As for this patch, it preserves old
> behaviour changing only things described in the proposed solution.
>
> Rafal
>
--
Mats Kindahl
Lead Software Developer
Replication Team
MySQL AB, www.mysql.com
