List:Commits« Previous MessageNext Message »
From:Guilhem Bichot Date:April 10 2008 1:59pm
Subject:Re: bk commit into 5.1 tree (mattiasj:1.2518) BUG#33479
View as plain text  
Hi,


On Tue, Apr 01, 2008 at 11:38:56PM +0200, Mattias Jonsson wrote:
> Hi,
> 
> In the bottom I have a proposal for handling the multi row insert 
> statement with statement replication problem.
> 
> It will not release the auto_increment_lock taken at the first 
> get_auto_increment until release_auto_increment is called.
> I have read through the code in sql_insert.cc, sql_load.cc and 
> sql_table.cc and as I read it, ha_release_auto_increment is always 
> called after an insert select, insert replace or load data. (can create 
> select be a problem here?)
> 
> It is all in the new patch I committed.
> 
> Regards
> Mattias
> 
> Guilhem Bichot wrote:
> >On Thu, Mar 27, 2008 at 05:16:37PM +0100, Mattias Jonsson wrote:
> >>Guilhem Bichot wrote:
> >>>On Fri, Mar 14, 2008 at 01:54:08AM +0100, Mattias Jonsson wrote:
> >>>>Guilhem Bichot wrote:
> >>>>>On Wed, Mar 05, 2008 at 11:56:16PM +0100, mattiasj@stripped
> wrote:
> >>>>>>ChangeSet@stripped, 2008-03-05 23:56:06+01:00, mattiasj@witty.
> +3 -0
> >>>>>>Bug#33479: auto_increment failures in partitioning
> >>>>>>
> >>>>>>Several problems with auto_increment in partitioning
> >>>>>>(with MyISAM, InnoDB and Falcon)
> >>>>>>
> >>>>>>Updated code only patch (the test cases will be in a separate
> patches
> >>>>>>5.1+ (InnoDB+MyISAM) and 6.0 (Falcon)
> >>>>>>diff -Nrup a/sql/ha_partition.cc b/sql/ha_partition.cc
> 

> >We agreed on IRC to evaluate a solution where get_auto_increment() is
> >forwarded to all partitions, and helped by holding the mutex. Could
> >you please sketch it? I am not absolutely sure that it would work,
> >especially if while we are computing the max of reserved intervals, an
> >INSERT VALUES(some_fixed_value) comes, where some_fixed_value == the
> >max.
> >
> 
> (for insert values (),(),()... it always tries to reserve as many values 
> as given).
> 
> Instead of forwarding the get_auto_increment, I changed the patch to not 
> release the mutex until release_auto_increment is called if there is a 
> multi row insert statement and statement based replication.
> 
> This proposed change will work with all statement based replication 
> since it is prohibiting concurrent multi row insert statements, by 
> serializing them using a mutex.
> 
> I added this in get_auto_increment (where auto_increment_lock already 
> was called):
> 
>     /*
>       in a multi row insert statement like insert ... select, load data
>       we must hold a lock/mutex for the whole statement if we have 
> statement
>       based replication.
>     */
>     if (!auto_increment_multi_row_stmt_lock &&
>         thd->variables.binlog_format == BINLOG_FORMAT_STMT &&
>         (thd->lex->sql_command == SQLCOM_REPLACE_SELECT ||
>          thd->lex->sql_command == SQLCOM_INSERT_SELECT ||
>          thd->lex->sql_command == SQLCOM_LOAD))
>     {
>       DBUG_PRINT("info", ("locking auto_increment_multi_row_stmt_lock"));
>       auto_increment_multi_row_stmt_lock= TRUE;
>     }
> 
> 
> Would it also need thd->variables.binlog_format ==
> BINLOG_FORMAT_MIXED, 

Yes, in fact it would be better to check
mysql_bin_log.is_open() &&
(thd->options & OPTION_BIN_LOG) &&
!thd->current_stmt_binlog_row_based

which checks that:
- binlog is globally enabled
- connection has not disabled it for itself
- it is not row-based
which all three together mean that connection is doing statement-based
binlogging for this statement.
If mixed mode switches to row-based then
thd->current_stmt_binlog_row_based is true.
If binlog is globally disabled, thd->variables.binlog_format is
irrelevant.

> or does that always do row based with auto_increment inserts? (or maybe 
> thd->lex->sql_command == SQLCOM_CREATE for create .. select?)

CREATE SELECT holds an exclusive lock on the created table until
SELECT ends.
That is, the table does not exist for other connections until SELECT
ends, so there is no concurrency issue.

> also added this in release_auto_increment:
>       /* Unlock the multi row statement lock taken in get_auto_increment */
>       if (auto_increment_multi_row_stmt_lock)
>       {
>         auto_increment_multi_row_stmt_lock= FALSE;
>         DBUG_PRINT("info", ("unlocking 
> auto_increment_multi_row_stmt_lock"));
>       }
> 
>       unlock_auto_increment();
> 
> and changed so that lock_auto_increment returns directly if it is 
> holding the multi_row lock:
>   virtual void lock_auto_increment()
>   {
>     if (auto_increment_multi_row_stmt_lock)
>       return;
>     DBUG_ASSERT(table_share->ha_data && !auto_increment_lock);
>     if(table_share->tmp_table == NO_TMP_TABLE)
>     {
>       auto_increment_lock= TRUE;
>       pthread_mutex_lock(&table_share->mutex);
>     }
>   }
> 
> and will only release it if it does not have the multi_row lock:
>   virtual void unlock_auto_increment()
>   {
>     DBUG_ASSERT(table_share->ha_data);
>     if(auto_increment_lock && !auto_increment_multi_row_stmt_lock)
>     {
>       pthread_mutex_unlock(&table_share->mutex);
>       auto_increment_lock= FALSE;
>     }
>   }

I will review the new patch.

-- 
   __  ___     ___ ____  __
  /  |/  /_ __/ __/ __ \/ /    Mr. Guilhem Bichot <guilhem@stripped>
 / /|_/ / // /\ \/ /_/ / /__   Sun Microsystems, Lead Software Engineer
/_/  /_/\_, /___/\___\_\___/   Bordeaux, France
       <___/   www.mysql.com   
Thread
bk commit into 5.1 tree (mattiasj:1.2518) BUG#33479mattiasj5 Mar
  • Re: bk commit into 5.1 tree (mattiasj:1.2518) BUG#33479Sergei Golubchik7 Mar
    • Re: bk commit into 5.1 tree (mattiasj:1.2518) BUG#33479Sergei Golubchik7 Mar
      • Re: bk commit into 5.1 tree (mattiasj:1.2518) BUG#33479Mattias Jonsson8 Mar
        • Re: bk commit into 5.1 tree (mattiasj:1.2518) BUG#33479Sergei Golubchik10 Mar
  • Re: bk commit into 5.1 tree (mattiasj:1.2518) BUG#33479Guilhem Bichot13 Mar
    • Re: bk commit into 5.1 tree (mattiasj:1.2518) BUG#33479Mattias Jonsson14 Mar
      • Re: bk commit into 5.1 tree (mattiasj:1.2518) BUG#33479Guilhem Bichot18 Mar
        • Re: bk commit into 5.1 tree (mattiasj:1.2518) BUG#33479Mattias Jonsson27 Mar
          • Re: bk commit into 5.1 tree (mattiasj:1.2518) BUG#33479Guilhem Bichot28 Mar
            • Re: bk commit into 5.1 tree (mattiasj:1.2518) BUG#33479Mattias Jonsson1 Apr
              • Re: bk commit into 5.1 tree (mattiasj:1.2518) BUG#33479Guilhem Bichot10 Apr