Re: bzr commit into mysql-5.1 branch (hezx:2632) Bug#37051

[He Zhenxing]

Sven Sandberg wrote:
> Hi Zhenxing,
> 
> The patch is very good, I'm impressed. I found a couple of mistakes in 
> the patch and some speling errors, see comments inline.
> 

Thanks! And thank you for your careful and constructive review.

> /Sven
> 
> 
> He Zhenxing wrote:
> > #At file:///media/sda3/work/mysql/bzrwork/b37051/5.1-rpl-new/
> > 
> >  2632 He Zhenxing	2008-07-30
> >       BUG#37051 Replication rules not evaluated correctly
> >       
> >       The problem of this bug is that we need to get the list of tables
> >       to be updated for a multi-update statement, which requires to
> >       open all the tables referenced by the statement and resolve all
> >       the fields involved in update in order to figure out the list of
> >       tables for update. However if there are replicate filter rules,
> >       some tables might not exist on slave and result in a failure
> >       before we could examine the filter rules.
> >       
> >       I think the whole problem can not be solved on slave alone,
> >       the master must record and send the information of tables
> >       involved for update to slave, so that the slave do not need to
> >       open all the tables referencec by the multi-update statement to
> 
> referencec -> referenced
> 

OK

> >       figure out which tables are involved for udpate.
> 
> udpate -> update
> 

OK

> >       
> >       So a status variable is added to Query_log event to store the
> >       value of table map for update on master. And on slave, it will
> >       try to get the value of this variable and use it to examine
> >       filter rules without opening any tables on slav, if this values
> 
> slav -> slave
> 

OK

[snip]

> > === modified file 'sql/log_event.cc'
> > --- a/sql/log_event.cc	2008-05-30 09:12:07 +0000
> > +++ b/sql/log_event.cc	2008-07-30 09:50:27 +0000
> > @@ -1632,6 +1632,12 @@ bool Query_log_event::write(IO_CACHE* fi
> >      int2store(start, charset_database_number);
> >      start+= 2;
> >    }
> > +  if (table_map_for_update)
> > +  {
> > +    *start++= Q_TABLE_MAP_FOR_UPDATE;
> > +    int8store(start, table_map_for_update);
> > +    start+= 8;
> > +  }
> 
> The patch does not increase the size of the buffer where the table map 
> is stored, so there is a risk of buffer overflow here. See the 
> definition of the buffer in the beginning of this function.
> 

yes, miss that, good finding, thanks!

> Actually, there seems to be a few problems here even before your patch:
> 
>   - Query_log_event::write() should use MAX_LOG_EVENT_HEADER, instead of 
> computing the length of the buffer. That would avoid duplicate code.
> 
>   - More importantly, MAX_LOG_EVENT_HEADER is wrongly computed, as it 
> does not account for the type code. The formula used in 
> Query_log_event::write() is probably more correct.
> 


Agreed, I think by MAX_LOG_EVENT_HEADER, you mean
MAX_SIZE_LOG_EVENT_STATUS. I will correct the computation of
MAX_ZIE_LOG_EVENT_STATUS and use it to reserve the buffer.

>   - MAX_LOG_EVENT_HEADER and MAX_SIZE_LOG_EVENT_STATUS should probably 
> be renamed to something like MAX_QUERY_LOG_EVENT_HEADER_LENGTH, and 
> MAX_QUERY_LOG_EVENT_STATUS_VARIABLES_LENGTH, because they are specific 
> to Query_log_events.
> 

Agreed, but I'll keep it that way now.

