I believe ALTER TABLE and other schema modification statements are always sort of fundamentally non-transactional, so opening a transaction before you ALTER TABLE isn't meaningful. For example, if you ALTER TABLE in a transaction and then roll it back, your alter still sticks:

mysql> create table x (a int);
Query OK, 0 rows affected (0.01 sec)

mysql> begin;
Query OK, 0 rows affected (0.00 sec)

mysql> alter table x add b int;
Query OK, 0 rows affected (0.01 sec)
Records: 0  Duplicates: 0  Warnings: 0

mysql> rollback;
Query OK, 0 rows affected (0.00 sec)

mysql> show create table x;
+-------+------------------------------------------------------------------------------------------------------------------+
| Table | Create Table                                                                                                     |
+-------+------------------------------------------------------------------------------------------------------------------+
| x     | CREATE TABLE `x` (
  `a` int(11) DEFAULT NULL,
  `b` int(11) DEFAULT NULL
) ENGINE=InnoDB DEFAULT CHARSET=latin1 |
+-------+------------------------------------------------------------------------------------------------------------------+
1 row in set (0.00 sec)

So I think there's no way we can strictly guarantee that the data is still unique between editing it and adding the key, no matter when we execute it, with transactions.

However, maybe it's possible with LOCK TABLE instead of transactions? I'm not entirely sure. We don't currently use LOCK TABLE anywhere since we haven't run into this kind of problem in practice.

You're right that it's currently possible for an install to run the migration (20170915.ref.01.migrate.php) and then somehow insert duplicates (e.g., didn't stop all the daemons, and they race each other, and the install is clustered) before we make it to any of the ALTER TABLE statements.

Possibly we could do this instead:

• LOCK (or, if that does not work, RENAME) the table.
• Do the migration.
• Add the key in the same migration.
• UNLOCK (or RENAME back) the table.

I'm inclined not to bother with this since the race window is very small and we've never encountered an issue in practice, but maybe worth keeping an eye out.

From some poking, it looks like LOCK TABLE <table> WRITE; effectively stops all reads and writes of any kind from other threads and would likely prevent this problem if it does arise in practice.

Yeah, I wasn't particularly worried about the race itself, more that if someone did hit it, they'd be kind of screwed since re-running the migration that fails (this one) wouldn't fix the problem. They'd have to figure out how to manually re-run 20170915.ref.01.migrate.php. At least if the drop dupes/add constraint work happened in the same migration, a user could just attempt bin/storage upgrade again and just not get incredibly unlucky twice in a row.

Actually, I doubt it would make a difference since storage upgrade tracks php and sql migrations separately, so there really isn't a clean way to combine that work in a single migration. ¯\_(ツ)_/¯

It's okay to do a .sql migration in a PHP migration like this:

queryfx(
  $conn,
  'ALTER TABLE ...');

...just slightly harder to read and write most of the time, and, mostly, getting the right values for SQL variables like {$COLLATE_SORT} is involved. In this case, we don't need to be able to resolve {$COLLATE_SORT} or any of those tricky variables, though (and {$NAMESPACE} gets handled automatically when we establish the connection).