List:Commits« Previous MessageNext Message »
From:Dmitry Lenev Date:October 5 2010 4:18am
Subject:Re: bzr commit into mysql-5.5-runtime branch (davi:3149) Bug#49938
Bug#54678
View as plain text  
Hello Davi!

* Davi Arnaut <davi.arnaut@stripped> [10/10/05 02:05]:
>>> === modified file 'sql/sql_truncate.cc'
>>> --- a/sql/sql_truncate.cc	2010-08-31 10:03:36 +0000
>>> +++ b/sql/sql_truncate.cc	2010-09-29 22:38:57 +0000
>>> @@ -29,145 +29,120 @@
>>>   #include "sql_truncate.h"
>>>
>>>
>>> -/*
>>> -  Delete all rows of a locked table.
>>> -
>>> -  @param  thd           Thread context.
>>> -  @param  table_list    Table list element for the table.
>>> -  @param  rows_deleted  Whether rows might have been deleted.
>>> -
>>> -  @retval  FALSE  Success.
>>> -  @retval  TRUE   Error.
>>> +/**
>>> +  Check if table which is going to be affected by TRUNCATE TABLE
>>> +  is a parent table in some non-self-referencing foreign key and
>>> +  if yes emit an error that it is illegal to truncate such table.
>>> +
>>> +  @param  thd    Thread context.
>>> +  @param  table  Table handle.
>>> +
>>> +  @retval FALSE  This table is not parent in a non-self-referencing foreign
>>> +                 key. Statement can proceed.
>>> +  @retval TRUE   This table is parent in a non-self-referencing foreign
> key,
>>> +                 error was emitted.
>>>   */
>>>
>>>   static bool
>>> -delete_all_rows(THD *thd, TABLE *table)
>>> +fk_truncate_illegal_if_parent(THD *thd, TABLE *table)
>>>   {
>>> -  int error;
>>> -  READ_RECORD info;
>>> -  bool is_bulk_delete;
>>> -  bool some_rows_deleted= FALSE;
>>> -  bool save_binlog_row_based= thd->is_current_stmt_binlog_format_row();
>>> -  DBUG_ENTER("delete_all_rows");
>>> +  FOREIGN_KEY_INFO *f_key_info;
>>> +  List<FOREIGN_KEY_INFO>  f_key_list;
>>> +  List_iterator_fast<FOREIGN_KEY_INFO>  it;
>>>
>>> -  /* Replication of truncate table must be statement based. */
>>> -  thd->clear_current_stmt_binlog_format_row();
>>> +  /* Whether the table is referenced by a foreign key. */
>>> +  if (! table->file->referenced_by_foreign_key())
>>> +    return FALSE;
>>>
>>> -  /*
>>> -    Update handler statistics (e.g. table->file->stats.records).
>>> -    Might be used by the storage engine to aggregate information
>>> -    necessary to allow deletion. Currently, this seems to be
>>> -    meaningful only to the archive storage engine, which uses
>>> -    the info method to set the number of records. Although
>>> -    archive does not support deletion, it becomes necessary in
>>> -    order to return a error if the table is not empty.
>>> -  */
>>> -  error= table->file->info(HA_STATUS_VARIABLE | HA_STATUS_NO_LOCK);
>>> -  if (error&&  error != HA_ERR_WRONG_COMMAND)
>>> -  {
>>> -    table->file->print_error(error, MYF(0));
>>> -    goto end;
>>> -  }
>>> +  table->file->get_foreign_key_list(thd,&f_key_list);
>>>
>>> -  /*
>>> -    Attempt to delete all rows in the table.
>>> -    If it is unsupported, switch to row by row deletion.
>>> -  */
>>> -  if (! (error= table->file->ha_delete_all_rows()))
>>> -    goto end;
>>> +  it.init(f_key_list);
>>>
>>> -  if (error != HA_ERR_WRONG_COMMAND)
>>> +  /* Check whether it is a self-referencing FK. */
>>> +  while ((f_key_info= it++))
>>>     {
>>> -    /*
>>> -      If a transactional engine fails in the middle of deletion,
>>> -      we expect it to be able to roll it back.  Some reasons
>>> -      for the engine to fail would be media failure or corrupted
>>> -      data dictionary (i.e. in case of a partitioned table). We
>>> -      have sufficiently strong metadata locks to rule out any
>>> -      potential deadlocks.
>>> -
>>> -      If a non-transactional engine fails here (that would
>>> -      not be MyISAM, since MyISAM does TRUNCATE by recreate),
>>> -      and binlog is on, replication breaks, since nothing gets
>>> -      written to the binary log.  (XXX: is this a bug?)
>>> -    */
>>> -    table->file->print_error(error, MYF(0));
>>> -    goto end;
>>> +    if (my_strcasecmp(system_charset_info,
> f_key_info->referenced_db->str,
>>> +                      table->s->db.str) ||
>>> +        my_strcasecmp(system_charset_info,
> f_key_info->referenced_table->str,
>>> +                      table->s->table_name.str))
>>> +      break;
>>>     }
>>>
>>> -  /*
>>> -    A workaround for Bug#53696  "Performance schema engine violates the
>>> -    PSEA API by calling my_error()".
>>> -  */
>>> -  if (thd->is_error())
>>> -    goto end;
>>> -
>>
>> AFAIU this bug is still there. Does this mean that this code should
>> have been moved somewhere else and not removed?
>
> Yes, the bug is still here but we don't need the hack anymore because we  
> no longer resort to row-by-row delete. What happens is that now my_error  
> is invoked two times, once by perfschema and another by print_error,  
> which is harmless.

Right. I have figured that after sending out my review :)

