pt-online-schema-change needs to lowercase columns names when renaming

[Imri Zvik]

I have a table with a column like this:
  `Last_referenced` datetime NOT NULL,

I try to run an alter similar to this:
CHANGE COLUMN `Last_referenced` `test_column_rename` datetime NOT NULL

But Last_referenced and test_column_rename doesn't appear in the common columns list, and therefore not included in the INSERT statement of the batch copy, resulting data loss (in this specific case, it fails since the column has no default value).

The root cause is that the check in find_renamed_cols (https://github.com/percona/percona-toolkit/blob/3.0/bin/pt-online-schema-change#L10117) is not normalizing the case of the columns, and therefore misses the fact this is a shared column.

[Jericho Rivera (jericho-rivera)]

Please provide a test case that we can reproduce, like a simple table and the full pt-osc command you used to alter this table. We will also need the exact pt-osc version used and errors encountered when the tool was executed.

[Imri Zvik (imrizvik)]

I will, but it seems like *any* alter which involves uppercase column names in renaming columns ALTER will trigger it.

I tested it with a very old version (2.2.5), but looking at the source code, it seems the bug still exists.

[Imri Zvik (imrizvik)]

Test schema:

CREATE TABLE `mydb`.`t1` (
  `c1` int(10) unsigned NOT NULL,
  `c2` varchar(255) NOT NULL,
  `Last_referenced` datetime NOT NULL,
  `c3` int(10) unsigned NOT NULL,
  `c4` int(10) unsigned NOT NULL DEFAULT '0',
  `c5` varchar(255) NOT NULL DEFAULT '',
  `c6` varchar(255) NOT NULL DEFAULT '',
  `c7` varchar(255) NOT NULL DEFAULT '',
  `c8` varchar(255) DEFAULT '',
  `c9` varchar(255) DEFAULT '',
  `c10` int(10) NOT NULL DEFAULT '0',
  PRIMARY KEY (`c1`,`c2`),
  KEY `Last_Referenced_c6_Index` (`Last_referenced`,`c6`)
) ENGINE=InnoDB DEFAULT CHARSET=latin1

Test command:

pt-online-schema-change --alter 'CHANGE COLUMN `Last_referenced` `c11` datetime NOT NULL' --ask-pass --noswap-tables --nodrop-triggers --chunk-time 0.1 --nodrop-new-table --no-check-alter --execute D=mydb,t=t1,h=mydbhost,P=3306

[Jericho Rivera (jericho-rivera)]

I can't seem to repeat it with latest pt-osc 3.0

percona:~/sandboxes/msb_5_7_18$ ./pt-online-schema-change --alter 'change column `Last_referenced` `cl1` datetime not null' --ask-pass --no-swap-tables --no-drop-triggers --no-drop-new-table --no-check-alter --execute D=test,t=t1,h=127.0.0.1,u=root,P=5718,S=/tmp/mysql_sandbox5718.sock
Enter MySQL password:
No slaves found. See --recursion-method if host percona has slaves.
Not checking slave lag because no slaves were found and --check-slave-lag was not specified.
Operation, tries, wait:
  analyze_table, 10, 1
  copy_rows, 10, 0.25
  create_triggers, 10, 1
  drop_triggers, 10, 1
  swap_tables, 10, 1
  update_foreign_keys, 10, 1
Altering `test`.`t1`...
Renaming columns:
  Last_referenced to cl1
Creating new table...
Created new table test._t1_new OK.
Altering new table...
Altered `test`.`_t1_new` OK.
2017-08-03T12:13:47 Creating triggers...
2017-08-03T12:13:47 Created triggers OK.
2017-08-03T12:13:47 Copying approximately 1 rows...
2017-08-03T12:13:47 Copied rows OK.
Not dropping old table because --no-drop-triggers was specified.
Not dropping triggers because --no-drop-triggers was specified. To drop the triggers, execute:
DROP TRIGGER IF EXISTS `test`.`pt_osc_test_t1_del`;
DROP TRIGGER IF EXISTS `test`.`pt_osc_test_t1_upd`;
DROP TRIGGER IF EXISTS `test`.`pt_osc_test_t1_ins`;
Not dropping the new table `test`.`_t1_new` because --no-drop-new-table was specified. To drop the new table, execute:
DROP TABLE IF EXISTS `test`.`_t1_new`;
Successfully altered `test`.`t1`.

percona:~/sandboxes/msb_5_7_18$ ./use test -e "show create table t1\G"
*************************** 1. row ***************************
       Table: t1
Create Table: CREATE TABLE `t1` (
  `c1` int(10) unsigned NOT NULL,
  `c2` varchar(255) NOT NULL,
  `Last_referenced` datetime NOT NULL,
  `c3` int(10) unsigned NOT NULL,
  `c4` int(10) unsigned NOT NULL DEFAULT '0',
  `c5` varchar(255) NOT NULL DEFAULT '',
  `c6` varchar(255) NOT NULL DEFAULT '',
  `c7` varchar(255) NOT NULL DEFAULT '',
  `c8` varchar(255) DEFAULT '',
  `c9` varchar(255) DEFAULT '',
  `c10` int(10) NOT NULL DEFAULT '0',
  PRIMARY KEY (`c1`,`c2`),
  KEY `Last_Referenced_c6_Index` (`Last_referenced`,`c6`)
) ENGINE=InnoDB DEFAULT CHARSET=latin1

percona:~/sandboxes/msb_5_7_18$ ./use test -e "show tables"
+----------------+
| Tables_in_test |
+----------------+
| _t1_new |
| t1 |
+----------------+

percona:~/sandboxes/msb_5_7_18$ ./use test -e "show create table _t1_new\G"
*************************** 1. row ***************************
       Table: _t1_new
Create Table: CREATE TABLE `_t1_new` (
  `c1` int(10) unsigned NOT NULL,
  `c2` varchar(255) NOT NULL,
  `cl1` datetime NOT NULL,
  `c3` int(10) unsigned NOT NULL,
  `c4` int(10) unsigned NOT NULL DEFAULT '0',
  `c5` varchar(255) NOT NULL DEFAULT '',
  `c6` varchar(255) NOT NULL DEFAULT '',
  `c7` varchar(255) NOT NULL DEFAULT '',
  `c8` varchar(255) DEFAULT '',
  `c9` varchar(255) DEFAULT '',
  `c10` int(10) NOT NULL DEFAULT '0',
  PRIMARY KEY (`c1`,`c2`),
  KEY `Last_Referenced_c6_Index` (`cl1`,`c6`)
) ENGINE=InnoDB DEFAULT CHARSET=latin1

[Imri Zvik (imrizvik)]

The problem is not that the column is not created, but that it is not detected as *common* column, and therefore not included in the chunk copy SELECT.
Run with PTDEBUG=1 and look for the common columns line.

[Jericho Rivera (jericho-rivera)]

I got it now, and I can confirm the bug. If column is all lowercase the alter will not fail.

percona:~/sandboxes/msb_5_7_18$ ./pt-online-schema-change --alter 'change column `Last_referenced` `cl1` datetime not null' --ask-pass --no-swap-tables --no-drop-triggers --no-drop-new-table --no-check-alter --execute D=test,t=t1,h=127.0.0.1,u=root,P=5718,S=/tmp/mysql_sandbox5718.sock
Enter MySQL password:
No slaves found. See --recursion-method if host percona has slaves.
Not checking slave lag because no slaves were found and --check-slave-lag was not specified.
Operation, tries, wait:
  analyze_table, 10, 1
  copy_rows, 10, 0.25
  create_triggers, 10, 1
  drop_triggers, 10, 1
  swap_tables, 10, 1
  update_foreign_keys, 10, 1
Altering `test`.`t1`...
Renaming columns:
  Last_referenced to cl1
Creating new table...
Created new table test._t1_new OK.
Altering new table...
Altered `test`.`_t1_new` OK.
2017-08-03T14:49:09 Creating triggers...
2017-08-03T14:49:09 Created triggers OK.
2017-08-03T14:49:09 Copying approximately 100 rows...
Not dropping triggers because --no-drop-triggers was specified. To drop the triggers, execute:
DROP TRIGGER IF EXISTS `test`.`pt_osc_test_t1_del`;
DROP TRIGGER IF EXISTS `test`.`pt_osc_test_t1_upd`;
DROP TRIGGER IF EXISTS `test`.`pt_osc_test_t1_ins`;
Not dropping the new table `test`.`_t1_new` because --no-drop-new-table was specified. To drop the new table, execute:
DROP TABLE IF EXISTS `test`.`_t1_new`;
`test`.`t1` was not altered.
2017-08-03T14:49:09 Error copying rows from `test`.`t1` to `test`.`_t1_new`: 2017-08-03T14:49:09 Copying rows caused a MySQL error 1364:
    Level: Warning
     Code: 1364
  Message: Field 'cl1' doesn't have a default value
    Query: INSERT LOW_PRIORITY IGNORE INTO `test`.`_t1_new` (`c1`, `c2`, `c3`, `c4`, `c5`, `c6`, `c7`, `c8`, `c9`, `c10`) SELECT `c1`, `c2`, `c3`, `c4`, `c5`, `c6`, `c7`, `c8`, `c9`, `c10` FROM `test`.`t1` LOCK IN SHARE MODE /*pt-online-schema-change 6462 copy table*/

percona:~/sandboxes/msb_5_7_18$ ./use test -e "show create table t1\G"
*************************** 1. row ***************************
       Table: t1
Create Table: CREATE TABLE `t1` (
  `c1` int(10) unsigned NOT NULL,
  `c2` varchar(255) NOT NULL,
  `Last_referenced` datetime NOT NULL,
  `c3` int(10) unsigned NOT NULL,
  `c4` int(10) unsigned NOT NULL DEFAULT '0',
  `c5` varchar(255) NOT NULL DEFAULT '',
  `c6` varchar(255) NOT NULL DEFAULT '',
  `c7` varchar(255) NOT NULL DEFAULT '',
  `c8` varchar(255) DEFAULT '',
  `c9` varchar(255) DEFAULT '',
  `c10` int(10) NOT NULL DEFAULT '0',
  PRIMARY KEY (`c1`,`c2`),
  KEY `Last_Referenced_c6_Index` (`Last_referenced`,`c6`)
) ENGINE=InnoDB DEFAULT CHARSET=latin1

percona:~/sandboxes/msb_5_7_18$ ./use test -e "show create table _t1_new\G"
*************************** 1. row ***************************
       Table: _t1_new
Create Table: CREATE TABLE `_t1_new` (
  `c1` int(10) unsigned NOT NULL,
  `c2` varchar(255) NOT NULL,
  `cl1` datetime NOT NULL,
  `c3` int(10) unsigned NOT NULL,
  `c4` int(10) unsigned NOT NULL DEFAULT '0',
  `c5` varchar(255) NOT NULL DEFAULT '',
  `c6` varchar(255) NOT NULL DEFAULT '',
  `c7` varchar(255) NOT NULL DEFAULT '',
  `c8` varc...

[Imri Zvik (imrizvik)]

I think this is high priority, because this can lead to silent data corruption if the column has a default value.

[Imri Zvik (imrizvik)]

The fix is also very trivial (lower case both sides when comparing).

[Carlos Salguero (carlos-salguero)]

pt-online-schema change is already using lowercase.
You need to specify the column name in lowercase.

Try this:

bin/pt-online-schema-change --alter 'CHANGE COLUMN `last_referenced` `c11` datetime NOT NULL' --chunk-time 0.1 --no-check-alter --execute h=127.1,P=12345,u=msandbox,p=msandbox,D=test,t=t1

[Imri Zvik (imrizvik)]

Carlos, I beg to differ. Using the column name in upper case is valid, as
column names are case insensitive, and pt-online-schema-change should
adhere to this rule.
One could easily copy and paste from SHOW CREATE TABLE, which retains the
case, and suffer from silent data loss. Saying that column names should be
specified in lower case is both against mysql's syntax rules, and also not
documented in pt-online-schema-change documentation.

On Aug 3, 2017 5:16 PM, "Carlos Salguero" <email address hidden>
wrote:

> pt-online-schema change is already using lowercase.
> You need to specify the column name in lowercase.
>
> Try this:
>
> bin/pt-online-schema-change --alter 'CHANGE COLUMN `last_referenced` `c11`
> datetime NOT NULL' --chunk-time 0.1 --no-check-alter --execute
> h=127.1,P=12345,u=msandbox,p=msandbox,D=test,t=t1
>
> ** Changed in: percona-toolkit
> Status: Confirmed => Invalid
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1705998
>
> Title:
> pt-online-schema-change needs to lowercase columns names when renaming
>
> Status in Percona Toolkit:
> Invalid
>
> Bug description:
> I have a table with a column like this:
> `Last_referenced` datetime NOT NULL,
>
> I try to run an alter similar to this:
> CHANGE COLUMN `Last_referenced` `test_column_rename` datetime NOT NULL
>
> But Last_referenced and test_column_rename doesn't appear in the
> common columns list, and therefore not included in the INSERT
> statement of the batch copy, resulting data loss (in this specific
> case, it fails since the column has no default value).
>
> The root cause is that the check in find_renamed_cols
> (https://github.com/percona/percona-toolkit/blob/3.0/bin/pt-online-
> schema-change#L10117) is not normalizing the case of the columns, and
> therefore misses the fact this is a shared column.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/percona-toolkit/+bug/1705998/+subscriptions
>

[Imri Zvik (imrizvik)]

Thanks, Carlos.
I am still not sure the severity is accurate.
This bug could easily lead to silent data loss, while using perfectly legal
column names - this classifies as very high severity in my opinion.

On Aug 4, 2017 16:50, "Carlos Salguero" <email address hidden> wrote:

> ** Changed in: percona-toolkit
> Status: Invalid => Triaged
>
> ** Changed in: percona-toolkit
> Importance: Undecided => Medium
>
> ** Changed in: percona-toolkit
> Assignee: (unassigned) => Carlos Salguero (carlos-salguero)
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1705998
>
> Title:
> pt-online-schema-change needs to lowercase columns names when renaming
>
> Status in Percona Toolkit:
> Triaged
>
> Bug description:
> I have a table with a column like this:
> `Last_referenced` datetime NOT NULL,
>
> I try to run an alter similar to this:
> CHANGE COLUMN `Last_referenced` `test_column_rename` datetime NOT NULL
>
> But Last_referenced and test_column_rename doesn't appear in the
> common columns list, and therefore not included in the INSERT
> statement of the batch copy, resulting data loss (in this specific
> case, it fails since the column has no default value).
>
> The root cause is that the check in find_renamed_cols
> (https://github.com/percona/percona-toolkit/blob/3.0/bin/pt-online-
> schema-change#L10117) is not normalizing the case of the columns, and
> therefore misses the fact this is a shared column.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/percona-toolkit/+bug/1705998/+subscriptions
>

[Carlos Salguero (carlos-salguero)]

Fix committed: https://github.com/percona/percona-toolkit/pull/252

[Imri Zvik (imrizvik)]

Carlos, I don't want to nit pick, but the changelog is wrong -
The *only* reason pt-online-schema-change failed the ALTER, and did not end up with silent data corruption is that the column *happen* to not have a default value. If the column had default value, the tool would have ran and swap the tables, leading to data loss.

[Carlos Salguero (carlos-salguero)]

Hi,

Yes, but the reason for the possible data loss is that the check_alter sub is not able to detect the column rename due to the mixed case in the column name

[Imri Zvik (imrizvik)]

And the mixed case is perfectly legal and valid. So a perfectly legal and
valid usage can lead to silent data loss. This ranks as a severe bug in my
book.

Btw, technically it is not mixed case but any upper case letters will cause
this. No need to mix case.

On Aug 7, 2017 7:11 PM, "Carlos Salguero" <email address hidden>
wrote:

> Hi,
>
> Yes, but the reason for the possible data loss is that the check_alter
> sub is not able to detect the column rename due to the mixed case in the
> column name
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1705998
>
> Title:
> pt-online-schema-change needs to lowercase columns names when renaming
>
> Status in Percona Toolkit:
> Fix Committed
>
> Bug description:
> I have a table with a column like this:
> `Last_referenced` datetime NOT NULL,
>
> I try to run an alter similar to this:
> CHANGE COLUMN `Last_referenced` `test_column_rename` datetime NOT NULL
>
> But Last_referenced and test_column_rename doesn't appear in the
> common columns list, and therefore not included in the INSERT
> statement of the batch copy, resulting data loss (in this specific
> case, it fails since the column has no default value).
>
> The root cause is that the check in find_renamed_cols
> (https://github.com/percona/percona-toolkit/blob/3.0/bin/pt-online-
> schema-change#L10117) is not normalizing the case of the columns, and
> therefore misses the fact this is a shared column.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/percona-toolkit/+bug/1705998/+subscriptions
>

[Shahriyar Rzayev (rzayev-sehriyar)]

Percona now uses JIRA for bug reports so this bug report is migrated to: https://jira.percona.com/browse/PT-737