[14:34:33] tgr|away: String lengths of the individual parameters? No. Why? [14:44:08] pgsql is failing epicly on travis again [16:57:00] Reedy: When I try it locally, it looks like no one ever patched PG to add the ip_changes table. [17:00:51] MatmaRex: the replacement for jquery.placeholder is just to set a placeholder property? [17:03:17] legoktm: yeah? [17:03:27] anomie: Yeah.. there's a bug about it [17:04:22] https://phabricator.wikimedia.org/T177258 [17:07:58] MatmaRex: I was going to send out an announcement to wikitech-ambassadors that those modules (badge/placeholder) are really going away in two weeks and its not going to get reverted again [17:09:40] fair [17:42:19] anomie: just wanted to know if I missed it; I'll write a patch then [17:42:35] would you prefer reusing PARAM_MAX or something separate? [17:44:22] tgr: PARAM_MAX is very different, don't try to reuse it. And explain why or you'll get YAGNI as a reply. [18:11:32] anomie: so I can set max length in my API modules... or what do you mean? [18:14:09] tgr: Why should so many things care about the string lengths of parameters that it should be added in core? [18:14:31] Versus that being part of some more specialized parameter validation. [18:14:39] done by the module. [18:15:13] seems like a very basic thing to me [18:15:34] how do existing modules handle it? [18:15:50] pretty much anything that will end up in the database has size limits [18:17:23] and clients want to know about those limits so without core support modules would have to add an error + separately some kind of help message integration, which seems awkward [18:19:02] probably things like ApiSandbox that build a UI should be able to learn about them in a standard way, too, so they can turn them into input field limits [19:05:15] tgr: OTOH, to do those sorts of things you also probably need to differentiate between byte length and character length. As for current handling, edit summaries get truncated, overlong titles or usernames or page contents are rejected as invalid, and other stuff is probably handled in one of these two ways too. It's probably not uncommon for the limit not to actually be exposed where the API can get at it in a straightforward manner. [19:07:02] that sounds like YGNI to me :) [19:08:21] I think character length is not really useful? Things almost always end up byte-limited since that's what the database cares about [19:10:28] anomie: there could be a separate MAX_BYTES and MAX_CHARS but the second does sound like YAGNI (why would someone prevent a string from being saved if it does fit into the DB field?) [19:11:20] tgr: The whole point of the comment table change was to do it as a character length. [19:11:41] The database isn [19:11:46] 't the only thing that limits lengths. [19:36:01] https://github.com/wikimedia/mediawiki-extensions-WikimediaMaintenance/blob/master/rcParamsTypeCheck.php [19:36:06] That's a really bad copy paste [19:38:55] https://github.com/macbre/index-digest [19:38:59] "By running index-digest at Wikia on shared database clusters (including tables storing ~450 mm of rows with 300+ GiB of data) we were able to reclaim around 1.25 TiB of MySQL storage space across all replicas." [20:11:36] Wondering aloud: do the "CAS update failed on user_touched for user ID" errors need to actually throw an exception? Or could they live with just logging it? [20:11:45] (cf: is anyone ever going to "fix" this finally?) [20:19:58] anomie: https://gerrit.wikimedia.org/r/#/c/390058/1..3/includes/api/ApiStashEdit.php :) [20:26:04] no_justification: I don't think any of us know what the "fix" is at least in the general case :/ [20:26:20] My main complaint is spammy logs :p [20:26:35] Which is why I'm asking about logging it as ERROR instead of throwing [20:30:50] usually the fix is to make sure whatever is writing to the user object is loading from the master [20:31:08] but there's other weird stuff too, AaronSchulz probably knows it the best [20:34:19] On of them seems to be in BetaFeatures' saving of preferences, when using api.php, at the restInPeace() phase [20:35:13] that sounds ... gross [20:35:33] the php code is calling back to api.php? [20:35:44] No no [20:36:07] Someone's making a request to api.php, and during the final restInPeace() phase some deferred update ends up calling BetaFeaturesHooks [20:36:14] ah [20:36:19] #0 /srv/mediawiki/php-1.31.0-wmf.7/extensions/BetaFeatures/BetaFeaturesHooks.php(287): User->saveSettings() [20:36:19] #1 /srv/mediawiki/php-1.31.0-wmf.7/includes/deferred/MWCallableUpdate.php(30): Closure$BetaFeaturesHooks::getPreferences() [20:36:19] #2 /srv/mediawiki/php-1.31.0-wmf.7/includes/deferred/DeferredUpdates.php(259): MWCallableUpdate->doUpdate() [20:36:19] #3 /srv/mediawiki/php-1.31.0-wmf.7/includes/deferred/DeferredUpdates.php(210): DeferredUpdates::runUpdate(MWCallableUpdate, Wikimedia\Rdbms\LBFactoryMulti, string, integer) [20:36:19] #4 /srv/mediawiki/php-1.31.0-wmf.7/includes/deferred/DeferredUpdates.php(131): DeferredUpdates::execute(array, string, integer) [20:36:19] #5 /srv/mediawiki/php-1.31.0-wmf.7/includes/MediaWiki.php(897): DeferredUpdates::doUpdates(string) [20:36:19] #6 /srv/mediawiki/php-1.31.0-wmf.7/includes/MediaWiki.php(719): MediaWiki->restInPeace(string, boolean) [20:36:20] #7 [internal function]: Closure$MediaWiki::doPostOutputShutdown() [20:36:20] #8 {main} [20:36:34] BF does some kind of evil things :/ [20:37:23] I vaguely remember some "trick" in there for the enable all bf features functionality [20:37:45] Why on *earth* would we be calling BetaFeaturesHooks::getPreferences() during shutdown?! [20:38:11] Actually, that stacktrace is pretty useless. We don't know what *injected* the stupid deferred update [20:38:51] yeah, just that its a closure that touches BF [20:39:23] Well, it could be the GetPreferences hook's fault [20:39:28] Does it pass a master-ok user? [20:39:35] Or does it pass a slave? [20:39:52] I'm thinking the latter. In which case: BF shouldn't be writing to savePreferences() [20:40:37] Ahhh, it's for the auto-enroll behavior I think [20:40:44] This is ugly code [20:40:48] * no_justification runs off for a beer [20:41:03] yeah that was my random guess [20:41:07] * bd808 looks at it [20:42:22] so it probably needs a guard on this whole thing somehow... [20:46:01] I wonder what the deferred is that leads to loading the user's prefs? [20:46:30] probably no good way to find that [20:50:30] my bet is on EditPage::updateWatchlist() actually [20:51:11] make an api call that edits a page and then in a deferred it looks to see if that page should be watchlisted [20:51:54] That call should probably load the users prefs before registering the diferred [20:53:16] actually the short circuit in the deferred callback there could be hoisted up... [21:01:24] that whole ramble by me above may be bogus [21:01:43] I'm not seeing the obvious link to prefs being loaded there [21:05:48] We could just remove BetaFeatures [21:05:50] :p [21:15:16] or change how the "give me all the beta features" flag works [21:15:49] I think it auto-selects things for dubious reasons of tracking adoption [21:16:06] not my call though I suppose [21:16:17] * bd808 wonders off to read resumes [21:19:28] async stack traces for PHP! [21:19:56] we could probably add some special handling for MWCallableUpdate if this is a problem that comes up often [21:21:23] in any case, CAS errors require an exception usually; user_touched maybe not, although IIRC there are some caching situations where not updating it could have weird side effects [21:21:53] we use it to make sure anon users don't get cached views right after editing a page, or something like that [22:18:32] bd808: normally the user is loaded via DB_REPLICA (as it should). Probably some hook just takes the user object provided and tries to save it. That normally works unless there is another request doing something to the user (often the same thing if a mere GET triggers) or another update happened in the same request with a properly loaded-for-update (DB_MASTER) version of the user, making the other main object for the user [22:18:34] unaware and still using the old values and CAS number. [22:19:36] User::getInstanceForUpdate is usually used for updates like that (which de-duplicates master instances if there are several callers). [22:19:54] "usually" as in "should be", heh [22:23:25] de-duplicating also avoid CAS errors when two callers use the master and *then* both do their updates [22:27:31] that said, doing those on GET is ugly to begin with [22:28:47] *nod* the BetaFeatures preference hook is doing ugly things [22:29:28] if you have the pref set to turn on all new beta features then it modifies your preferences and saves when there is a BF that you don't have on yet [22:29:53] ideally that whole process would be rethought, but ... yeah [22:36:01] you are working on multi-DC stuff, AaronSchulz? Victoria reminded me yesterday that we need someone to lead the program now that Gabriel has left [22:42:26] TimStarling: what does that entail? [22:42:56] quarterly and annual planning [22:45:00] potentially Cindy could take it on (haven't asked her yet) but she doesn't know much about it [22:52:41] you know what I think about it, I think we should prioritise a pilot production deployment, at the expense of performance [22:53:07] i.e. I'm not so keen on the long tail of T92357 [22:53:08] T92357: Fix problematic database master queries performed on HTTP GET/HEAD - https://phabricator.wikimedia.org/T92357 [22:57:53] TimStarling: I'd like to have rollback fixed first. Other stuff can probably be done later. [22:58:46] * AaronSchulz also wonders who and to what extent would be working on the project [23:01:44] yeah, annual planning for FY2019 has already started, and includes answering that to some extent [23:02:37] we didn't have much time allocated for multi-DC in the FY2018 MWPT annual plan [23:04:57] the comments on T175672 are surprisingly lacking in clue, nobody has figured out what the errors mean or why they happen? [23:04:57] T175672: Make apache/maintenance hosts TLS connections to mariadb work - https://phabricator.wikimedia.org/T175672 [23:09:46] TimStarling: I suppose if that constant in the php source was change to 1.2 and php was recompiled and tested, that might confirm whether it is https://bugs.php.net/bug.php?id=74445 or something else going on as well [23:09:56] a proxy would be useful to avoid latency anyway though [23:13:00] regardless of other issues, that bug is a blocker since we only allow 1.2 [23:13:34] (a blocker for using PHP driver TLS) [23:25:25] it apparently works in HHVM, but fails in PHP 5.6 [23:58:36] did you know that libmysqlclient bundles its own SSL library?