Good to know.

At least the link is happening through an index :)

The view change was introduced in rOPUP0545ee8d0981: wiki replicas: provide backward compatibility for MCR changes which is actually linked to T174047: Hide deprecated/unused fields on toolforge replica [MCR] rather than T184446.

The data formerly available via page.page_content_model has moved/is moving to content_models.model_name which is connected to the page data via the long chain of page.latest -> revision.rev_id -> slots.slot_revision_id -> slot_roles.slot_content_id -> content.content_id -> content_models.content_model. On the MediaWiki internal side this is probably a nice improvement, but it does add a lot of index scanning overhead in our attempt to provide a backwards compatible view of the page table for Wiki Replica consumers. The really big question here if this is causing significant performance problems is how valuable is page.page_content_model to the typical page table user and how can we inform them that they will need to change their queries to get that data.

I confirm that the only thing all those joins are needed for is back compat for page.page_content_model. It's entirely up to you whether to continue providing this back compat or to make your users change their code.

There's somewhat similar back-compat going on for archive.ar_text_id, archive.ar_content_model, archive.ar_content_format, revision.rev_text_id, revision.rev_content_model, and revision.rev_content_format.

Are queries against the Labs DBs audited and logged, and if so, can we just run a grep on said logs to see how often people query page_content_model? I think the decision to retain or forgo the backwards comparability should be take into account the amount of the users or use cases that need it.

My point is, if the back compatibility can be done using the same names, that is ok, but if it "overshadows" existing tables, we are punishing people updating their code while not encouringing others to do so. I don't mind having a "page_old" or "page_backwards_compatible", but I don't think for those that do full scans of page, we should force them to do scans of revision, which will make their queries break anyway because they will not finish.

root@labsdb1011[enwiki]> flush status;
Query OK, 0 rows affected (0.00 sec)

root@labsdb1011[enwiki]> select count(*) total_pages, sum(page_is_redirect) redirects from page;
+-------------+-----------+
| total_pages | redirects |
+-------------+-----------+
|    44957538 |  10132574 |
+-------------+-----------+
1 row in set (5 min 47.50 sec)

root@labsdb1011[enwiki]> SHOW STATUS like 'Hand%';
+----------------------------+----------+
| Variable_name              | Value    |
+----------------------------+----------+
| Handler_commit             | 1        |
| Handler_delete             | 0        |
| Handler_discover           | 0        |
| Handler_external_lock      | 0        |
| Handler_icp_attempts       | 0        |
| Handler_icp_match          | 0        |
| Handler_mrr_init           | 0        |
| Handler_mrr_key_refills    | 0        |
| Handler_mrr_rowid_refills  | 0        |
| Handler_prepare            | 0        |
| Handler_read_first         | 1        |
| Handler_read_key           | 0        |
| Handler_read_last          | 0        |
| Handler_read_next          | 44957538 |
| Handler_read_prev          | 0        |
| Handler_read_retry         | 0        |
| Handler_read_rnd           | 0        |
| Handler_read_rnd_deleted   | 0        |
| Handler_read_rnd_next      | 0        |
| Handler_rollback           | 0        |
| Handler_savepoint          | 0        |
| Handler_savepoint_rollback | 0        |
| Handler_tmp_update         | 0        |
| Handler_tmp_write          | 0        |
| Handler_update             | 0        |
| Handler_write              | 0        |
+----------------------------+----------+
26 rows in set (0.26 sec)

Meanwhile, the same query on enwiki_p had to be killed after 40 minutes:

root@labsdb1011[enwiki]> use enwiki_p
Database changed
root@labsdb1011[enwiki_p]> flush status;
Query OK, 0 rows affected (0.00 sec)

root@labsdb1011[enwiki_p]> select count(*) total_pages, sum(page_is_redirect) redirects from page;
root@labsdb1011[enwiki_p]> select count(*) total_pages, sum(page_is_redirect) redirects from page;
^CCtrl-C -- query killed. Continuing normally.
ERROR 1317 (70100): Query execution was interrupted
root@labsdb1011[enwiki_p]> SHOW STATUS like 'Hand%';
+----------------------------+---------+
| Variable_name              | Value   |
+----------------------------+---------+
| Handler_commit             | 0       |
| Handler_delete             | 0       |
| Handler_discover           | 0       |
| Handler_external_lock      | 0       |
| Handler_icp_attempts       | 0       |
| Handler_icp_match          | 0       |
| Handler_mrr_init           | 0       |
| Handler_mrr_key_refills    | 0       |
| Handler_mrr_rowid_refills  | 0       |
| Handler_prepare            | 0       |
| Handler_read_first         | 0       |
| Handler_read_key           | 7182424 |
| Handler_read_last          | 0       |
| Handler_read_next          | 0       |
| Handler_read_prev          | 0       |
| Handler_read_retry         | 0       |
| Handler_read_rnd           | 0       |
| Handler_read_rnd_deleted   | 0       |
| Handler_read_rnd_next      | 7182424 |
| Handler_rollback           | 1       |
| Handler_savepoint          | 0       |
| Handler_savepoint_rollback | 0       |
| Handler_tmp_update         | 0       |
| Handler_tmp_write          | 0       |
| Handler_update             | 0       |
| Handler_write              | 0       |
+----------------------------+---------+
26 rows in set (0.24 sec)