> >    /*
> >      Here there could be code like
> >      if (command-line-option-which-says-"log_this_variable" && inited)
> > @@ -1709,7 +1715,8 @@ Query_log_event::Query_log_event(THD* th
> >     auto_increment_increment(thd_arg->variables.auto_increment_increment),
> >     auto_increment_offset(thd_arg->variables.auto_increment_offset),
> >     lc_time_names_number(thd_arg->variables.lc_time_names->number),
> > -   charset_database_number(0)
> > +   charset_database_number(0),
> > +   table_map_for_update((ulonglong)thd_arg->variables.table_map_for_update)
> >  {
> >    time_t end_time;
> >  
> > @@ -1874,7 +1881,8 @@ Query_log_event::Query_log_event(const c
> >     db(NullS), catalog_len(0), status_vars_len(0),
> >     flags2_inited(0), sql_mode_inited(0), charset_inited(0),
> >     auto_increment_increment(1), auto_increment_offset(1),
> > -   time_zone_len(0), lc_time_names_number(0), charset_database_number(0)
> > +   time_zone_len(0), lc_time_names_number(0), charset_database_number(0),
> > +   table_map_for_update(0)
> >  {
> >    ulong data_len;
> >    uint32 tmp;
> > @@ -2016,6 +2024,10 @@ Query_log_event::Query_log_event(const c
> >        charset_database_number= uint2korr(pos);
> >        pos+= 2;
> >        break;
> > +    case Q_TABLE_MAP_FOR_UPDATE:
> > +      CHECK_SPACE(pos, end, 8);
> > +      table_map_for_update= uint8korr(pos);
> > +      pos+= 8;
> 
> Missing 'break' here
> 

right, thanks!

> >      default:
> >        /* That's why you must write status vars in growing order of code */
> >        DBUG_PRINT("info",("Query_log_event has unknown status vars (first has\
> > @@ -2238,6 +2250,13 @@ void Query_log_event::print_query_header
> >                    print_event_info->delimiter);
> >      print_event_info->charset_database_number= charset_database_number;
> >    }
> > +  if (table_map_for_update)
> > +  {
> > +    char int_buf[22];
> > +    longlong10_to_str(uint8korr(&table_map_for_update), int_buf, 10);
> > +    my_b_printf(file, "SET @@session.table_map_for_update=%s%s\n",
> > +                int_buf, print_event_info->delimiter);
> > +  }
> 
> This must be a mistake? There is no @@session.table_map_for_update 
> variable. This method is invoked only from mysqlbinlog, which is not 
> subject to --replicate-* options. So I think the above 7 lines can just 
> be removed.
> 

No, this is intended, this should also work when user dump binlog by
using mysqlbinlog and feed to mysql client program. So I think I need to
define this system variable.

> >  }
> >  
> >  
> > @@ -2425,6 +2444,8 @@ int Query_log_event::do_apply_event(Rela
> >        else
> >          thd->variables.collation_database= thd->db_charset;
> >        
> > +      thd->variables.table_map_for_update= (table_map)table_map_for_update;
> > +      
> >        /* Execute the query (note that we bypass dispatch_command()) */
> >        const char* found_semicolon= NULL;
> >        mysql_parse(thd, thd->query, thd->query_length,
> &found_semicolon);
> > 
> > === modified file 'sql/log_event.h'
> > --- a/sql/log_event.h	2008-03-28 13:52:33 +0000
> > +++ b/sql/log_event.h	2008-07-30 09:50:27 +0000
> > @@ -242,6 +242,7 @@ struct sql_ex_info
> >                                     1 + 1 + 255 /* catalog */ + \
> >                                     4 /* autoinc */ + \
> >                                     6 /* charset */ + \
> > +                                   8 /* table_map_for_update */ + \
> >                                     MAX_TIME_ZONE_NAME_LENGTH)
> 
> As mentioned above, this computation is wrong (before your patch) 
> because it does not account for the one-byte type codes.
> 

yes, will fix this

> >  #define MAX_LOG_EVENT_HEADER   ( /* in order of Query_log_event::write */ \
> >    LOG_EVENT_HEADER_LEN + /* write_header */ \
> > @@ -306,6 +307,8 @@ struct sql_ex_info
> >  #define Q_LC_TIME_NAMES_CODE    7
> >  
> >  #define Q_CHARSET_DATABASE_CODE 8
> > +
> > +#define Q_TABLE_MAP_FOR_UPDATE 9
> >  /* Intvar event post-header */
> >  
> >  #define I_TYPE_OFFSET        0
> > @@ -1455,6 +1458,22 @@ protected:
> >      This field is written if it is not 0.
> >      </td>
> >    </tr>
> > +  <tr>
> > +    <td>table_map_for_update</td>
> > +    <td>Q_TABLE_MAP_FOR_UPDATE == 9</td>
> > +    <td>8 byte integer</td>
> > +
> > +    <td>The value of the table map that is to be updated by the
> > +    multi-update query statement. Every bit of this variable
> > +    represents a table, and is set to 1 if the corresponding table is
> > +    to be updated by this statement.
> > +
> > +    The value of this variable is set when executing a multi-update
> 
> I think the terminology is multi-table update.

Agree

> 
> > +    statement and used by slave to apply filter rules without opening
> > +    all the tables on slave. This is requires because some tables may
> 
> requires -> required
> 

OK

> > +    not exist on slave because of the filter rules.
> > +    </td>
> > +  </tr>
> >    </table>
> >  
> >    @subsection Query_log_event_notes_on_previous_versions Notes on Previous
> Versions
> > @@ -1548,6 +1567,11 @@ public:
> >    const char *time_zone_str;
> >    uint lc_time_names_number; /* 0 means en_US */
> >    uint charset_database_number;
> > +  /*
> > +    map for tables that will be updated for a multi-update query
> 
> multi-table update
> 

OK

> > +    statement, for other query statements, this will be zero.
> > +  */
> > +  ulonglong table_map_for_update;

[snip]