maria-developers team mailing list archive

[Sachin Setiya <sachin.setiya@xxxxxxxxxxx>]

Hi Alexander,

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