[16:16:15] TimStarling: Bleugh. So as we can't use the RENAME INDEX MySQL 5.7 feature... I dropped the tables.sql change from mine [16:16:16] https://gerrit.wikimedia.org/r/#/c/345534/3 [16:16:32] But as indexExists/indexInfo calls indexName... It gets remapped [16:20:30] Custom updater function? Have to copy some code from the mysql db classes... But not call indexInfo [16:23:33] If we don't drop the old index, we risk people having two the same with different names, wasting space and cpu time [16:24:19] If we add the drop in the add patch.. And it doesn't exist, it'll fail for both [16:32:00] * Reedy ponders for a bit [16:32:19] Certainly, RENAME INDEX, if we could use it, is a *much* cleaner solution [20:00:02] DanielK_WMDE_: I'll be 3-5min late. Just getting back now. [20:00:12] sorry, should have checked that [20:01:30] I'll figure this out after my meeting [20:02:16] Thanks [20:02:46] As long as you're meeting isn't gonna last 5 or 6 hours... :P I'm gonna be about for a while, and hopefully avoid a 3am bedtime for the nth time recently [20:19:06] Reedy for https://gerrit.wikimedia.org/r/#/c/345534/3 i though the dropIndex function in mediawiki would check if the index exist [20:19:12] if it didnt shoulden't it not execute then? [20:20:05] it does [20:20:13] But we remap it for query purposes [20:20:36] So it's not checking for the one that is literally named in the updater entry [20:21:18] oh [20:21:43] Reedy we should remove the remap in your change [20:21:50] Since it wont be needed [20:22:45] Technically, with sqlite ignoring USE INDEX, no it's not [20:23:45] oh [20:24:02] Reedy with sqlite we drop the table and then readd the table [20:24:18] Not quite [20:24:21] though with the remap gone the dropIndex should work [20:24:31] In sqlite, we add a new table, we copy all the rows, we drop the old table [20:24:39] We're not doing that in mysql [20:24:44] oh [20:24:46] yep [20:25:06] i was meaning for sqlite. mysql we can drop index [20:25:19] We don't care about sqlite [20:25:22] it's not broken here [20:25:37] Ok [20:25:50] Reedy what about doing this http://stackoverflow.com/questions/2480148/how-can-i-employ-if-exists-for-creating-or-dropping-an-index-in-mysql [20:25:55] example SHOW INDEX FROM table_name WHERE KEY_NAME = 'index_name' [20:26:24] mw already can tell you whether an index exists [20:26:49] but we are using the remap right. So mw will be told a lie. [20:27:03] Well, no. MW asks the wrong question [20:27:10] oh [20:28:00] Removing the mapping might not be a bad idea... But it does kinda feel icky [20:28:23] But if the index usage is right in mysql... And we have nothing to need to remap... Because the other dbms don't have the indexes/don't force the usages... [20:29:26] I was meaning removing the mapping in the same change as yours. Because there would be no point in the remapping as all dbs will need to do the schema upgrade if your change is merged. [20:30:17] We've still got an inconsistency across different implementations [20:30:33] Like I say, although sqlite doesn't have that index hinting feature... [20:30:42] oh [21:20:52] Reedy: looks like it will last 2 hours, I think that will be 11pm your time [21:21:00] That's fine :) [21:25:46] paladox: https://gerrit.wikimedia.org/r/#/c/346642 [21:25:49] Looks like you're right [21:26:02] :) [21:26:15] Well... As long as jerkins did actually test the dependant patch [21:26:34] Reedy hmm, lets do another recheck and confirm with zuul [21:27:06] yep it tested both [21:27:23] https://integration.wikimedia.org/ci/job/mediawiki-phpunit-hhvm-jessie/5601/console [21:27:29] I see it tested one, then the other [21:27:35] But did it test one ontop of the other? [21:27:42] 21:18:16 DEBUG:zuul.Cloner:Fetched ref refs/zuul/master/Z2055b9343dea4829b3f5c2e3898754c4 from mediawiki/core [21:27:42] 21:18:16 DEBUG:zuul.Repo:Checking out 296a7664858f89ed2a48c2a778554776785cee42 [21:27:42] 21:18:16 INFO:zuul.Cloner:Prepared mediawiki/core repo with commit 296a7664858f89ed2a48c2a778554776785cee42 [21:27:52] git checkout that hash won't get the dependency [21:28:06] git review would [21:28:14] It should test the one it depends on first [21:28:23] but in the same w/c? [21:28:34] let's just put that commit into the parent [21:28:44] i retrack that ^^ statement it merges the dependant patch onto the patch you are currently testing [21:29:26] Ah. If I put a depends-on... It will, right? [21:29:30] yep [21:29:52] Reedy theres a zuul plugin that should make it easier to view in gerrit. [21:30:11] let's use depends-on to check [21:30:18] yep [21:30:21] that should work [21:30:26] as it worked for me in the past [21:31:00] if depends-on doesn't work... I'm gonna be shouting at Hashar very soon ;) [21:31:25] lol [21:33:06] Reedy according too https://integration.wikimedia.org/zuul/ it is working [21:33:10] by that i mean depends on [21:35:48] Reedy in the next gerrit update depends-on: should be correctly put above the change id https://gerrit.wikimedia.org/r/#/c/346642/ :) [21:36:53] Reedy confirmed that the patch indeed is testing with depends on. [21:37:06] i see [21:37:07] [ 'dropIndex', 'archive', 'ar_usertext_timestamp', [21:37:08] 'patch-archive_kill_ar_usertext_timestamp.sql' ], [21:37:09] [ 'addIndex', 'archive', 'usertext_timestamp', 'patch-archive-usertext_timestamp.sql' ], [21:37:39] inside in includes/installer/MysqlUpdater.php [21:37:48] on change https://gerrit.wikimedia.org/r/#/c/346642/2/ [21:37:48] I should hope it does with depends-on [21:38:06] But that doesn't mean it's testing with it [21:38:23] yeh it does [21:38:52] otherwise it isen't testing 2d39859c779817a9101b1e3303e67d6616f51a58 which means the tests will be testing something other then the change [21:42:09] so. now i've had some food. removing this mapping from the mysql code does make sense [21:42:20] :0 [21:42:23] :) [21:42:27] Reedy late dinner. [21:42:33] The previous layout of the indexName code was crap. And the remaps were in database. And then noop'd in sqlite [21:42:36] which was silly [21:42:43] they were special cases for mysql [21:42:44] yep. [21:44:23] I do wonder if we need 'patch-archive-usertext_timestamp.sql' or we can reuse the old patch version of it [21:44:36] hmm [21:45:47] core/maintenance/archives/patch-archive-user-index.sql is identical [21:45:48] Reedy i carn't seem to find that file [21:45:48] https://github.com/wikimedia/mediawiki/tree/master/maintenance/archives/patch-archive-usertext_timestamp.sql [21:45:49] oh [21:46:09] found it [21:46:09] https://github.com/wikimedia/mediawiki/blob/master/maintenance/archives/patch-archive-user-index.sql [21:47:40] Reedy yes maybe we could drop that. [21:47:50] Though i wonder will this affect old wiki's? [21:48:28] well, it's more, we can reuse the same patch file [21:48:33] Or completely remove that line [21:48:35] Because [21:48:35] [ 'addIndex', 'archive', 'usertext_timestamp', 'patch-archive-user-index.sql' ], [21:48:46] oh, lets replace it [21:49:20] by ^^ that i mean do what you suggested [21:49:44] I'll wait for Tim to sanity check before making further changes.. [21:49:53] ok [21:53:28] * Reedy tests with install.php [21:53:44] MW master as is [21:53:45] | archive | 1 | usertext_timestamp | 1 | ar_user_text | A | 0 | NULL | NULL | | BTREE | | | [21:53:45] | archive | 1 | usertext_timestamp | 2 | ar_timestamp | A | 0 | NULL | NULL | | BTREE | | | [21:55:50] with that patch.. we end up with [21:55:50] | archive | 1 | ar_usertext_timestamp | 1 | ar_user_text | A | 0 | NULL | NULL | | BTREE | | | [21:55:51] | archive | 1 | ar_usertext_timestamp | 2 | ar_timestamp | A | 0 | NULL | NULL | | BTREE | | | [21:55:57] so you saw that I had [21:55:58] [ 'addIndex', 'archive', 'usertext_timestamp', 'patch-rename-ar_usertext_timestamp.sql' ] [21:56:21] addIndex works here because usertext_timestamp is not affected by remapping [21:56:35] it checks if the usertext_timestamp index exists, and if not, renames the old index to the new one [21:57:12] Reedy's patch had two separate patch files, one dropping the index, one adding it, but that is twice as slow, requires copying the archive table twice [21:57:38] why not do it like I had it, with addIndex actually doing a rename, but use drop index/add index in the same ALTER TABLE statement? [21:59:02] Amusingly, I already said something akin to that on your patch :) [22:00:45] I just realised how I made this mistake about the version [22:01:13] I had the MySQL 5.1 manual bookmarked, but the website now redirects to the MySQL 5.7 manual [22:01:13] We might update your patch to for this... As it's the old mysql rename [22:01:21] 5.1 is so old that they've stopped hosting its manual [22:01:30] Haha, that's helpful [22:02:52] do you want me to upload an untested patch? or do you want to wait a couple of hours for a tested one? [22:03:02] because I've got to go do other things right now [22:03:17] I was just going to amend your rename to be [22:03:17] ALTER TABLE /*$wgDBprefix*/archive [22:03:17] DROP INDEX ar_usertext_timestamp, [22:03:17] ADD INDEX usertext_timestamp ( ar_user_text , ar_timestamp ); [22:03:32] Based on me saying it earlier, and you saying the same thing just now [22:03:40] yeah, that's what I have [22:03:59] I'll upload it [22:04:48] there you go [22:05:17] thanks [22:06:11] So the test plan should be.. Install a wiki with REL1_28, swap to master with that patch, check index renamed [22:09:08] s/28/27/ [22:09:08] https://phabricator.wikimedia.org/source/mediawiki/browse/REL1_27/maintenance/tables.sql;14aa034af65b8713cb8c3cf5a31784d48df9d798$497 [22:09:42] as that's where the ar_ prefixed index for archive was created, broken by a removal of the prefix in rel1_28 with the removal of the remapping too [22:11:29] well, except you don't end up with ar_ prefixed there either.. [22:15:40] Isn't this only actually caused if runs tables.sql directly to mysql, and doesn't make MW do it? [22:17:33] oh, duh, remapping [22:17:41] But yes. The patch is semantically correct [22:19:44] https://gerrit.wikimedia.org/r/#/c/345540/ should questionably be merged to stop the rest of the MW code referring to something that isn't right for anything else but mysql. But doesn't break anthing by not being [22:24:04] Tims patch needs cherry picking to REL1_28 and RELEASE-NOTES adding [22:37:56] done that.. [22:38:35] https://github.com/wikimedia/mediawiki/commit/307851618430b8f2573a18fc88d925b671fd36cb should go into REL1_28 for the remapping reverts... I'm not sure if it's worth doing https://github.com/wikimedia/mediawiki/commit/3b000dc2a8743e71fd0b29a194ec80312d8654b3 and Tim's comment updates too... [22:39:01] first one already done. stupid me [22:39:29] So not worth doing the latter, in a point release. As that's followup cleanup [22:45:23] Reedy :) [22:45:39] i see you a patch merged that fixes the problem now :) [23:37:00] Just https://gerrit.wikimedia.org/r/#/c/345540/ to decide about into master then