>>> -  /* Handler didn't support fast delete. Delete rows one by one. */
>>> -
>>> -  init_read_record(&info, thd, table, NULL, TRUE, TRUE, FALSE);
>>> -
>>> -  /*
>>> -    Start bulk delete. If the engine does not support it, go on,
>>> -    it's not an error.
>>> -  */
>>> -  is_bulk_delete= ! table->file->start_bulk_delete();
>>> +  /* Table is parent in a non-self-referencing foreign key. */
>>> +  if (f_key_list.is_empty() || f_key_info)
>>> +  {
>>> +    my_error(ER_TRUNCATE_ILLEGAL_FK, MYF(0),
> table->s->table_name.str);
>>> +    return TRUE;
>>> +  }
>>
>> Could you please clarify why "f_key_list.is_empty()" part
>> of the above condition is needed?
>
> When we reach empty we know that the table is referenced by a foreign  
> key. In this case, we allow truncate only if all the FKs of this table  
> are self-referencing.
>
> I've updated the comment to:
>
>   /*
>     Table is parent in a non-self-referencing foreign key or the table
>     has a foreign key that refers to another table. The intention is to
>     allow truncate only for tables that do not depend on other tables.
>   */

After looking a bit more at this check and the above comment
I've got even more confused :( I think we need to discuss
this on IRC.

>
> Some background on what is being avoided: Bug#56785

Erghm... So the idea is to prohibit truncation of child tables as well?
Then why do we bail-out of this function if it is not parent in some
foreign key?.

I guess we don't want to do this as this would make MySQL's TRUNCATE TABLE
more restrictive than one implemented by Oracle Database.

And this is yet another, even more incompatible, change!

I think there are other ways to solve bug#56785 (e.g. do proper metadata
locking for parent and child tables).

...


>>> -    thd->transaction.stmt.modified_non_trans_table= TRUE;
>>> -    thd->transaction.all.modified_non_trans_table= TRUE;
>>> +    /* We don't need to load triggers. */
>>> +    DBUG_ASSERT(table_ref->trg_event_map == 0);
>>> +    /*
>>> +      Our metadata lock guarantees that no transaction is reading
>>> +      or writing into the table. Yet, to open a write cursor we need
>>> +      a thr_lock lock. Allow to open base tables only.
>>> +    */
>>> +    table_ref->required_type= FRMTYPE_TABLE;
>>> +    /*
>>> +      Ignore pending FLUSH TABLES since we don't want to release
>>> +      the MDL lock taken above and otherwise there is no way to
>>> +      wait for FLUSH TABLES in deadlock-free fashion.
>>> +    */
>>> +    flags= MYSQL_OPEN_IGNORE_FLUSH | MYSQL_OPEN_SKIP_TEMPORARY;
>>> +    /*
>>> +      Even though we have an MDL lock on the table here, we don't
>>> +      pass MYSQL_OPEN_HAS_MDL_LOCK to open_and_lock_tables
>>> +      since to truncate a MERGE table, we must open and lock
>>> +      merge children, and on those we don't have an MDL lock.
>>> +      Thus clear the ticket to satisfy MDL asserts.
>>> +    */
>>> +    table_ref->mdl_request.ticket= NULL;
>>> +    /* The handler truncate protocol dictates a exclusive lock. */
>>> +    table_ref->mdl_request.set_type(MDL_EXCLUSIVE);
>>>     }
>>
>> Maybe it makes sense to set the type of metadata lock right
>> in the parser?
>>
>
> No, because we first check, with a shared lock, whether the engine  
> supports table recreate.

