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