Hi Mats,
Thanks for doing the review. I have some comments below. Things I do not
comment on were implemented as described.
Here's the new patch: http://lists.mysql.com/commits/32225
> > +/**
> > + Compare slave field size to that of the master. Error if
> slave < master.
>
> What does it mean that slave < master? Suggest to use a more
> elaborate wording.
Wording corrected.
> > + uint slave_value= 0;
> > + uint master_value= master_size;
>
> Call it master_metadata and slave_metadata?
done
>
> There are quite a few parameter passed, and they have a set
> of constraints on themselves that you need to check. The code
> assumes that:
>
> - That table is non-null
> - That type == table->field[col]->type()
> - That table->s == tsh
> - That table->field[col] is non-null and references valid memory
>
> It is usually prudent to check the constraints on the input
> parameters with assertions, since users of the function
> easily can make mistakes that introduce subtle errors.
>
> An alternative is to remove the type parameter and the col
> parameter, and instead pass the table (since you need it to
> get access to the table share) and the field (since you need
> to access it for the field data), and compute everything else
> inside the function. That follows the maxim that "the
> precondition of a function should be as weak as possible, but
> not weaker".
done
> Also, when writing functions to be as efficient as possible:
> try to pass frequently used parameters before less frequently
> used parameters since that usually puts the frequently used
> parameters in registers, and also in correct registers for
> further processing when calling functions internally.
>
Ok. Some refactoring done.
> > + int error= 0;
> > +
> > + /*
> > + Return 0 if there is no metadata from the master.
> > + */
> > + if (!master_size)
> > + return error;
> > +
> > + switch (type) {
>
> Although this is correct it's starting to become quite a few
> switches for checking the field types.
Agreed. I have compared them side-by-side, but I have some problems
combining them. See below.
>
> If we had a virtual Field::field_metadata() function, we
> could do something like this:
>
> if (master_value < slave_field->field_metadata())
> {
> ...
> }
>
> You could move the code that computes the metadata into
> virtual functions (as we have discussed previously) but since
> you introduced code in
> WL#3228/WL#3915 to transform the fields of a table to field
> metadata and added it to Table_map_log_event, I suggest that
> you use the following approach to avoid code duplication:
Right. While I agree in principle, it would be a lot of work to do this
because each of the areas where I use the switch for type does something a
little different. Further, the field classes combine many of the field types
but I need them separated. Notice the distinction of type vs. real_type. In
several cases I have to store the real_type in the field metadata from the
master because it is lost when the fields classes are generated. While I
would like to find a way to combine these functions, I don't think it would
be easy to do and could end up being a bit tricky. I will look at this
again, however but for now I've left the switch in the method but have made
the other improvements.
>
> Table_map_log_event tmap(thd, table, /* dummy */ 0, /*
> dummy */ 0);
> tmap.save_field_metadata();
> .
> .
> .
> if (master_value < tmap.get_field_metadata(col))
> {
> ...
> }
>
> That will remove the switch entirely, and will in addition
> avoid code duplication, which is always a maintenance risk.
> It appears that the
> get_field_metadata() function is mentioned in comments, but
> does not exist, but it is easy to add that function to
> Table_map_log_event.
I cannot use this technique because the table map does not calculate field
metadata for all fields. It only does those fields that use additional
metadata. The get_field_metadata comments were corrected. It should have
read "save_field_metadata."
>
> An alternative is, of course, to factor out the code in
> save_field_metadata() into virtual functions in the Field
> hierarchy, but in that case, make sure to have a non-virtual
> public function that calls a (pure) virtual private function,
> since it might be interesting to introduce caching to speed
> up use of this piece of data.
>
> > + case MYSQL_TYPE_NEWDECIMAL:
> > + {
> > + uint master_precision= (master_size >> 8U) & 0x00ff;
> > + uint master_decimal= master_size & 0x00ff;
>
> Use const qualifier on these since you do not intend to
> change them. It's a good habit. It doesn't make the code more
> efficient, but it makes it easier to avoid programming mistakes.
done
>
>
> > + slave_value= table->field[col]->pack_length();
> > + master_value= my_decimal_get_binary_size(master_precision,
> > master_decimal); + break;
> > + }
> > + case MYSQL_TYPE_DOUBLE:
> > + case MYSQL_TYPE_FLOAT:
> > + slave_value= table->field[col]->pack_length();
> > + break;
> > + case MYSQL_TYPE_BIT:
> > + {
> > + slave_value= ((Field_bit *)table->field[col])->bytes_in_rec +
> > + ((((Field_bit
> *)table->field[col])->bit_len > 0) ? 1 :
> > 0); + uint from_len= (master_size >> 8U) & 0x00ff;
> > + uint from_bit_len= master_size & 0x00ff;
> > + master_value= from_len + ((from_bit_len > 0) ? 1 : 0);
> > + break;
> > + }
> > + case MYSQL_TYPE_SET:
> > + case MYSQL_TYPE_STRING:
> > + case MYSQL_TYPE_ENUM:
> > + {
> > + if (((master_size & 0xff00) == (MYSQL_TYPE_SET << 8)) ||
> > + ((master_size & 0xff00) == (MYSQL_TYPE_ENUM << 8)))
> > + master_value= master_size & 0x00ff;
> > + else
> > + master_value= master_size & 0x00ff;
> > + if ((table->field[col]->real_type() == MYSQL_TYPE_SET) ||
> > + (table->field[col]->real_type() == MYSQL_TYPE_ENUM))
> > + slave_value= table->field[col]->pack_length();
> > + else
> > + slave_value= table->field[col]->field_length + 1;
> > + break;
> > + }
> > + case MYSQL_TYPE_VARCHAR:
> > + slave_value= table->field[col]->field_length;
> > + break;
> > + case MYSQL_TYPE_TINY_BLOB:
> > + case MYSQL_TYPE_MEDIUM_BLOB:
> > + case MYSQL_TYPE_LONG_BLOB:
> > + case MYSQL_TYPE_BLOB:
> > + slave_value= ((Field_blob
> *)table->field[col])->pack_length_no_ptr();
> > + break;
> > + default:
> > + slave_value= master_value;
> > + }
> > + if (slave_value < master_value)
> > + {
> > + error= 1;
> > + char buf[256];
> > + my_snprintf(buf, sizeof(buf), "Column %d size mismatch - "
> > + "master has size %d, %s.%s on slave has size %d",
>
> Also give information on what the constraint is, i.e., that
> the slave's width has to be greater or equal to the master's width.
>
done
> > + col, master_value, tsh->db.str,
> > + tsh->table_name.str, slave_value);
> > + rli->report(ERROR_LEVEL, ER_BINLOG_ROW_WRONG_TABLE_DEF,
> > + ER(ER_BINLOG_ROW_WRONG_TABLE_DEF), buf);
> > + }
> > + return (error);
> > +}
> > +
> > /*
> > Is the definition compatible with a table?
> >
> > @@ -156,6 +252,12 @@ table_def::compatible_with(RELAY_LOG_INF
> > rli->report(ERROR_LEVEL, ER_BINLOG_ROW_WRONG_TABLE_DEF,
> > ER(ER_BINLOG_ROW_WRONG_TABLE_DEF), buf);
> > }
> > + /*
> > + Check the slave's field size against that of the master.
> > + */
> > + if (!error)
> > + error=
> check_slave_field_size(table->field[col]->type(), col, table,
> > + m_field_metadata[col],
> tsh, rli);
> > }
> >
> > return error;
>
>
