![](https://secure.gravatar.com/avatar/bf3ce935cdc1cff7cb213acd71f21f73.jpg?s=120&d=mm&r=g)
Hi Alexander,
On Fri, Aug 16, 2019 at 8:49 AM Alexander Barkov
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