[13:40:01] TimStarling: do you want someone else to look at ResultWrapper patch or I just merge it? I went though it twice already and I'm satisfied. plus, this class is used so much that the fact integration tests all pass give a lot of confidence [13:41:05] you can merge it [13:42:07] I think most callers just do foreach ( $res as $row ) which is pretty certain to work. More complicated things are not done all that often [13:42:37] grrr... release note merge conflict! [13:43:04] I'll rebase in a minute, there's some stuff in gate-and-submit which will conflict too [13:43:15] the point of the change is to make $res->key() give something sensible, that wasn't working before but nobody noticed [13:44:11] and it also now just feels right :) [15:44:48] Pchelolo: low prio, but perhaps could do with a status update regarding https://phabricator.wikimedia.org/T249745 [15:45:04] Krinkle: kk, will look at it again. [15:45:10] (e.g. what is waiting for whom, etc next steps) [15:46:25] we have an virtual offsite this week, will do early next week [18:06:10] TimStarling: I haven't yet reviewed https://gerrit.wikimedia.org/r/c/mediawiki/core/+/574101/25 but I do understand now, 2 months later, that it relates to the multi-dc patch for sqlbagostuff, and that it relates to batching setMulti. [19:11:55] does anything actually call setMulti? [19:13:28] ok maybe there is one thing in core that uses it (ResourceLoader's KeyValueDependencyStore) [19:16:38] TimStarling: right, that was my question as well. [19:16:42] nothing in prod currently [19:17:02] but there is one prominent use case (RL) currently blocked on being enabled in prod until MainStash switches from Redis to MainStash DB [19:17:21] s/blocked on/blocked from/ [19:17:58] but afaik that use case is in a deferred update so latency isn't too sensitive there, but as it can easily have a hundred rows to update, it seems preferably to batch that. [19:19:09] "upsert multiple rows" sounds a lot more reasonable (to me) than what the patch stack previously appeared to enable in terms of merging rows which sounded rather edge-casey. [19:19:15] but I just didn't understand the nuance there [19:19:59] if I understand correctly, calling it with multiple rows today basically results in generally unexpected outcomes. [19:21:02] however, looking at the patch now, it seems a lot more non-trivial in terms of the resulting query than what I expected. [19:21:54] if all that is required, then maybe it's more accurate to say mysql doesn't support multi-row upsert. [19:22:14] it supports multi-row replace, that is trivial [19:22:21] yeah [19:22:55] the modtoken patch switches set() from replace to upsert in order to not overwrite newer rows. [19:23:05] it needs some kind of condition there [19:24:38] I would be fine with merging it if there is a thing that definitely needs it, I mostly just wanted a task on phabricator explaining what needs it and why the existing/simpler solutions don't work [19:25:45] oh I see, you linked it to T274174 just today [19:25:45] T274174: Add modtoken field and flags to objectcache table - https://phabricator.wikimedia.org/T274174 [19:26:51] in a multi-dc mainstash setup, both dcs will write to their local db (no replicas in use, although we will have a replica for failover purposes, nothing elese), and there are two thigns we scribbled down as requirements that motivate the 'modtoken' column and by extent the need for (something like) upsert. 1) eventual consistency and 2) letting old statements naturally no-op when playing them back if a server is out of rotation for a little [19:26:51] while. [19:27:33] so most queries bascially in addition to treating old exptime as if the row doesn't exist (which we already do today) we'd also no-op writes if the existing row is newer than what we're trying to write. [19:27:48] right [19:27:56] the way Aaron did that for set(), is to have the upsert condition assign itself if modtoken is older. [19:28:23] e.g. value=IF(modtoken>"newtoken",newvalue,value) [19:28:26] so this is a definite plan? the task description makes it sound hypothetical [19:28:43] hardware is up and running and idle in both DCs [19:28:50] and SRE is waiting to decom redis stash [19:28:55] ok [19:29:30] more context and patches at https://phabricator.wikimedia.org/T212129 [19:29:33] (parent task) [19:29:59] I did not realize until yesterday syncing with Aaron, that this upsert IF() approach does not work when there are multiple rows [19:30:10] it does something, but not the right thing. [19:30:19] Aaron knew this months ago, but I hadn't connected the dots between these patches yet [19:33:46] I don't think its out of the question to keep these serially though, and perhaps deprecate/throw in rdbms for passing multiple rows (or if we have a good use case for the current seemingly-odd outcome, to document it very clearly). [19:34:05] or maybe there's a simpler approach there that we haven't thought of that would allow batching [19:34:32] but the approach aaron is suggesting in https://gerrit.wikimedia.org/r/c/mediawiki/core/+/574101/ would work? [19:35:51] here's an alternative: https://www.php.net/manual/en/mysqli.quickstart.multiple-statement.php [19:36:52] I haven't reviewed or tested the proposed approach. [19:37:21] but it certainly seems complex and novel, not something afaik well-documented as "here is how you do multi-row upsert in mariadb". [19:37:34] and I'm generally trying to avoid new inventions for something that is unlikely to be unique to us [19:40:00] if we had a mysqli::multi_query() wrapper, it might be useful for other things, right? [19:40:24] making it be convenient to use with query builder functions might be complicated [19:41:47] yeah, might not need much of a wrapper. We generally use mysqli_ functions directly in the Database subclass and this optimisation could initially be specific to that. [19:42:09] with the default being the current fallback the base class has already which is try insert, else update [19:42:37] (for each row) [19:43:09] but yeah, generalising it further could be beneficial as well for sure [19:43:58] PG has async query execution so it could optimise query batches that way