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 <b...@mariadb.com> 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





_______________________________________________
Mailing list: https://launchpad.net/~maria-developers
Post to     : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Reply via email to