@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.) hq.recaptime.dev/wiki/Phorge
phorge phabricator

Guarantee the `key_position` key is created properly

Summary:
Ref T12987. I was focused on the RefCursor table and overlooked that we need some care on this key.

It's currently possible to run `bin/storage upgrade --no-adjust`, then start Phabricator, and end up with duplicate records in this table. If you try to run `bin/storage adjust` later, it will try to add the unique key but fail. This is unusual for normal installs (they usually do not use `--no-adjust`) but we do it in the cluster and I did this exact thing on `secure`.

Normally, to avoid this, when a new table with a unique key is introduced, we also add a migration to explicitly add that key.

This is mostly harmless in this case. Fix this mistake (force the table to contain only unique rows; add the key) and try using `LOCK TABLES` to make this atomic. If this doesn't cause problems we can use this in similar situations in the future.

The "alter table may unlock things" warning comes from here:

https://dev.mysql.com/doc/refman/5.7/en/lock-tables.html

It seems like it's fine to issue `UNLOCK TABLES` even if you don't have any locks, so I think this script should always do the right thing now, regardless of ALTER TABLE unlocking or not unlocking tables.

Test Plan: Ran `bin/storage upgrade -f`, saw table end up in the right state. I'll also check this on `secure`, where the starting state is a little messier.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12987

Differential Revision: https://secure.phabricator.com/D18623

+52
+52
resources/sql/autopatches/20170918.ref.01.position.php
··· 1 + <?php 2 + 3 + $table = new PhabricatorRepositoryRefPosition(); 4 + $conn = $table->establishConnection('w'); 5 + $key_name = 'key_position'; 6 + 7 + try { 8 + queryfx( 9 + $conn, 10 + 'ALTER TABLE %T DROP KEY %T', 11 + $table->getTableName(), 12 + $key_name); 13 + } catch (AphrontQueryException $ex) { 14 + // This key may or may not exist, depending on exactly when the install 15 + // ran previous migrations and adjustments. We're just dropping it if it 16 + // does exist. 17 + 18 + // We're doing this first (outside of the lock) because the MySQL 19 + // documentation says "if you ALTER TABLE a locked table, it may become 20 + // unlocked". 21 + } 22 + 23 + queryfx( 24 + $conn, 25 + 'LOCK TABLES %T WRITE', 26 + $table->getTableName()); 27 + 28 + $seen = array(); 29 + foreach (new LiskMigrationIterator($table) as $position) { 30 + $cursor_id = $position->getCursorID(); 31 + $hash = $position->getCommitIdentifier(); 32 + 33 + // If this is the first copy of this row we've seen, mark it as seen and 34 + // move on. 35 + if (empty($seen[$cursor_id][$hash])) { 36 + $seen[$cursor_id][$hash] = true; 37 + continue; 38 + } 39 + 40 + // Otherwise, get rid of this row as it duplicates a row we saw previously. 41 + $position->delete(); 42 + } 43 + 44 + queryfx( 45 + $conn, 46 + 'ALTER TABLE %T ADD UNIQUE KEY %T (cursorID, commitIdentifier)', 47 + $table->getTableName(), 48 + $key_name); 49 + 50 + queryfx( 51 + $conn, 52 + 'UNLOCK TABLES');