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
