Page MenuHomePhabricator

Hide deprecated/unused fields on toolforge replica [MCR]
Open, NormalPublic

Description

Fields that will become unused with MCR schema migration should be hidden in the toolforge replica of the wiki databases, to avoid tools from reading incomplete/inconsistent data. These fields will eventually be dropped anyway, and will stop to be updated when T198312 is done, which is expected to happen by the end of the year MCR migration plan. Affected fields are:

page.page_content_model
revision.rev_content_model
revision.rev_content_format
revision.rev_text_id

These fields should become unavailable on toolforge soon, following an appropriate notice period.

NOTE: this ticket originally asked for compatibility to be maintained by adding joins to the views for the page and revision table. This idea has been modified due to performance issues, see T195515. -- Compatibility is now opt-in with the _compat tables, which are now sometimes the faster tables due to the number of joins involved in direct use of the actor and comment tables, depending on use case.

Migrating Tools to the New Schema

Tools that need to know the content model of a revision's content can look it up using the new slots and content tables, which are already available on labs. The following query returns the content model of a revision's main slot:

SELECT content_model, model_name
FROM slots
JOIN content ON slot_content_id = content_id
JOIN content_models ON model_id = content_model
WHERE slot_revision_id = 12345 AND slot_role_id = 1.

The slot_role_id = 1 in the end indicates the revision's main slot. See https://www.mediawiki.org/wiki/Multi-Content_Revisions/Database_Schema for reference.

The last join provides the model name for the model ID. It may be more sensible to do this programmatically: This mapping never changes and only very rarely grows, so it can be cached aggressively, or even be hard coded for the well known models (e.g. 1 for wikitext).

NOTE: rev_content_format and rev_text_id are only relevant when loading serialized blobs from external storage, which is not possible on toolforge. They are removed without replacement.

Related Objects

StatusAssignedTask
Declineddchen
OpenNone
OpenNone
DuplicateNone
OpenNone
ResolvedAbit
OpenNone
OpenNone
OpenNone
OpenNone
DuplicateNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
Resolvedppelberg
ResolvedKrinkle
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
StalledNone
OpenNone
OpenNone
OpenNone
OpenNone
ResolvedAnomie
ResolvedAnomie
Resolvedtstarling
ResolvedAnomie
ResolvedAnomie
ResolvedAnomie
ResolvedAnomie
ResolvedAnomie
ResolvedAnomie
ResolvedAnomie
ResolvedAnomie
Resolveddaniel
ResolvedAnomie
ResolvedAnomie
ResolvedMarostegui
ResolvedAnomie
Resolvedtstarling
ResolvedAnomie
Resolveddaniel
ResolvedAnomie
ResolvedAnomie
ResolvedAnomie
Resolveddaniel
Resolveddaniel
Resolveddaniel
ResolvedNone

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
bd808 added a subscriber: bd808.Apr 6 2018, 7:06 PM
Tgr added a subscriber: Tgr.Apr 8 2018, 4:58 PM
Bstorm added a subscriber: Bstorm.Apr 12 2018, 7:01 PM

So we need to filter on role_name = 'main'? I want to make sure I understand that piece while I throw the views together for this.

For these compatibility views, yes.

I'm messing around with these and making sure I understand right. I've been assuming this sort of thing is going to be a left join so that we get old stuff and new stuff. Should this be an inner join (actually filtering rows out, which is a bit more severe)? I mean, are we going to write everything to the new columns, so I'm not filtering page records out? The logical AND gave me pause on strategy here :)

For the time being, yes, left joins would be good. When the old columns are dropped we might change them to regular joins.

Change 430024 had a related patch set uploaded (by Bstorm; owner: Bstorm):
[operations/puppet@production] wiki replicas: provide backward compatibility for MCR changes

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

Change 430024 merged by Bstorm:
[operations/puppet@production] wiki replicas: provide backward compatibility for MCR changes

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

