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