maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

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.

- 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.

- Change this method in Field:

virtual Binlog_type_info *binlog_type_info();

to

virtual Binlog_type_info *binlog_type_info(MEM_ROOT *) const;

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:

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)
   { };


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?

- 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?

- 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;

You can find error names in include/mysqld_ername.h



Thanks.