maria-developers team mailing list archive

[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.

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
diff --git a/mysql-test/suite/innodb/r/instant_alter_charset,redundant.rdiff b/mysql-test/suite/innodb/r/instant_alter_charset,redundant.rdiff
index 935b5dbf78c..d7d2a32d742 100644
--- a/mysql-test/suite/innodb/r/instant_alter_charset,redundant.rdiff
+++ b/mysql-test/suite/innodb/r/instant_alter_charset,redundant.rdiff
@@ -1,10 +1,10 @@
---- instant_alter_charset.result
-+++ instant_alter_charset,redundant.result
-@@ -254,7 +254,6 @@
+--- instant_alter_charset.result	2019-04-23 17:42:23.324326518 +0400
++++ instant_alter_charset,redundant.result	2019-04-23 17:42:46.047531591 +0400
+@@ -279,7 +279,6 @@
  alter table boundary_255
- modify b varchar(200) charset utf8mb3,
+ modify a varchar(70) charset utf8mb4,
  algorithm=instant;
 -ERROR 0A000: ALGORITHM=INSTANT is not supported. Reason: Cannot change column type. Try ALGORITHM=COPY
- alter table boundary_255
- modify c varchar(300) charset utf8mb3,
- algorithm=instant;
+ drop table boundary_255;
+ create table fully_compatible (
+ id int auto_increment unique key,
diff --git a/mysql-test/suite/innodb/r/instant_alter_charset.result b/mysql-test/suite/innodb/r/instant_alter_charset.result
index 49908192458..1f5f61f9451 100644
--- a/mysql-test/suite/innodb/r/instant_alter_charset.result
+++ b/mysql-test/suite/innodb/r/instant_alter_charset.result
@@ -273,6 +273,14 @@ modify c varchar(300) charset utf8mb3,
 algorithm=instant;
 ERROR 0A000: ALGORITHM=INSTANT is not supported. Reason: Cannot change column type. Try ALGORITHM=COPY
 drop table boundary_255;
+create table boundary_255 (
+a varchar(70) charset utf8mb3
+) engine=innodb;
+alter table boundary_255
+modify a varchar(70) charset utf8mb4,
+algorithm=instant;
+ERROR 0A000: ALGORITHM=INSTANT is not supported. Reason: Cannot change column type. Try ALGORITHM=COPY
+drop table boundary_255;
 create table fully_compatible (
 id int auto_increment unique key,
 from_charset char(255),
diff --git a/mysql-test/suite/innodb/t/instant_alter_charset.test b/mysql-test/suite/innodb/t/instant_alter_charset.test
index 9335e4a5139..37801b9414d 100644
--- a/mysql-test/suite/innodb/t/instant_alter_charset.test
+++ b/mysql-test/suite/innodb/t/instant_alter_charset.test
@@ -292,23 +292,33 @@ alter table boundary_255
   modify a varchar(50) charset utf8mb3,
   algorithm=copy;
 
-if ($row_format == 'redundant') {
 --error ER_ALTER_OPERATION_NOT_SUPPORTED_REASON
 alter table boundary_255
   modify b varchar(200) charset utf8mb3,
   algorithm=instant;
-}
-if ($row_format != 'redundant') {
+
 --error ER_ALTER_OPERATION_NOT_SUPPORTED_REASON
 alter table boundary_255
-  modify b varchar(200) charset utf8mb3,
+  modify c varchar(300) charset utf8mb3,
   algorithm=instant;
-}
 
+drop table boundary_255;
+
+create table boundary_255 (
+  a varchar(70) charset utf8mb3
+) engine=innodb;
+
+if ($row_format == 'redundant') {
+alter table boundary_255
+  modify a varchar(70) charset utf8mb4,
+  algorithm=instant;
+}
+if ($row_format != 'redundant') {
 --error ER_ALTER_OPERATION_NOT_SUPPORTED_REASON
 alter table boundary_255
-  modify c varchar(300) charset utf8mb3,
+  modify a varchar(70) charset utf8mb4,
   algorithm=instant;
+}
 
 drop table boundary_255;