maria-developers team mailing list archive

[Eugene Kosov <claprix@xxxxxxxxx>]

Hi.

23.04.2019, 16:49, "Alexander Barkov" <bar@xxxxxxxxxxx>:
>    Hi,
>
> It seems I forgot to record the rdiff file.
>
> On 4/23/19 4:55 PM, Eugene Kosov wrote:
>>  Hello, Marko, Alexander.
>>
>>  23.04.2019, 15:37, "Marko Mäkelä" <marko.makela@xxxxxxxxxxx>:
>>>  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 think it's mostly broken test rather a code regression.
>>
>>  Now there is only one case when field length can be increased due to charset change
>>  and it's utf8mb3 -> utf8mb4.
>>
>>  I think the only test case we need now is CHAR(70) UTF8MB3 -> CHAR(70) UTF8MB4
>>  It should work only fro ROW_FORMAT=REDUNDANT
>
> Please find an incremental patch fixing tests.
>
> It adds the block suggested by Eugene and updates the rdiff file
> accordingly.

I think you can remove an old boudary_255 table completely as it's redundant now.

>
> Thanks.
>
>>>  Also, please check the spelling once more. I found "Uncode-1.1" in a comment.
>>>
>>>  See also the Description of MDEV-18584. I’d want to allow a few more
>>>  instantaneous conversion of CHAR columns in InnoDB.
>>>  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.
>>>
>>>  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
>>>>    >
>>>
>>>  --
>>>  Marko Mäkelä, Lead Developer InnoDB
>>>  MariaDB Corporation

-- 
Eugene