Re: [Maria-developers] Review for MDEV-19708 RBR replication loses data silently ignoring important column attributes

2 Sep 2019


      Hi Alexander,

On Fri, Aug 16, 2019 at 8:49 AM Alexander Barkov  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