So one last thing on this (with the exception of changes needed just before columns are removed). When I run some of the resulting queries from that patch, I noticed that the slots table does a full table scan.
This one in particular (understanding that wiki is my local DB):

LEFT JOIN 
  (`wiki`.`slots` JOIN `wiki`.`slot_roles` on (slot_role_id = role_id AND role_name = 'main') 
  JOIN `wiki`.`content` on (slot_content_id = content_id) 
  JOIN `wiki`.`content_models` on (content_model = model_id)) 
on slot_revision_id = ar_rev_id;

I'm looking in case there is something that needs to be indexed here. Since the join loads slots with a lot of joins, then it filters via the last line, perhaps that's just to be expected here, though. Adding additional indexes so far haven't changed that.

Anomie added a comment.May 4 2018, 2:58 PM

Since these tables are all empty at the moment, it's reasonably likely that the database engine is simply noticing that fact and deciding that a full scan of empty tables is more efficient than using empty indexes.

Once we do populate these tables (see T183488 and T182682) we should revisit that to ensure that proper indexes are being used.

jcrespo added a subscriber: jcrespo.May 4 2018, 3:26 PM

Full table scans are indeed expected and preferred when tables have 0 to a only a few rows. However, I think Bstorm is right to be concerned, we have seen lately an increase in load on labsdbs, but I have yet to check if it is related to the usage of views or just coincidental.

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

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

Change 430942 merged by Marostegui:
[operations/puppet@production] wiki replicas: depool labsdb1010 for MCR changes

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

