List:Commits« Previous MessageNext Message »
From:Daogang Qu Date:November 10 2010 7:02am
Subject:Re: Please review bug#57666
View as plain text  
2010-11-09 18:58, Sven Sandberg wrote:
> Hi Daogang,
>
> Thanks for this work. It's great that we get rid of this special case, 
> it even reduces the size and complexity of the code. You also fixed an 
> additional memory leak. Very good!
>
> I have three suggestions:
>
> (S1) Now, the parameter is_multi_insert in upgrade_lock_type is 
> unused. Can you remove this too?
Right. Thanks!
>
> (S2) Also, it is now guaranteed that 
> thd.is_current_stmt_binlog_format_row() is true whenever we do insert 
> delayed. I would suggest to assert this, like:
>
>   DBUG_ASSERT(!log_query || thd.is_current_stmt_binlog_format_row());
To multi-values insert delayed STMT, the log_query will be assigned to
TRUE when executing the first row, will be assigned to FALSE when
executing other row. So I don't think the DBUG_ASSERT is suitable here.
See below too.
>
> (S3) We can also simplify Delayed_insert::handle_inserts further:
>
>
> === modified file 'sql/sql_insert.cc'
> --- sql/sql_insert.cc    2010-11-09 10:54:59 +0000
> +++ sql/sql_insert.cc    2010-11-09 10:55:35 +0000
> @@ -2936,8 +2936,6 @@ bool Delayed_insert::handle_inserts(void
>                             (ulong) row->query.length));
>      if (log_query)
>      {
> -      if (thd.is_current_stmt_binlog_format_row())
No. The insert delayed statement still will execute the function when
mysql_bin_log is disabled.
So it's better to update as following:
if (mysql_bin_log.is_open())
or
if (mysql_bin_log.is_open() && thd.is_current_stmt_binlog_format_row())

The second one is more careful.

Which one do you like?

> -      {
>          /* Flush rows of previous statement*/
>          if (thd.binlog_flush_pending_rows_event(TRUE, FALSE))
>          {
> @@ -2947,7 +2945,6 @@ bool Delayed_insert::handle_inserts(void
>          /* Set query for Rows_query_log event in RBR*/
>          thd.set_query(row->query.str, row->query.length);
>          thd.variables.binlog_rows_query_log_events= 
> row->binlog_rows_query_log_events;
> -      }
>
>        /*
>          This is the first value of an INSERT statement.
> @@ -3028,7 +3025,7 @@ bool Delayed_insert::handle_inserts(void
>      */
>      table->auto_increment_field_not_null= FALSE;
>
> -    if (log_query && thd.is_current_stmt_binlog_format_row())
> +    if (log_query)
The same with above case.
Updated as following:
if (log_query && mysql_bin_log.is_open())
or
if (log_query && mysql_bin_log.is_open() && 
thd.is_current_stmt_binlog_format_row())

Which one do you like?

>        thd.set_query(NULL, 0);
>      delete row;
>      /*
> @@ -3090,8 +3087,7 @@ bool Delayed_insert::handle_inserts(void
>    */
>    has_trans= thd.lex->sql_command == SQLCOM_CREATE_TABLE ||
>                table->file->has_transactions();
> -  if (thd.is_current_stmt_binlog_format_row() &&
> -      thd.binlog_flush_pending_rows_event(TRUE, has_trans))
> +  if (thd.binlog_flush_pending_rows_event(TRUE, has_trans))
Updated as following:
if ( mysql_bin_log.is_open() &&
     thd.binlog_flush_pending_rows_event(TRUE, has_trans))
or
if ( mysql_bin_log.is_open() &&
      thd.is_current_stmt_binlog_format_row() &&
     thd.binlog_flush_pending_rows_event(TRUE, has_trans))

Which one do you like?

Best Regards,

Daogang

>
>      goto err;
>
>    if ((error=table->file->extra(HA_EXTRA_NO_CACHE)))
>
>
> On 11/09/2010 01:44 AM, Daogang Qu wrote:
>> Hi Luis and Sven,
>> Please review the appended patch. Sorry for that.
>> My patch is still refused to append to commit list
>> by mx4.sun.com and mx3.sun.com. The problem
>> can't be fixed by Helpdesk so far.
>>
>> Best Regards,
>>
>> Daogang
>>
>>
>> === modified file 
>> 'mysql-test/extra/binlog_tests/binlog_insert_delayed.test'
>> --- mysql-test/extra/binlog_tests/binlog_insert_delayed.test    
>> 2010-08-30 06:03:28 +0000
>> +++ mysql-test/extra/binlog_tests/binlog_insert_delayed.test    
>> 2010-11-08 23:49:49 +0000
>> @@ -34,11 +34,11 @@
>>  let $table=t1;
>>  let $count=0;
>>
>> -insert delayed into t1 values (207);
>> +insert /* before delayed */ delayed /* after delayed */ into t1 
>> values (207);
>>  inc $count;
>>  --source include/wait_until_rows_count.inc
>>
>> -insert delayed into t1 values (null);
>> +insert /*! delayed */ into t1 values (null);
>>  inc $count;
>>  --source include/wait_until_rows_count.inc
>>
>>
>> === modified file 'mysql-test/suite/binlog/r/binlog_row_binlog.result'
>> --- mysql-test/suite/binlog/r/binlog_row_binlog.result    2010-10-03 
>> 23:42:39 +0000
>> +++ mysql-test/suite/binlog/r/binlog_row_binlog.result    2010-11-09 
>> 00:06:33 +0000
>> @@ -1240,8 +1240,8 @@
>>  master-bin.000001    #    Query    #    #    COMMIT
>>  drop table t1,t2,t3,tt1;
>>  create table t1 (a int not null auto_increment, primary key (a)) 
>> engine=myisam;
>> -insert delayed into t1 values (207);
>> -insert delayed into t1 values (null);
>> +insert /* before delayed */ delayed /* after delayed */ into t1 
>> values (207);
>> +insert /*! delayed */ into t1 values (null);
>>  insert delayed into t1 values (300);
>>  FLUSH TABLES;
>>  show binlog events from <binlog_start>;
>>
>> === modified file 
>> 'mysql-test/suite/binlog/r/binlog_statement_insert_delayed.result'
>> --- 
>> mysql-test/suite/binlog/r/binlog_statement_insert_delayed.result    
>> 2010-08-30 06:03:28 +0000
>> +++ 
>> mysql-test/suite/binlog/r/binlog_statement_insert_delayed.result    
>> 2010-11-09 00:03:41 +0000
>> @@ -1,6 +1,6 @@
>>  create table t1 (a int not null auto_increment, primary key (a)) 
>> engine=myisam;
>> -insert delayed into t1 values (207);
>> -insert delayed into t1 values (null);
>> +insert /* before delayed */ delayed /* after delayed */ into t1 
>> values (207);
>> +insert /*! delayed */ into t1 values (null);
>>  insert delayed into t1 values (300);
>>  FLUSH TABLES;
>>  show binlog events from <binlog_start>;
>> @@ -10,14 +10,14 @@
>>  master-bin.000001    #    Query    #    #    COMMIT
>>  master-bin.000001    #    Query    #    #    use `test`; create 
>> table t1 (a int not null auto_increment, primary key (a)) engine=myisam
>>  master-bin.000001    #    Query    #    #    BEGIN
>> -master-bin.000001    #    Query    #    #    use `test`; insert 
>> delayed into t1 values (207)
>> +master-bin.000001    #    Query    #    #    use `test`; insert /* 
>> before delayed */  /* after delayed */ into t1 values (207)
>>  master-bin.000001    #    Query    #    #    COMMIT
>>  master-bin.000001    #    Query    #    #    BEGIN
>>  master-bin.000001    #    Intvar    #    #    INSERT_ID=208
>> -master-bin.000001    #    Query    #    #    use `test`; insert 
>> delayed into t1 values (null)
>> +master-bin.000001    #    Query    #    #    use `test`; insert /*!  
>> */ into t1 values (null)
>>  master-bin.000001    #    Query    #    #    COMMIT
>>  master-bin.000001    #    Query    #    #    BEGIN
>> -master-bin.000001    #    Query    #    #    use `test`; insert 
>> delayed into t1 values (300)
>> +master-bin.000001    #    Query    #    #    use `test`; insert  
>> into t1 values (300)
>>  master-bin.000001    #    Query    #    #    COMMIT
>>  master-bin.000001    #    Query    #    #    use `test`; FLUSH TABLES
>>  RESET MASTER;
>>
>> === modified file 'mysql-test/suite/binlog/r/binlog_stm_binlog.result'
>> --- mysql-test/suite/binlog/r/binlog_stm_binlog.result    2010-09-01 
>> 02:51:08 +0000
>> +++ mysql-test/suite/binlog/r/binlog_stm_binlog.result    2010-11-09 
>> 00:12:52 +0000
>> @@ -717,8 +717,8 @@
>>  master-bin.000001    #    Query    #    #    COMMIT
>>  drop table t1,t2,t3,tt1;
>>  create table t1 (a int not null auto_increment, primary key (a)) 
>> engine=myisam;
>> -insert delayed into t1 values (207);
>> -insert delayed into t1 values (null);
>> +insert /* before delayed */ delayed /* after delayed */ into t1 
>> values (207);
>> +insert /*! delayed */ into t1 values (null);
>>  insert delayed into t1 values (300);
>>  FLUSH TABLES;
>>  show binlog events from <binlog_start>;
>>
>> === modified file 'mysql-test/suite/rpl/r/rpl_stm_insert_delayed.result'
>> --- mysql-test/suite/rpl/r/rpl_stm_insert_delayed.result    
>> 2010-06-19 09:24:34 +0000
>> +++ mysql-test/suite/rpl/r/rpl_stm_insert_delayed.result    
>> 2010-11-09 00:01:27 +0000
>> @@ -51,8 +51,8 @@
>>  INSERT DELAYED IGNORE INTO t1 VALUES(1);
>>  INSERT DELAYED IGNORE INTO t1 VALUES(1);
>>  flush table t1;
>> -use `test`; INSERT DELAYED IGNORE INTO t1 VALUES(1)
>> -use `test`; INSERT DELAYED IGNORE INTO t1 VALUES(1)
>> +use `test`; INSERT  IGNORE INTO t1 VALUES(1)
>> +use `test`; INSERT  IGNORE INTO t1 VALUES(1)
>>  select * from t1;
>>  a
>>  1
>> @@ -60,10 +60,10 @@
>>  show binlog events in 'slave-bin.000002' from <binlog_start> limit 1,6;
>>  Log_name    Pos    Event_type    Server_id    End_log_pos    Info
>>  slave-bin.000002    #    Query    #    #    BEGIN
>> -slave-bin.000002    #    Query    #    #    use `test`; INSERT 
>> DELAYED IGNORE INTO t1 VALUES(1)
>> +slave-bin.000002    #    Query    #    #    use `test`; INSERT  
>> IGNORE INTO t1 VALUES(1)
>>  slave-bin.000002    #    Query    #    #    COMMIT
>>  slave-bin.000002    #    Query    #    #    BEGIN
>> -slave-bin.000002    #    Query    #    #    use `test`; INSERT 
>> DELAYED IGNORE INTO t1 VALUES(1)
>> +slave-bin.000002    #    Query    #    #    use `test`; INSERT  
>> IGNORE INTO t1 VALUES(1)
>>  slave-bin.000002    #    Query    #    #    COMMIT
>>  select * from t1;
>>  a
>>
>> === modified file 'sql/sql_insert.cc'
>> --- sql/sql_insert.cc    2010-11-04 15:40:18 +0000
>> +++ sql/sql_insert.cc    2010-11-08 10:16:05 +0000
>> @@ -472,7 +472,7 @@
>>      bool log_on= (thd->variables.option_bits & OPTION_BIN_LOG ||
>>                    ! (thd->security_ctx->master_access & SUPER_ACL));
>>      if (global_system_variables.binlog_format == BINLOG_FORMAT_STMT &&
>> -        log_on && mysql_bin_log.is_open() && is_multi_insert)
>> +        log_on && mysql_bin_log.is_open())
>>      {
>>        /*
>>          Statement-based binary logging does not work in this case, 
>> because:
>> @@ -2940,7 +2940,10 @@
>>        {
>>          /* Flush rows of previous statement*/
>>          if (thd.binlog_flush_pending_rows_event(TRUE, FALSE))
>> +        {
>> +          delete row;
>>            goto err;
>> +        }
>>          /* Set query for Rows_query_log event in RBR*/
>>          thd.set_query(row->query.str, row->query.length);
>>          thd.variables.binlog_rows_query_log_events= 
>> row->binlog_rows_query_log_events;
>> @@ -3013,36 +3016,6 @@
>>        table->file->extra(HA_EXTRA_WRITE_CANNOT_REPLACE);
>>      }
>>
>> -    if (log_query && mysql_bin_log.is_open() &&
>> -        !thd.is_current_stmt_binlog_format_row())
>> -    {
>> -      bool backup_time_zone_used = thd.time_zone_used;
>> -      Time_zone *backup_time_zone = thd.variables.time_zone;
>> -      if (row->time_zone != NULL)
>> -      {
>> -        thd.time_zone_used = true;
>> -        thd.variables.time_zone = row->time_zone;
>> -      }
>> -
>> -      /* if the delayed insert was killed, the killed status is
>> -         ignored while binlogging */
>> -      int errcode= 0;
>> -      if (thd.killed == THD::NOT_KILLED)
>> -        errcode= query_error_code(&thd, TRUE);
>> -
>> -      /*
>> -        In SBR, only the query which has one single value
>> -        will be binlogged here.
>> -      */
>> -      if (thd.binlog_query(THD::STMT_QUERY_TYPE,
>> -                           row->query.str, row->query.length,
>> -                           FALSE, FALSE, FALSE, errcode))
>> -        goto err;
>> -
>> -      thd.time_zone_used = backup_time_zone_used;
>> -      thd.variables.time_zone = backup_time_zone;
>> -    }
>> -
>>      if (table->s->blob_fields)
>>        free_delayed_insert_blobs(table);
>>      thread_safe_decrement(delayed_rows_in_use,&LOCK_delayed_status);
>>
>
>

Thread
Please review bug#57666Daogang Qu9 Nov
  • Re: Please review bug#57666Sven Sandberg9 Nov
    • Re: Please review bug#57666Daogang Qu10 Nov