Davi Arnaut wrote:
[...]
>> This patch corrects this issue by checking for the existance of
>> an IGNORE
>
> existance -> existence
>
ack
>> option before setting an error state during row-by-row delete
>> iteration.
>> @ mysql-test/r/innodb_mysql.result
[...]
>> +
>> +--error 1451
>
> 1451 -> ER_ROW_IS_REFERENCED_2
>
ack
[...]
>> @@ -709,6 +709,8 @@ bool multi_delete::send_data(List<Item>
>> TABLE_LIST *del_table;
>> DBUG_ENTER("multi_delete::send_data");
>>
>> + bool no_error= thd->lex->current_select->no_error;
>
> May I suggest to rename it to ignore? It's the name used elsewhere.
Ok, but rename which one? current_select->no_error or "bool no_error"?
>
> What is the difference between Lex::current_select::no_error and
> Lex::ignore? do you test it?
LEX::ignore is a global state used in the parser used to represent the
IGNORE-token (which we maybe shouldn't be using). It is also used in the
predicate assigning current_select->no_error (in mysql_delete(),
JOIN::optimize() )
I'm quite sure we could consolidate some of our error variables. However
no_error can change independently from LEX::ignore. Example: write_record().
I'm not sure what you're asking when you write "do you test it?". Do you
want me to attempt to remove one of the variables in favor of the other?
>> +
>> for (del_table= delete_tables;
>> del_table;
>> del_table= del_table->next_local, secure_counter++)
>> @@ -729,9 +731,28 @@ bool multi_delete::send_data(List<Item>
>> if (table->triggers&&
>> table->triggers->process_triggers(thd, TRG_EVENT_DELETE,
>> TRG_ACTION_BEFORE, FALSE))
>> - DBUG_RETURN(1);
>
> Do we ignore errors during trigger processing for the other forms
> (INSERT, UPDATE, etc) of IGNORE?
According to documentation and WL4103 it seems that we should interpret
IGNORE as a "best effort" to continue despite errors and we do this
mainly on a row-by-row basis. Not ignoring triggers seems to be counter
intuitive to the IGNORE directive.
Note that WL4103 isn't finished. There some good arguments but no
decision (despite architectural review).
I need to do a new investigation to tell for sure, but I believe that
INSERT uses different approaches depending on bulk-inserts, row-by-row
inserts, delayed inserts or INSERT INTO ... SELECT ..
There is also the concept of thd->abort_on_warning which depends on
lex->ignore and the implicit setting of thd->is_error().
If you want I can extend the tests and patch to treat INSERT, UPDATE,
etc too, but I think we should divide the effort into sprints then.
>
>> + {
>> + /*
>> + If the IGNORE option is used the errors will be translated
>> into
>> + warnings and we should not interrupt the loop.
>> + */
>> + if (!no_error)
>> + DBUG_RETURN(1);
>> + }
>> table->status|= STATUS_DELETED;
>> - if (!(error=table->file->ha_delete_row(table->record[0])))
>> + error=table->file->ha_delete_row(table->record[0]);
>> +
>> + /*
>> + If an error occurred and there is no IGNORE flag;
>> + Signal an error and exit.
>> + */
>
> No need to rework here, just make the else check the flag:
>
> if (!(error=table->file->ha_delete_row(table->record[0])))
> ..
> else if (!ignore)
> print_error
No other reason than that I prefer the first. :-)
No action taken.
[...]
>>
>> @@ -869,21 +897,30 @@ int multi_delete::do_deletes()
>> table->triggers->process_triggers(thd, TRG_EVENT_DELETE,
>> TRG_ACTION_BEFORE, FALSE))
>> {
>> - local_error= 1;
>> - break;
>> + if (!no_errors)
>> + {
>> + local_error= 1;
>> + break;
>> + }
>
> Isn't this a fatal error? It seems we shouldn't ignore fatal errors.
>
I don't think so. It is possible to continue execution to get a partial
success which is the point of IGNORE.
>> }
>> if ((local_error=table->file->ha_delete_row(table->record[0])))
>> {
>> - table->file->print_error(local_error,MYF(0));
>> - break;
>> + if (!no_errors)
>> + {
>> + table->file->print_error(local_error,MYF(0));
>> + break;
>> + }
>
> Please take the opportunity and remove any tabs.
OK
>
>> }
>> deleted++;
>> if (table->triggers&&
>> table->triggers->process_triggers(thd, TRG_EVENT_DELETE,
>> TRG_ACTION_AFTER, FALSE))
>> {
>> - local_error= 1;
>> - break;
>> + if (!no_errors)
>> + {
>> + local_error= 1;
>> + break;
>> + }
>
> Same question about triggers.
>
Same response. :)
Thanks for your time!
