maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

Hi Marko,

Thanks for your review.

On 4/23/19 4:37 PM, Marko Mäkelä wrote:
> Bar, thank you for the revision.
>
> I ran all tests and found a regression for ROW_FORMAT=REDUNDANT, in
> the test innodb.instant_alter_charset,redundant. We should allow any
> extension of VARCHAR maximum length (in bytes) for
> ROW_FORMAT=REDUNDANT. The test attempts to enlarge the column from
> 50*3 to 200*3 bytes. That cannot work for other ROW_FORMAT in InnoDB,
> but it does for ROW_FORMAT=REDUNDANT.

I sent an update for this piece of tests (as suggested by Eugene)
in my previous email.


> Also, please check the spelling once more. I found "Uncode-1.1" in a comment.

Fixed. Thanks for noticing this.

> See also the Description of MDEV-18584. I’d want to allow a few more
> instantaneous conversion of CHAR columns in InnoDB.

Right, we could do this, in a separate commit.

> I agree that the (table->file->ha_table_flags() &
> HA_EXTENDED_TYPES_CONVERSION) is a bit ugly and could be replaced with
> something more descriptive.

I agree. Let's do it in a separate commit as well.

Thanks.


> Marko
>
> On Fri, Apr 19, 2019 at 2:24 PM Alexander Barkov <bar@xxxxxxxxxxx> wrote:
> > Hi Marko, Eugene,
> >
> > Thanks for good suggestions. A new version is attached.
> >
> >
> > See details inline:
> >
> > On 4/19/19 2:46 PM, Marko Mäkelä wrote:
> > > Hi Bar,
> > >
> > > I see that these bugs were based on wrong assumptions that ASCII or
> > > UCS2 columns would only contain valid ASCII or UTF-16 data. We should
> > > have tested these assumptions during the development of MDEV-15564.
> > > Luckily these were found before the GA release of MariaDB 10.4.
> >
> > That was my fault. When we had discussions on this topic with
> > Eugene, I overlooked these problems. Sorry for this.
> >
> > > Here is just a quick note before I will return to work on Tuesday.
> > >
> > > Please remove ,algorithm=copy from tests. Instead, use
> > > --enable_info
> > > alter table ...
> > > --disable_info
> > > and make sure that the table contains at least 1 row when the ALTER
> > > TABLE is executed. In that way, ALGORITHM=COPY will report that a
> > > nonzero amount of rows were affected, while the native ALTER would
> > > report that 0 rows were affected.
> >
> > I added this into the new tests in the end of instant_alter_charset.test
> >
> > Note, I didn't add this into original MDEV-15564 tests,
> > as they mostly use empty tables.
> >
> > > I would also suggest testing with some bad data (non-ASCII chars in
> > > ASCII column, or Unicode surrogate pairs in UCS2) to see what kind of
> > > errors would be triggered (if any), or what the converted data would
> > > look like after ALTER IGNORE TABLE.
> >
> > This is a very good idea. I've just added "ALTER IGNORE"s
> > into the new tests.
> >
> > > Please run the commit comment through a spell-checker. I spotted the
> > > typos "correspoding" and "actuallt".
> >
> > Done.
> >
> > > Also, please mention in the commit comment that these are regressions
> > > caused by MDEV-15564. That should make our lifes easier, should we
> > > ever decide to backport MDEV-15564 to earlier versions.
> >
> > Done.
> >
> > > I will review the code and tests in detail next week. I hope that
> > > Eugene can review it as well.
> > >
> > > Best regards,
> > >
> > > Marko