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