maria-developers team mailing list archive

[Nikita Malyavin <nikitamalyavin@xxxxxxxxx>]

Hello, Sergei,

On Fri, 5 May 2023 at 23:28, Sergei Golubchik <serg@xxxxxxxxxxx> wrote:

> > -  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.
>
The new table in online alter can have fewer columns than the old one, but
we still have to go
through all the old table's columns.

For the replication case, the slave table can be narrower than the
master one.
See rpl_extra_col_master.test. I found it by adding a corresponding
assertion.



> > +       ++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
>

While refactoring, I found that `i` is what I needed. So, removing  fidx
altogether, in favor of `i`.

>
> >      /*
> >        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?
>
Oops. Will also be `i` with %u format.


The corrections can be found in c3879da39
<https://github.com/MariaDB/server/commit/c3879da39fa39204930e46750aba7387de4d3a69>


-- 
Yours truly,
Nikita Malyavin