[01:42:01] <Krinkle>	 Reedy: if you have a minute - https://gerrit.wikimedia.org/r/#/q/is:open+owner:self+(message:tests+OR+message:build)
[01:42:05] <Krinkle>	 few minor commits to libs
[01:43:08] <Reedy>	 0 results Krinkle
[01:43:18] <Reedy>	 well, cause self != Reedy
[01:43:18] <Reedy>	 :P
[01:43:27] <Krinkle>	 https://gerrit.wikimedia.org/r/#/q/is:open+owner:Krinkle+(message:tests+OR+message:build)
[01:43:28] <Krinkle>	 sorry :D
[01:43:28] <Reedy>	 https://gerrit.wikimedia.org/r/#/q/is:open+owner:Krinkle+(message:tests+OR+message:build)
[01:43:46] <Krinkle>	 Reedy: also, good news about phpunit 4.8 getting a compat layer for phpunit 5 and 6
[01:43:54] <Reedy>	 :o
[01:44:13] <Krinkle>	 that'll make it easy for us to adopt phpunit 6 API and keep 4.8 by default, but in Jenkins locally upgrade to 6 for faster and less buggy coverage
[01:48:42] <Reedy>	 TimStarling: Can you have a look at https://gerrit.wikimedia.org/r/#/c/344601/2 please? I want to get this fixed in master/REL1_28 for the upcoming releases... I don't know if there's really a better way, other than this revert, and probably a few others which used these features until they were removed
[01:49:39] <Reedy>	 https://phabricator.wikimedia.org/T154872 and https://phabricator.wikimedia.org/T104756
[01:49:59] <Reedy>	 Specifically, https://phabricator.wikimedia.org/T104756#3128022
[02:00:49] <TimStarling>	 so... in the USE INDEX clauses it was changed from ar_usertext_timestamp to usertext_timestamp?
[02:01:30] <Reedy>	 Yeah, that's what indexName did
[02:02:48] <TimStarling>	 I mean e.g. ApiQueryDeletedRevs:
[02:02:52] <TimStarling>	 		$this->addOption(
[02:02:53] <TimStarling>	 			'USE INDEX',
[02:02:53] <TimStarling>	 			[ 'archive' => ( $mode == 'user' ? 'usertext_timestamp' : 'name_title_timestamp' ) ]
[02:02:53] <TimStarling>	 		);
[02:04:35] <TimStarling>	 the theory is that the sqlite difference with mysql is resolved?
[02:05:10] <Reedy>	 not exactly, and it's not likely to be unless we kill off the archive table
[02:05:18] <Reedy>	 sqlite can't have multiple indexes with the same name
[02:05:24] <Reedy>	 mysql can (on different tables)
[02:05:35] <TimStarling>	 yeah, you linked to the commit where I fixed this in the first place
[02:06:04] <TimStarling>	 I introduced indexName()
[02:06:19] <TimStarling>	 so I'm wondering what has changed to allow it to be removed
[02:06:25] <Reedy>	 It seems to mostly have been a misunderstanding during the 1.28 release cycle that these indexes had been normalised everywhere
[02:06:41] <Reedy>	 Aaron specifically removed the remappings in https://github.com/wikimedia/mediawiki/commit/bec6151e5fe30e780d82527c4c53e54379149a4e
[02:07:12] <TimStarling>	 he just had no clue what he was doing?
[02:07:35] <TimStarling>	 I think ApiQueryDeletedRevs has always been an SQL error on SQLite
[02:07:50] <Reedy>	 Kinda looks like that... I don't know if it was a case of "it works on my dev wiki"
[02:09:11] <TimStarling>	 well, you said that other patches should be reverted so that code uses ar_usertext_timestamp
[02:09:17] <Reedy>	 Finding when some stuff changed is obviously made harder by usual code churn etc breaking blame
[02:09:34] <TimStarling>	 but it seems that it was usertext_timestamp from the outset, and nobody noticed that it was broken on SQLite
[02:09:57] <TimStarling>	 probably Special:DeletedContributions has always given an SQL error on SQLite
[02:11:16] <TimStarling>	 ok you didn't exactly say that
[02:11:16] <Reedy>	 but even on mysql now we're doing...
[02:11:18] <Reedy>	 CREATE INDEX /*i*/ar_usertext_timestamp ON /*_*/archive (ar_user_text,ar_timestamp);
[02:11:37] <TimStarling>	 right, and you're tracking down indexName() removal
[02:11:40] <Reedy>	 So the SQL query dumped in the desc on T154872 would fail similarly, on newer mysql
[02:11:41] <stashbot>	 T154872: Special:DeletedContributions query error on wikis created after Mediawiki 1.28 upgrade - https://phabricator.wikimedia.org/T154872
[02:11:58] <Reedy>	 not newer mysql, newer mw installs using mysql
[02:14:06] <TimStarling>	 the indexName() call is still there, in Database::replaceVars()
[02:14:47] <Reedy>	 Just it's a noop after the mappings being removed
[02:15:33] <Reedy>	 Presumably because the remappable usages were removed at some stage prior
[02:16:53] <TimStarling>	 if I understand this correctly, your revert is fine and mergeable
[02:17:13] <Reedy>	 Right, I just don't think it fixes the problem completely...
[02:17:34] <TimStarling>	 after your revert, the ar_usertext_timestamp in tables.sql will be mapped to usertext_timestamp, consistent with previous installed wikis
[02:18:02] <TimStarling>	 DeletedContributions has always been an SQL error on SQLite and will continue to be so
[02:18:24] <TimStarling>	 so that could be fixed
[02:18:46] <Reedy>	 https://github.com/wikimedia/mediawiki/blob/e01fd443887b47c86d5248a4a32eca5e5ed98a97/includes/specials/pagers/DeletedContribsPager.php#L132
[02:18:54] <Reedy>	 That line wants to become ar prefixed again
[02:19:02] <TimStarling>	 it's never been ar prefixed
[02:19:23] <TimStarling>	 oh ok, it was actually
[02:19:44] <TimStarling>	 very briefly in 2011, and then reverted straight afterwards
[02:19:57] <Reedy>	 that was Roans patch IIRC?
[02:20:07] <Reedy>	 Or was he reverting it..
[02:20:19] <Reedy>	 https://github.com/wikimedia/mediawiki/commit/a7c7a3fd3305dc118416bebff2d4d209e26cfd88#diff-b66afbfd39311e7420cd927df1d153bcL47
[02:20:24] <Reedy>	 That's 2009
[02:21:09] <TimStarling>	 the stupid...
[02:21:26] <TimStarling>	 anyway, if you remove your -1 then I can merge your revert
[02:21:40] <TimStarling>	 and then we can fix bugs that have been present since 2009 in some subsequent commit, right?
[02:21:49] <Reedy>	 Removed
[02:21:52] <Reedy>	 Haha, right
[02:22:03] <Reedy>	 We also need to fix the inconsistency in mysql
[02:22:11] <Reedy>	 As some versions will have it prefixed, some won't
[02:22:26] <Reedy>	 And as WMF isn't going to be adding the prefix anytime soon... We should probably patch MW to remove it again
[02:22:44] <Reedy>	 And stop it adding it in first place if someone was doing a really old upgrade
[02:24:57] <TimStarling>	 I'm going to have lunch
[02:25:27] <Reedy>	 Probably want a stiff drink to go with it
[02:27:26] <Reedy>	 Thanks. I should probably get some sleep, and then break down what else needs to be done to finish tidying this up
[02:27:50] <Reedy>	 I guess two more patches needed... One to fix up the index usage in the MW code. And then one further to fix the indexes on mysql
[02:35:49] <Reedy>	 TimStarling: When you get back, could you merge on REL1_28 please? https://gerrit.wikimedia.org/r/#/c/344602/ to put us at a level playing field for me fixing the rest up later
[03:54:58] <TimStarling>	 I thought, really the problem was that comment "Backwards-compatibility hack"
[03:55:17] <TimStarling>	 it seems to imply that it can be removed after some period of time
[03:55:27] <TimStarling>	 so I wondered what idiot wrote that comment
[03:55:35] <TimStarling>	 of course the answer was me
[04:02:09] <TimStarling>	 there's some discussion in CR
[11:26:38] <Reedy>	 Well, if we didn't have big tables on Wikimedia (the biggest), we could've fixed the indexes
[12:34:31] <Reedy>	 TimStarling: I think that's all the patches we need created (including unbreaking the deleted revisions api/special page).. Just need some poking at the migration patches to stop breaking SQLite, but also fixing/normalising MySQL in the process...
[17:12:03] <James_F>	 anomie: Thank you very much for the quick merge.
[19:46:52] <TimStarling>	 good stuff