Hi Alexander, On Fri, Aug 16, 2019 at 8:49 AM Alexander Barkov <b...@mariadb.com> wrote: > > Hi Sachin, > > I've reviewed the code in bb-10.5-19708. > > It looks good. > > I suggest some cleanups. > > > - Remove Field::binlog_type_info_slave, it's not used. Actually this was used for reading binlog_type_info data at slave. I guess i will do this in seperate commit. > > - Remove Field::binlog_type_info_master. > Please don't cache Binlog_type_info inside the Field itself. > Let the caller code cache it in an array of Binlog_type_info pointers. Okay.
> > - Change this method in Field: > > virtual Binlog_type_info *binlog_type_info(); > > to > > virtual Binlog_type_info *binlog_type_info(MEM_ROOT *) const; > Done > Let's pass mem_root as a parameter instead of using table->mem_root. > Please add the 'const' qualifier! > > > - Restore the 'const' qualifier to > Field::enum_conv_type rpl_conv_type_from > > - Instead of doing this: > Done > Binlog_type_info * Field_new_decimal::binlog_type_info() > { > Field_num::binlog_type_info(); > this->binlog_type_info_master->m_metadata= precision + (decimals() << 8); > this->binlog_type_info_master->m_metadata_size= 2; > return this->binlog_type_info_master; > } > > > Please add a number of Binlog_type_info constructor helpers, > in addition to the current one. > > For example, for numeric types this would be good: > > Binlog_type_info(uchar type_code, > uint16 metadata, > uint8 metadata_size, > binlog_signess_t signess) > :m_type_code(type_code), > m_metadata(metadata), > m_metadata_size(metadata_size), > m_signess(signess), > m_cs(NULL), > m_enum_typelib(NULL), > m_set_typelib(NULL), > m_geom_type(GEOM_GEOMETRY) > { }; > > Done > so you can do something simple like this: > > Binlog_type_info Field_new_decimal::binlog_type_info(MEM_ROOT *root) const > { > return new (root) > Binlog_type_info(type(), > precision + (decimals() << 8), 2, > signess_arg); > } > > - It seems you always set signess to SIGNESS_NOT_RELEVANT. > Where is signess set to SIGNED or UNSIGNED? > > - This is something we've never used before: > > +#include <vector> > +#include <string> > +#include <functional> > +#include <memory> > > We need to discuss this with Serg. > Is it possible to avoid this? > I guess not, we need vector, I have not tested remaining > - There is a new test file > mysql-test/suite/rpl/t/rpl_mdev_19708.test.diff > but I could not find a corresponding .result.diff. > Where is it? Actually in this commit we are not doing slave side of 19708, So I will remove this test file. > > - Please use error names instead of numbers in tests: > > > Instead of: > > --error 1231 > SET GLOBAL binlog_row_metadata = -1; > > it should be: > > --error ER_WRONG_VALUE_FOR_VAR > SET GLOBAL binlog_row_metadata = -1; Done > > You can find error names in include/mysqld_ername.h > > > > Thanks. Code branch bb-10.5-19708 -- Regards Sachin Setiya Software Engineer at MariaDB _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp