In P75285 I pasted the problematic query and its explain, then a trimmed-down version that still reproduces the issue (i.e., index isn't used with > 1 predicates), and a potential fix: P75285#302739. We're essentially just grouping common subexpressions in the query. This really is just playing with the optimizer, but the manual for index_merge actually suggests something similar: https://dev.mysql.com/doc/refman/8.4/en/index-merge-optimization.html

I imagine the fix could be along the lines of: introducing a new method to DatabaseBlockStore, similar to getRangeCond but accepting multiple ranges. Internally, the method would essentially just loop over all ranges and do what it already does, except that the $start === $end case is deduplicated by keeping track of all the relevant $start values, and making a single condition with all of them at the end.

And perhaps that could be done publicly in gerrit? It could easily be disguised as a refactoring / interface improvement / query readability change.

In general though, I'm not sure if changing the query will be enough to convince the optimizer for all possible inputs. And then there's the question of how other DBMSs will behave. Introducing a new composite index on (bt_ip_hex, bt_range_start, bt_range_end) to avoid the index_merge should also solve the problem.

@tstarling Hi what do you think of the above comment? Since you did the block refactor.

In T392309#10755542, @Daimona wrote:

I imagine the fix could be along the lines of: introducing a new method to DatabaseBlockStore, similar to getRangeCond but accepting multiple ranges. Internally, the method would essentially just loop over all ranges and do what it already does, except that the $start === $end case is deduplicated by keeping track of all the relevant $start values, and making a single condition with all of them at the end.

And perhaps that could be done publicly in gerrit? It could easily be disguised as a refactoring / interface improvement / query readability change.

Sounds good. Do you want me to write it?

In general though, I'm not sure if changing the query will be enough to convince the optimizer for all possible inputs. And then there's the question of how other DBMSs will behave. Introducing a new composite index on (bt_ip_hex, bt_range_start, bt_range_end) to avoid the index_merge should also solve the problem.

A schema change would be more work -- maybe we can bundle this with T389028 if we decide to add an index for that. I suggest deploying the query and monitoring the logs to see if we still have a problem.

If anyone has a million blocks in PostgreSQL and complains that their queries are slow, we can just tell them to migrate to MariaDB.

In T392309#10763298, @tstarling wrote:

In T392309#10755542, @Daimona wrote:

I imagine the fix could be along the lines of: introducing a new method to DatabaseBlockStore, similar to getRangeCond but accepting multiple ranges. Internally, the method would essentially just loop over all ranges and do what it already does, except that the $start === $end case is deduplicated by keeping track of all the relevant $start values, and making a single condition with all of them at the end.

And perhaps that could be done publicly in gerrit? It could easily be disguised as a refactoring / interface improvement / query readability change.

Sounds good. Do you want me to write it?

Thank you, I'll write it now and commit it as a cleanup.

In T392309#10763304, @Daimona wrote:

Thank you, I'll write it now and commit it as a cleanup.

https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1138500

Erring on the side of caution, I didn't tag this task. But maybe that'd be fine?

Patch merged. It'll go out with the train next week, but I imagine it could be cherry-picked to wmf.25 if needed. Do we want to backport the patch to the applicable release branches too? I just realized this task was NDA and not security, so tagging Security-Team now for input...

In T392309#10765850, @Daimona wrote:

I imagine it could be cherry-picked to wmf.25 if needed.

With this task still private and the gerrit change set being fairly benign IMO, I don't think this necessitates a late-Thursday or Friday deployment. I'm happy to deploy it early on Monday the 28th though, if we don't want to wait for it to roll out with the train.

Do we want to backport the patch to the applicable release branches too?

Yes. We should definitely try to backport to the current, supported release branches, if possible.

In T392309#10766307, @sbassett wrote:

In T392309#10765850, @Daimona wrote:

I imagine it could be cherry-picked to wmf.25 if needed.

With this task still private and the gerrit change set being fairly benign IMO, I don't think this necessitates a late-Thursday or Friday deployment. I'm happy to deploy it early on Monday the 28th though, if we don't want to wait for it to roll out with the train.

Make sense, and I imagine this would be up to SRE anyway (depending on whether we get more instances of the expensive query).

Do we want to backport the patch to the applicable release branches too?

Yes. We should definitely try to backport to the current, supported release branches, if possible.

Can I go ahead and do that in gerrit, or would you rather wait for the next security release?

In T392309#10766972, @Daimona wrote:

In T392309#10766307, @sbassett wrote:

Yes. We should definitely try to backport to the current, supported release branches, if possible.

Can I go ahead and do that in gerrit, or would you rather wait for the next security release?

Since the change set for master is already public and merged, I think it's fine to backport now. @Reedy can decide if he wants to additionally mention this issue with T389302.

I looked into backporting but it's messy... In REL1_43, we need to make it work for the old block schema. In REL1_42 the code is different due to r1023560; maybe that one should be backported first? Other release branches don't have this method, or the new block schema for that matter.

(assuming based on the above that this task is no longer NDAed)