The hashtags tool currently has 145 copies of its enHashtagUpdate job running on the job grid. I'm 90% certain that this is related to the page view change.

In T194341#4203763, @jcrespo wrote:

My point is, if the back compatibility can be done using the same names, that is ok, but if it "overshadows" existing tables, we are punishing people updating their code while not encouringing others to do so. I don't mind having a "page_old" or "page_backwards_compatible", but I don't think for those that do full scans of page, we should force them to do scans of revision, which will make their queries break anyway because they will not finish.

Making a "page_backwards_compatible view would require tool maintainers to update their code to use it, but it would be a much simpler update for them to make than fully implementing the 6 way join that the new MCR table structure requires to get to the content model.

In T194341#4201917, @Huji wrote:

Are queries against the Labs DBs audited and logged, and if so, can we just run a grep on said logs to see how often people query page_content_model? I think the decision to retain or forgo the backwards comparability should be take into account the amount of the users or use cases that need it.

To my knowledge the queries made to the Wiki Replica servers are not logged, so I do not think we have an easy way to grep for historical usage. My naive assumption is that usage of the page_content_model field is low. I can't think of many reasons that the content model would matter for tools that are crawling the page table. I'm leaning towards recommending that we remove the new join logic and instead return whatever is in page.page_content_model for now and then change the view in the future to provide an empty string/null response for that field after it has been dropped from the production schema. Tools that do need the page_content_model data can be updated to do the needed joins themselves rather than relying on a view layer shim.

To my knowledge the queries made to the Wiki Replica servers are not logged

We have[[ https://dev.mysql.com/doc/refman/5.7/en/performance-schema-statement-digests.html | performance_schema ]] enabled on all MariaDB servers, which doesn't log each individual query, but gives an idea of what kind of queries are the most frequent (distribution). At any time we can enable finer logging, at the cost of performance. This is not publicly available because of privacy issues (it can provide usage patterns or leak private data -at least on production).

If we keep the backward-compatible field in the views, there is one join that can be removed (revision), but most of that will still remain. I don't have any real insight at all into how often or where that field is used by folks in tools, but replacing it with NULL might break fewer things that might include data structures that depend on its existence while still making unusable. If that one is a big problem, that seems like damage control?

Overall, I think it is better to break a couple tools than to break all of them by overburdening the DBs.

page_latest could be used in place of revision_id, I think, since that's what the join is on, which would cut out that one very large table. I'm not sure if that would resolve this entirely, though.

Then it would be

page.latest -> slots.slot_revision_id -> slot_roles.slot_content_id -> content.content_id -> content_models.content_model

That could be problematic when slots increases a lot (which it presumably will).

Change 433085 had a related patch set uploaded (by Bstorm; owner: Brooke Storm):
[operations/puppet@production] wiki replicas: return page to a full view

https://gerrit.wikimedia.org/r/433085

Change 433085 merged by Bstorm:
[operations/puppet@production] wiki replicas: return page to a full view

https://gerrit.wikimedia.org/r/433085

Change 433206 had a related patch set uploaded (by Bstorm; owner: Bstorm):
[operations/puppet@production] wiki replicas: depool labsdb1010

https://gerrit.wikimedia.org/r/433206

Change 433206 merged by Marostegui:
[operations/puppet@production] wiki replicas: depool labsdb1010

https://gerrit.wikimedia.org/r/433206

Mentioned in SAL (#wikimedia-operations) [2018-05-17T14:35:18Z] <marostegui> Reload haproxy on dbproxy1010 to depool labsdb1010 https://phabricator.wikimedia.org/T174047 https://phabricator.wikimedia.org/T194341

How much longer until this actually takes effect? I reran the query right now and the EXPLAIN is still listing the revision table.

@Huji There are 3 servers with high load, it takes a bit to deploy changes there :-) Thanks @Bstorm and @Marostegui for the ongoing work on this!

Change 433591 had a related patch set uploaded (by Bstorm; owner: Bstorm):
[operations/puppet@production] wiki replicas: depool labsdb1011

https://gerrit.wikimedia.org/r/433591

Change 433591 merged by Marostegui:
[operations/puppet@production] wiki replicas: depool labsdb1011

https://gerrit.wikimedia.org/r/433591

Mentioned in SAL (#wikimedia-operations) [2018-05-17T16:11:28Z] <marostegui> Reload haproxy on dbproxy1010 to depool labsdb1011 T174047 T194341

Finally got out the full view of the page table without joins to all three replicas.