Right. I suggest to change this. I see no point in taking shared
lock for this check as after your patch we always upgrade metadata
lock after doing it anyway.

> Hum, actually, but this is also poses a problem  
> in a re-execution context, as the type will always be exclusive after  
> the first time the a engine for the table does not support recreate.
>
> Suggestions?

See above.

...

>>> +    ulong timeout= thd->variables.lock_wait_timeout;
>>> +    if (thd->mdl_context.
>>> +        upgrade_shared_lock_to_exclusive(table_ref->mdl_request.ticket,
> timeout))
>>>         DBUG_RETURN(TRUE);
>>> +    tdc_remove_table(thd, TDC_RT_REMOVE_ALL, table_ref->db,
>>> +                     table_ref->table_name, FALSE);
>>>     }
>>
>> Taking into account that we now always upgrade metadata locks
>> for base tables maybe it makes sense to take X metadata lock on
>> table being truncated right from the start?
>
> I think so but I remember that kostja had some desire to keep a shared  
> lock as long as necessary. I asked him once about this but go no 
> response...

See above.

>>> === modified file 'storage/myisammrg/ha_myisammrg.cc'
>>> --- a/storage/myisammrg/ha_myisammrg.cc	2010-09-08 08:25:37 +0000
>>> +++ b/storage/myisammrg/ha_myisammrg.cc	2010-09-29 22:38:57 +0000
>>> @@ -1203,6 +1203,22 @@ ha_rows ha_myisammrg::records_in_range(u
>>>   }
>>>
>>>
>>> +int ha_myisammrg::truncate()
>>> +{
>>> +  int err= 0;
>>> +  MYRG_TABLE *table;
>>> +  DBUG_ENTER("ha_myisammrg::truncate");
>>> +
>>> +  for (table= file->open_tables; table != file->end_table; table++)
>>> +  {
>>> +    if ((err= mi_delete_all_rows(table->table)))
>>> +      break;
>>> +  }
>>> +
>>> +  DBUG_RETURN(err);
>>> +}
>>> +
>>> +
>>
>> Hmm... MERGE doesn't support HA_CAN_RECREATE and doesn't implement
>> delete_all_rows() method. So it didn't support ordinary TRUNCATE
>> before your patch. Also it has HTON_NO_PARTITION set so this method
>> is not necessary for partitioning.
>
> This is one corner case where TRUNCATE resorted to row-by-row delete.
>

Oh my... Sorry I have missed this fact.

>> I don't think we should enable support of TRUNCATE for MERGE
>> in your patch. Otherwise we probably should do something about
>> resetting auto-increment and add test coverage for this new
>> functionality.
>
> Since the implementation is simple, I think it is worth implementing to  
> remain compatible. There is some test coverage already, that's why I  
> ended up implementing it. As for auto-increment, I think it does not  
> apply as the engine does not implement the auto-increment handler 
> methods.

Well, it looks like one can create MERGE table with auto-increment support,
but I see what you mean.

>
> OK. Most points are addressed, we can discuss the rest over IRC.
>

OK.

-- 
Dmitry Lenev, Software Developer
Oracle Development SPB/MySQL, www.mysql.com

Are you MySQL certified?  http://www.mysql.com/certification
Thread
bzr commit into mysql-5.5-runtime branch (davi:3149) Bug#49938 Bug#54678Davi Arnaut30 Sep
Re: bzr commit into mysql-5.5-runtime branch (davi:3149) Bug#49938Bug#54678Dmitry Lenev4 Oct
  • Re: bzr commit into mysql-5.5-runtime branch (davi:3149) Bug#49938Bug#54678Davi Arnaut4 Oct
    • Re: bzr commit into mysql-5.5-runtime branch (davi:3149) Bug#49938Bug#54678Dmitry Lenev5 Oct