Hi Andrei,
I've implemented all of your suggestions save one or two (see below). Here
is a link to my latest patch:
http://lists.mysql.com/commits/30817
I am running the entire test suite on both Linux and Windows now. If the
results are good, I will declare the patch ready for review.
Chuck
> From: Andrei Elkin [mailto:aelkin@stripped]
> > + virtual const uchar *unpack(uchar* to, const uchar
> *from, uint from_length)
> > + {
> > + uint length=pack_length();
> > + uint len= (from_length && (from_length < length)) ?
> > + from_length : length;
> > + memcpy(to,from,len);
> > + return from+len;
> > + }
>
> Shall we DBUG_ASSERT(from_lenght == 0 || from_length <= length)?
> Either unpack with the basic 2-args method or when from_length is
> informal it must not exceed the basic's pack_length. In other words
> it's good to have a guard against overrun.
Done. Good suggestion.
> > + for (unsigned int i= 0 ; i < m_table->s->fields ; i++)
> > + {
> > + switch (m_table->s->field[i]->real_type()) {
> > + case MYSQL_TYPE_NEWDECIMAL:
> > + {
> > + m_field_metadata[index]=
> > + (byte)((Field_new_decimal
> *)m_table->s->field[i])->precision;
>
> > + index++;
>
> Imo although
>
> m_field_metadata[index++]
>
> is less readable but is safer
> as no chances to forget incrementing of index.
I agree. Done.
> > Constructor used to build an event for writing to the binary log.
> > @@ -6385,7 +6497,7 @@ Table_map_log_event::Table_map_log_event
> > m_dblen(m_dbnam ? tbl->s->db.length : 0),
> > m_tblnam(tbl->s->table_name.str),
> > m_tbllen(tbl->s->table_name.length),
> > - m_colcnt(tbl->s->fields), m_coltype(0),
> > + m_colcnt(tbl->s->fields), m_field_metadata(0),
> > m_table_id(tid),
> > m_flags(flags)
> > {
> > @@ -6406,6 +6518,8 @@ Table_map_log_event::Table_map_log_event
> > m_data_size+= m_dblen + 2; // Include length and
> terminating \0
> > m_data_size+= m_tbllen + 2; // Include length and
> terminating \0
> > m_data_size+= 1 + m_colcnt; // COLCNT and column types
> > + m_field_metadata_size= get_field_metadata_size();
>
> > + m_data_size+= m_field_metadata_size + 1;
>
> Suggest to comment: +1 is due to net_store_length-ing of
> m_field_metadata_size, right?
> Then it might not be enough as merely the max number of
> columns is more than
> 256 (I met a link with "MySQL: Maximum number of columns in
> one table - 3398").
> To multiply the max on 2 or even 4 still keeps the needed
> size within 2 bytes.
Done. See patch.
>
>
> > -
>
> > uchar *m_memory;
> > ulong m_table_id;
> > flag_set m_flags;
>
> > -
>
> (and of course not to forget on consmetics: usually reviewers
> ask not to touch unnecessary lines ... Could you
> restore the blank lines above (and there are some places across the
> patch) and remove blanks added ?)
Right. I restored the lines.
>
> We spoke about `stop' that's for wl#3915 ie for me, thanks!
>
> <started askin before found the mail with a prototype code
> for wl3915>
> it's not changed anymore in the following. Why? Just for testing?
Yes, it was just for testing. I think it is code you can use.
>
> > DBUG_ENTER("unpack_row");
> > DBUG_ASSERT(row_data);
> > size_t const master_null_byte_count=
> (bitmap_bits_set(cols) + 7) / 8;
> > @@ -191,10 +193,11 @@ unpack_row(RELAY_LOG_INFO const *rli,
> > unsigned int null_mask= 1U;
> > // The "current" null bits
> > unsigned int null_bits= *null_ptr++;
> > - for (field_ptr= begin_ptr ; field_ptr < end_ptr ; ++field_ptr)
>
> > + int i= 0;
>
> <started askin>
> Another cosmetics wrt to i counter. I'd rather moved
> initialization in
> increment into for (i=0,...i++) as i is incremented per
> each loop and
> that would designate it's not needed outside of it.
>
Actually, "i" will be used. See my patch from yesterday that I sent you with
the comments for WL#3915. The variable is used to stop the for loop then
again in the loop that skips the columns on the master.
> > + inline table_def *get_tabledef(TABLE *tbl)
> > + {
> > + table_def *td= 0;
> > + for (TABLE_LIST *ptr= tables_to_lock; ptr && !td; ptr=
> ptr->next_global)
> > + if (ptr->table == tbl)
> > + td= &((RPL_TABLE_LIST *)ptr)->m_tabledef;
> > + return (td);
> > + }
> >
>
> Don't have a good solution for get_tabledef which is to be called per
> row.
> Computation expence grows with when number of tables. One day we
> would like to optimize that...
> One idea is to build a table array to fill with the list's
> entries. If the list order coincide with the order of Rows_log events'
> tables it'd be enough to compare the current array's index or
> - when all
> rows are done for the current table - to switch to the next.
>
Ok.
> Besides the code in the patch I was thinking on emulation of old ->
> new replication.
> There is a DBUG_EXECUTE_IF() examples left by Mats probably to emulate
> a similar scenario when table_map event got changed.
Right. I've tested the code replicating from an old master and vice-versa.
Things work fine.
Chuck
