Re: bzr commit into mysql-5.1-bugteam branch (kristofer.pettersson:2842) Bug#40127

[Davi Arnaut]

Hi Kristofer,

On 3/27/09 5:18 AM, Kristofer Pettersson wrote:
> #At file:///home/thek/Development/cpp/mysqlbzr/51-bug40127/ based on
> revid:gni@stripped
>
>   2842 Kristofer Pettersson	2009-03-27
>        Bug#40127 Multiple table DELETE IGNORE hangs on foreign key constraint
> violation
>                  on 5.0

[..]

>
> === modified file 'sql/sql_delete.cc'
> --- a/sql/sql_delete.cc	2009-02-09 22:51:59 +0000
> +++ b/sql/sql_delete.cc	2009-03-27 08:18:14 +0000
> @@ -709,6 +709,8 @@ bool multi_delete::send_data(List<Item>
>     TABLE_LIST *del_table;
>     DBUG_ENTER("multi_delete::send_data");
>
> +  bool ignore= thd->lex->current_select->no_error;
> +
>     for (del_table= delete_tables;
>          del_table;
>          del_table= del_table->next_local, secure_counter++)
> @@ -731,7 +733,25 @@ bool multi_delete::send_data(List<Item>
>                                               TRG_ACTION_BEFORE, FALSE))
>           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 the IGNORE option is used errors caused by ha_delete_row don't
> +        have to stop the iteration.
> +      */
> +      if (!ignore&&  error)
> +      {
> +        table->file->print_error(error,MYF(0));
> +        DBUG_RETURN(1);
> +      }
> +
> +      /*
> +        Increase the reported number of deleted rows only if no error occurred
> +        during ha_delete_row.
> +        Also, don't execute the AFTER trigger if the row operation failed.
> +      */
> +      if (!error)
>         {
>           deleted++;
>           if (!table->file->has_transactions())
> @@ -741,11 +761,6 @@ bool multi_delete::send_data(List<Item>
>                                                 TRG_ACTION_AFTER, FALSE))
>             DBUG_RETURN(1);
>         }
> -      else
> -      {
> -        table->file->print_error(error,MYF(0));
> -        DBUG_RETURN(1);
> -      }
>       }

Those changes are not really necessary, they add more branches and are 
bug prone (as one was inserted the first time you went with this 
approach). Something like this is enough:

@@ -741,7 +743,11 @@ bool multi_delete::send_data(List<Item>
                                                TRG_ACTION_AFTER, FALSE))
            DBUG_RETURN(1);
        }
-      else
+      /*
+        If the IGNORE option is used errors caused by ha_delete_ ..
+        have to stop the iteration.
+      */
+      else if (!ignore)


>       else
>       {
> @@ -834,6 +849,11 @@ int multi_delete::do_deletes()
>   {
>     int local_error= 0, counter= 0, tmp_error;
>     bool will_batch;
> +  /*
> +    If the IGNORE option is used all non fatal errors will be translated
> +    to warnings and we should not break the row-by-row iteration.
> +  */
> +  bool ignore= thd->lex->current_select->no_error;
>     DBUG_ENTER("do_deletes");
>     DBUG_ASSERT(do_delete);
>
> @@ -872,18 +892,34 @@ int multi_delete::do_deletes()
>           local_error= 1;
>           break;
>         }
> -      if ((local_error=table->file->ha_delete_row(table->record[0])))
> +
> +      local_error=table->file->ha_delete_row(table->record[0]);
> +
> +      /*
> +        If the IGNORE option is used errors caused by ha_delete_row don't
> +        have to stop the iteration.
> +      */
> +      if (local_error && !ignore)
>         {
> -	table->file->print_error(local_error,MYF(0));
> -	break;
> +        table->file->print_error(local_error,MYF(0));
> +        break;
>         }
> -      deleted++;
> -      if (table->triggers&&
> -          table->triggers->process_triggers(thd, TRG_EVENT_DELETE,
> -                                            TRG_ACTION_AFTER, FALSE))
> +
> +      /*
> +        Increase the reported number of deleted rows only if no error occurred
> +        during ha_delete_row.
> +        Also, don't execute the AFTER trigger if the row operation failed.
> +      */
> +      if (!local_error)
>         {
> -        local_error= 1;
> -        break;
> +        deleted++;
> +        if (table->triggers&&
> +            table->triggers->process_triggers(thd, TRG_EVENT_DELETE,
> +                                              TRG_ACTION_AFTER, FALSE))

Please add a test case for this. Add a after action trigger that 
increments a variables (or inserts on a table) and make the second row 
on the table to error out on the FK constraint.

> +        {
> +          local_error= 1;
> +          break;
> +        }
>         }
>       }

Again, just use the above scheme.

OK to push with the above addressed.

Regards,

-- Davi Arnaut