maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Nikita,

On May 05, Nikita Malyavin wrote:
> revision-id: 24c653be25a (mariadb-11.0.1-123-g24c653be25a)
> parent(s): c1cbda5a2c8
> author: Nikita Malyavin
> committer: Nikita Malyavin
> timestamp: 2023-05-04 20:49:43 +0300
> message:
> 
> unpack_row: unpack a correct number of fields
> 
> diff --git a/sql/rpl_record.cc b/sql/rpl_record.cc
> index 908d2c39d97..30bd80bdfc7 100644
> --- a/sql/rpl_record.cc
> +++ b/sql/rpl_record.cc
> @@ -228,24 +228,30 @@ int unpack_row(const rpl_group_info *rgi, TABLE *table, uint const colcnt,
>    const TABLE *conv_table= rpl_data.conv_table;
>    DBUG_PRINT("debug", ("Table data: tabldef: %p, conv_table: %p",
>                         tabledef, conv_table));
> -
> +  bool is_online_alter= rpl_data.is_online_alter();
>    DBUG_ASSERT(rgi);
>  
> -  for (field_ptr= begin_ptr; field_ptr < end_ptr && *field_ptr; ++field_ptr)
> +  for (field_ptr= begin_ptr; field_ptr < end_ptr
> +                             /* In Online Alter conv_table can be wider than
> +                               original table, but we need to unpack it all. */
> +                             && (*field_ptr || is_online_alter);

I failed to understand this, could you please explain? The comment didn't
help much, unfortunately. The test case didn't either.

> +       ++field_ptr)
>    {
> +    intptr fidx= field_ptr - begin_ptr;

intptr is a variant of int with sizeof(intptr) == sizeof(void*).
it's needed to cast a pointer to int, do some math, cast back.

here you need my_ptrdiff_t or ptrdiff_t

>      /*
>        If there is a conversion table, we pick up the field pointer to
>        the conversion table.  If the conversion table or the field
>        pointer is NULL, no conversions are necessary.
>       */
> -    Field *conv_field= conv_table ? conv_table->field[field_ptr - begin_ptr] : NULL;
> +    Field *conv_field= conv_table ? conv_table->field[fidx] : NULL;
>      Field *const f= conv_field ? conv_field : *field_ptr;
> +#ifdef DBUG_TRACE
> +    Field *dbg= is_online_alter ? f : *field_ptr;
> +#endif
>      DBUG_PRINT("debug", ("Conversion %srequired for field '%s' (#%ld)",
>                           conv_field ? "" : "not ",
> -                         (*field_ptr)->field_name.str,
> -                         (long) (field_ptr - begin_ptr)));
> +                         dbg->field_name.str,
> +                         (long) ()));

eh? you surely meant `(long) fidx` ?
how did it even compile?

>      DBUG_ASSERT(f != NULL);

Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx