Hi,
I have tested your patch in another tree, where the problem also showed up.
It works, and is OK to push, with one minor change, see below.
/Mattias
ps. good commit comment!
Magne Mahre wrote:
> #At file:///export/home/tmp/z/mysql-6.0-runtime-fix37433/ based on
> revid:magne.mahre@stripped
>
> 2783 Magne Mahre 2009-01-13
> Bug #37433 Deadlock between open_table, close_open_tables,
> get_table_share, drop_open_table
>
> In the partition handler code, LOCK_open and share->LOCK_ha_data
> are acquired in the wrong order in certain cases. When doing a
> multi-row INSERT (i.e a INSERT..SELECT) in a table with auto-
> increment column(s). the increments must be in a monotonically
> continuous increasing sequence (i.e it can't have "holes"). To
> achieve this, a lock is held for the duration of the operation.
> share->LOCK_ha_data was used for this purpose.
>
> Whenever there was a need to open a view _during_ the operation
> (views are not currently pre-opened the way tables are), and
> LOCK_open was grabbed, a deadlock could occur. share->LOCK_ha_data
> is other places used _while_ holding LOCK_open.
>
> A new mutex was introduced in the HA_DATA_PARTITION structure,
> for exclusive use of the autoincrement data fields, so we don't
> need to overload the use of LOCK_ha_data here.
>
> A module test case has not been supplied, since the problem occurs
> as a result of a race condition, and testing for this condition
> is thus not deterministic. Testing for it could be done by
> setting up a test case as described in the bug report.
> modified:
> sql/ha_partition.cc
> sql/ha_partition.h
>
> === modified file 'sql/ha_partition.cc'
>
> === modified file 'sql/ha_partition.cc'
> --- a/sql/ha_partition.cc 2008-11-24 22:54:28 +0000
> +++ b/sql/ha_partition.cc 2009-01-13 11:50:40 +0000
> @@ -2528,6 +2528,7 @@
> }
> DBUG_PRINT("info", ("table_share->ha_data 0x%p", ha_data));
> bzero(ha_data, sizeof(HA_DATA_PARTITION));
> + pthread_mutex_init(&ha_data->mutex, MY_MUTEX_INIT_FAST);
> }
> if (is_not_tmp_table)
> pthread_mutex_unlock(&table_share->LOCK_ha_data);
>
> === modified file 'sql/ha_partition.h'
> --- a/sql/ha_partition.h 2008-11-15 00:34:27 +0000
> +++ b/sql/ha_partition.h 2009-01-13 11:50:40 +0000
> @@ -45,6 +45,7 @@
> {
> ulonglong next_auto_inc_val; /**< first non reserved value */
> bool auto_inc_initialized;
> + pthread_mutex_t mutex; /* hold while accessing fields */
> } HA_DATA_PARTITION;
>
> #define PARTITION_BYTES_IN_POS 2
> @@ -908,18 +909,21 @@
> virtual int reset_auto_increment(ulonglong value);
> virtual void lock_auto_increment()
> {
> + HA_DATA_PARTITION *ha_data;
> /* lock already taken */
> if (auto_increment_safe_stmt_log_lock)
> return;
> DBUG_ASSERT(table_share->ha_data && !auto_increment_lock);
> if(table_share->tmp_table == NO_TMP_TABLE)
> {
> + ha_data= (HA_DATA_PARTITION*) table_share->ha_data;
> auto_increment_lock= TRUE;
> - pthread_mutex_lock(&table_share->LOCK_ha_data);
> + pthread_mutex_lock(&ha_data->mutex);
> }
> }
> virtual void unlock_auto_increment()
> {
> + HA_DATA_PARTITION *ha_data;
MJ: move the above row down...
> DBUG_ASSERT(table_share->ha_data);
> /*
> If auto_increment_safe_stmt_log_lock is true, we have to keep the lock.
> @@ -928,7 +932,8 @@
> */
> if(auto_increment_lock && !auto_increment_safe_stmt_log_lock)
> {
> - pthread_mutex_unlock(&table_share->LOCK_ha_data);
MJ: ...here, and combine it with the row below:
HA_DATA_PARTITION *ha_data= (HA_DATA_PARTITION*) table_share->ha_data;
> + ha_data= (HA_DATA_PARTITION*) table_share->ha_data;
> + pthread_mutex_unlock(&ha_data->mutex);
> auto_increment_lock= FALSE;
> }
> }
>
>