Mentioned in SAL (#wikimedia-operations) [2018-05-07T08:19:41Z] <marostegui> Reload haproxy on dbproxy1010 to depool labsdb1010 - https://phabricator.wikimedia.org/T174047

Mentioned in SAL (#wikimedia-operations) [2018-05-07T14:59:45Z] <marostegui> Reload haproxy on dbproxy1010 to repool labsdb1010 - https://phabricator.wikimedia.org/T174047

Change 431605 had a related patch set uploaded (by Bstorm; owner: Bstorm):
[operations/puppet@production] wiki replicas: Depool labsdb1011 for MCR table changes

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

Change 431605 merged by Marostegui:
[operations/puppet@production] wiki replicas: Depool labsdb1011 for MCR table changes

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

Mentioned in SAL (#wikimedia-operations) [2018-05-07T16:37:48Z] <marostegui> Reload haproxy on dbproxy1010 to depool labsdb1011 - https://phabricator.wikimedia.org/T174047

Mentioned in SAL (#wikimedia-operations) [2018-05-07T22:31:24Z] <bstorm_> labsdb1009,labsdb1010,labsdb1011 are now on up-to-date views per T174047

Mentioned in SAL (#wikimedia-operations) [2018-05-08T05:19:33Z] <marostegui> Reload haproxy on dbproxy1010 to repool labsdb1011 - https://phabricator.wikimedia.org/T174047

Bstorm added a comment.May 8 2018, 3:18 PM

This should be good to go until we are actually dropping columns. At that point, the views should change to not look at the old columns at all to construct the fake columns.

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

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

Dropping this off SDC boards. This is only needed when we stop writing the old schema (T198312), which is not a blocker for SDC in any way.

daniel renamed this task from Provide backwards compatibility views for toolforge replica [MCR] to Hide deprecated/unused fields on toolforge replica [MCR].Jul 11 2018, 12:17 PM
daniel updated the task description. (Show Details)

More important than this, could you help us document and draft a communication with a summary of MCR changes for cloud users, and I will coordinate with cloud team?

daniel updated the task description. (Show Details)Jul 11 2018, 12:27 PM

I changed this ticket to ask for the affected fields to be hidden / dropped from the view, instead of trying to provide backwards compatibility.

@jcrespo, @bd808 I have updated the task description with more details, and added a section with instructions for migrating tools. Do you think this is sufficient? Do you want me to send an email out, or will you do that?

jcrespo added a comment.EditedJul 11 2018, 12:38 PM

I can take care of sending an email or coordinating with cloud team, but I don't really know all the changes and/or roadmap. Could you point me to a summary of that and I can take care of the rest? E.g. "The following fields will no longer be updated. The following fileds /tables will appear. etc." Anything so we can update a FAQ on wikitech.org (better if it is just a link to mediawiki.org you already have).

Linking to tables.sql HEAD is not enough because fields are not dropped right away, and sometimes it is out of sync with HEAD due to them taking some time to be applied.

The summary is enough for the first phase, but I am going to guess it is just the beginning and next phases will need similar treatment.

@jcrespo All the info you requested should already be in the task description:

  • new tables are already there, will be populated when T183488 is done, which is due to start happening in August, and will probably take to the end of September to complete on all wikis. The completion of this will be marked by T198308: Enable MCR migration stage "write both, read new" on live systems. At this point, the old fields can and should soon be hidden from the replica views.
  • the old fields will stop being updated with T198312: Set the WMF cluster to use the new MCR-only schema, which is expected to happen some time in Q2. The query to use instead is given in the task description. At this point, the old fields must no longer be available in the replica views (this ticket).
  • the old fields will be dropped from the revision and page tables... whenever it suits you. The plan says Q3, but there is no rush.

Thank you, I will see how we can best communicate this.

Change 447654 had a related patch set uploaded (by Bstorm; owner: Bstorm):
[operations/puppet@production] wiki replicas: moving compatibility views to $table_compat

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

The patch I suggested above should be safe to apply right away, allowing maintainers to move to the _compat views, if they want to (maybe that could feature in the early stages of the communication plan), and when the columns need to be hidden, they can be hidden on the main views, but not the compat views (which will need updating to not break, of course).

This should boost performance (and could possibly be applied to other refactors) while allowing a possibly lower-friction way of preserving old code (though unmaintained tools would still just break).

@jcrespo I am prepared to push the patch I put up with the "_compat" tables to provide an easy fix for people with not-so-maintained but still important tools. I'm not sure I should NULL the fields on this ticket just yet?

However, if we haven't communicated the changes much yet, perhaps this should come after that? That or perhaps an announcement about the timetable and changes could include that there will be tables named "$table_compat" in some cases to use with older tools in case there isn't really anyone around to refactor them much more than that?

For that matter, will anything break if I deploy that now? Like is the compatibility joins in the current tables actually needed at this point?

Yeah, let's announce it first as this time is a "breaking change" and we can wait a bit to deploy. I have also pending to to a rolling restart for mariadb upgrade and we can do it at the same time as otherwise for large tables all changes may fail because of metadata locking.

BTW, if you think it is ok, we can also amend wikireplica documentation indicating that we try, but cannot guarantee backwards compatibility on db schema, and if people don't want to maintain the changes frequently, the API is the most stable method.

jcrespo added a comment.EditedSep 11 2018, 1:47 PM

will anything break

I don't think because fields have been added but not yet removed, but I cannot promise that won't happen soon (it is not in my hand, it is the mw core maintainers who handle that) or it is happening now (we should test on deploy on a smaller db first).

That sounds like a reasonable edit for the docs.

(we should test on deploy on a smaller db first).

Any suggestions? scwiki perhaps?
If it isn't breaking yet, then I could merge now, deploy to that db, and actually do a general rebuild of the views during the reboot depools.

I've written this: https://wikitech.wikimedia.org/wiki/Help:Toolforge/Database#Stability_of_the_mediawiki_database_schema please review if it makes sense and ping if you see issues or edit it yourself and it would be great if you could send an email referencing this to cloud-l.

If it isn't breaking

The views shouldn't be breaking, but I am not so sure about the tools using them...

the reboot depools.

That is not likely happening this week because of the dc switchover :-(

I don't think because fields have been added but not yet removed, but I cannot promise that won't happen soon (it is not in my hand, it is the mw core maintainers who handle that) or it is happening now (we should test on deploy on a smaller db first).

No fields are going to be removed in the foreseeable future (T198557). Some, like rev_text_id and rev_content_model, will no longer be written some, probably some time later this year (T198312). All new fields and tables have been populated and are being kept up to date, so tools can start to use the new schema immediately (I'll update the description to reflect that). The live cluster will start doing that as well in the next couple of weeks, see T198308: Enable MCR migration stage "write both, read new" on live systems.

In my mind, legacy joins are not needed if this is announced as a breaking change. The vast majority of tools probably doesn't care about rev_content_model.

@daniel look at the writeup I mention above, and see if it seems reasonable- note that tools may rely on old structure, in terms of fields may not be dropped yet, but I guess they will eventually be emptied. Note that the main issues we learned is that compatibility views created issues due to unoptimized queries- and there was no way to access the real underlying structure.

daniel updated the task description. (Show Details)Sep 11 2018, 2:08 PM
daniel added a comment.EditedSep 11 2018, 2:13 PM

@jcrespo sounds good to me, yes. Compare also https://www.wikidata.org/wiki/Wikidata:Stable_Interface_Policy, which states that changes to the database schema are announced like changes to a public API, but less effort is made to provide backwards compatibility as would be made for a stable interface.

@jcrespo, looks great! I think that, since the work is already done anyway, with the slow development possible on some tools, having some compatibility views available should be helpful. I'm eager to remove some of the compatibility joins from the replicas (which is part of what this patch does), but it can wait until after the datacenter switchover. I'll merge and deploy it to a smaller wiki so see how that goes. scwiki seems ok to me for now.

If nobody objects, I'll do that today. It will only affect new wikis going forward until I do the full rebuild.

jcrespo added a comment.EditedSep 11 2018, 2:21 PM

no issue with scwiki or any other small one- you may still need the depooling to avoid headaches on the larger ones :-) and I may be able to help with that as mentioned.

Change 447654 merged by Bstorm:
[operations/puppet@production] wiki replicas: moving compatibility views to $table_compat

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

Ok, scwiki now has the compatibility whatnot moved to separate tables. If that doesn't break a lot of tools for some reason by tomorrow, I probably should just go ahead and put up patches to depool servers to do the whole batch instead of waiting. Some tools are really suffering from the performance constraints.

Krinkle updated the task description. (Show Details)Sep 12 2018, 4:26 AM

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

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

Change 460005 merged by Marostegui:
[operations/puppet@production] wiki replicas: depool labsdb1010 for view updates

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

Mentioned in SAL (#wikimedia-operations) [2018-09-12T12:23:05Z] <marostegui> Reload haproxy on dbproxy1010 to depool labsdb1010 - https://phabricator.wikimedia.org/T174047

Mentioned in SAL (#wikimedia-operations) [2018-09-12T13:24:19Z] <marostegui> Reload haproxy on dbproxy1010 to repool labsdb1010 - T174047

Change 460016 had a related patch set uploaded (by Bstorm; owner: Bstorm):
[operations/puppet@production] wiki replicas: depool labsdb1009 to run view updates

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

Change 460016 merged by Marostegui:
[operations/puppet@production] wiki replicas: depool labsdb1009 to run view updates

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

Mentioned in SAL (#wikimedia-operations) [2018-09-13T09:24:54Z] <marostegui> Reload haproxy on dbproxy1011 to depool labsdb1009 - https://phabricator.wikimedia.org/T174047

Mentioned in SAL (#wikimedia-operations) [2018-09-13T12:45:32Z] <marostegui> Reload haproxy on dbproxy1011 to repool labsdb1009 - https://phabricator.wikimedia.org/T174047

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

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

Change 460362 merged by Marostegui:
[operations/puppet@production] wiki replicas: depool labsdb1011 to run view updates

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

Mentioned in SAL (#wikimedia-operations) [2018-09-13T14:36:32Z] <marostegui> Reload haproxy on dbproxy1010 to depool labsdb1011 - https://phabricator.wikimedia.org/T174047

Bstorm updated the task description. (Show Details)Feb 4 2019, 4:48 PM