Page MenuHomePhabricator

Apply updates for MCR, actor migration, and content migration, to production wikis.
Open, MediumPublic

Description

The following schema changes should be applied to the revision and archive table in production, at DBA's discretion:

  • Drop rev_text_id, rev_content_model, and rev_content_format that MCR obsoleted.
  • Drop ar_text_id, ar_content_model, and ar_content_format that MCR obsoleted.
  • Replace rev_comment with rev_comment_id.
  • Replace rev_user and rev_user_text with rev_actor, plus associated index changes.

Schema change progress:

  • s1
    • eqiad
    • codfw
  • s2
    • eqiad
    • codfw
  • s3
      • testwiki first
    • eqiad
    • codfw
  • s4
    • eqiad
    • codfw
  • s5
    • eqiad
    • codfw
  • s7
    • eqiad
    • codfw
  • s8
    • eqiad
    • codfw
  • Once the change is deployed, remove the columns from modules/role/files/mariadb/filtered_tables.txt to avoid having triggers on them

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Should I get a ticket open and get communicating with the community ASAP on dropping rev_text_id, rev_content_model, rev_content_format, ar_text_id, ar_content_model, and ar_content_format?

+1
I will also start the change on a s6 (frwiki, jawiki, ruwiki) codfw slave first, leave it for a few days, and then on an s6 eqiad slave and leave it for a few days as well to make sure we are good to go.

Also ping @ArielGlenn as this will be a massive schema change, pinging in case there's stuff that needs to change within the dumps world!

Should I get a ticket open and get communicating with the community ASAP on dropping rev_text_id, rev_content_model, rev_content_format, ar_text_id, ar_content_model, and ar_content_format?

+1
I will also start the change on a s6 (frwiki, jawiki, ruwiki) codfw slave first, leave it for a few days, and then on an s6 eqiad slave and leave it for a few days as well to make sure we are good to go.

Also ping @ArielGlenn as this will be a massive schema change, pinging in case there's stuff that needs to change within the dumps world!

Are you planning to apply all changes at the same time? In theory the code should just work but I'll have another close look at everything to make sure.

Should I get a ticket open and get communicating with the community ASAP on dropping rev_text_id, rev_content_model, rev_content_format, ar_text_id, ar_content_model, and ar_content_format?

+1
I will also start the change on a s6 (frwiki, jawiki, ruwiki) codfw slave first, leave it for a few days, and then on an s6 eqiad slave and leave it for a few days as well to make sure we are good to go.

Also ping @ArielGlenn as this will be a massive schema change, pinging in case there's stuff that needs to change within the dumps world!

Are you planning to apply all changes at the same time? In theory the code should just work but I'll have another close look at everything to make sure.

No, definitely not. I will do it very slowly, for s6, if there are no issues there I will move to s5.

daniel added a comment.Fri, May 8, 7:52 AM

Yes, sorry. I didn't notice that the merge failed when I updated this ticket. We'll get this resolved today, or next week at the latest.

daniel added a comment.EditedFri, May 8, 7:53 AM

Also ping @ArielGlenn as this will be a massive schema change, pinging in case there's stuff that needs to change within the dumps world!

Generation of XML dumps should not be affected, I worked on that with Ariel over the last months. I don't know about other stuff.

daniel added a comment.EditedFri, May 8, 7:56 AM

From my reading, we have no such emulation on the main views that people are really using. Content model/format fields are the same between archive and revision, so I'm not sure what you mean.

My bad, I just didn't see it.

We have joins against the slots framework (MCR?) on _compat views and no such joins on the views that bear the same names as the tables because it would be prohibitively slow. What joins are already in them for data scrubbing or because of _temp tables are a real problem for our systems and user community.

Excellent!

Since this task seems to be about dropping columns, we just need to drop the fields from the views on the replicas before they are dropped upstream of the replica servers to prevent the views from just throwing errors when queried.

This will drop some columns, but it will also add some (rev_comment_id and rev_actor) which should probably be exposed on labs.

For instance, we have if(rev_deleted&1,null,rev_text_id) as rev_text_id in the revision view.

I'm surprised rev_text_id is replicated at all. It's meaningless if it can't be used to load the actual data blob.

Yes, sorry. I didn't notice that the merge failed when I updated this ticket. We'll get this resolved today, or next week at the latest.

No worries at all! Thank you!

When these new fields rev_actor and rev_comment_id are added, what populates them?

daniel added a comment.EditedFri, May 8, 8:53 AM

When these new fields rev_actor and rev_comment_id are added, what populates them?

Nothing yet. They are unused for now. We are adding them now so we don't have to touch this table in prod again when we need them.

EDIT: Actually, I think I was mistaking here. I'll have to dig in more deeply to understand the current status of the actor/comment migration in production. This used to be Brad's domain.

One more question: are these changes going to be applied in deployment-prep first? Because I'd like to test there just to be extra sure.

One more question: are these changes going to be applied in deployment-prep first? Because I'd like to test there just to be extra sure.

Good point, yes. They should be. I'll update the task description to include all-labs.dblist.

daniel updated the task description. (Show Details)Fri, May 8, 10:16 AM
daniel updated the task description. (Show Details)
daniel updated the task description. (Show Details)

One more question: are these changes going to be applied in deployment-prep first? Because I'd like to test there just to be extra sure.

Good point, yes. They should be. I'll update the task description to include all-labs.dblist.

I am confused, are we talking about Beta (https://wikitech.wikimedia.org/wiki/Help:Labs_labs_labs#Beta_Cluster) or about wiki replicas? (https://wikitech.wikimedia.org/wiki/Portal:Data_Services#Wiki_Replicas)
DBAs don't own beta, so those alter tables will need to be applied by someone with grants for those database.
Wiki replicas will receive the schema changes normally (but we'll wait for @Bstorm to give us green light there per T238966#6118001)

Marostegui updated the task description. (Show Details)Fri, May 8, 11:51 AM

@daniel I have refactored a bit the task description to make it more similar to https://wikitech.wikimedia.org/wiki/Schema_changes#Workflow_of_a_schema_change which is a lot clearer for us DBAs.

Regarding your suggestion about applying the schema change on the order of:
group0
group1
etc

That's very hard for us to do, as we tend to do it per section, which means: s5, s6 entirely....rather than on a per wiki basis, which is a lot lot lot harder to manage and more error prone.

What we usually do is apply it on this order:
First s6, which has only 3 wikis (but with good amount of traffic so we can catch regressions: frwiki jawiki ruwiki) and then s5 which has more wikis and good amount of traffic too.
Then the rest of sections.

In this particular case, as it is such a critical one, I can apply it only on testwikiwiki first if that will make us more confident. Would that be enough?
I can do testwikiwiki first and then if we are good, I can proceed with s6 and the rest
How does that sound?

@daniel I have refactored a bit the task description to make it more similar to https://wikitech.wikimedia.org/wiki/Schema_changes#Workflow_of_a_schema_change which is a lot clearer for us DBAs.

Thanks! I tried to supply all the info requested there, but I guess I ended up making things confusing :)

I am confused, are we talking about Beta (https://wikitech.wikimedia.org/wiki/Help:Labs_labs_labs#Beta_Cluster) or about wiki replicas? (https://wikitech.wikimedia.org/wiki/Portal:Data_Services#Wiki_Replicas) DBAs don't own beta, so those alter tables will need to be applied by someone with grants for those database.

In the context of Ariel's question, we were talking about the Beta cluster. And... I also don't know who looks after the database there. I guess someone in the Release-Engineering-Team?

I can do testwikiwiki first and then if we are good, I can proceed with s6 and the rest
How does that sound?

Yes, sure, that works!

Thank you - once the change is merged I can start with testwiki and ping you here for more testing.
And yes, I believe Release-Engineering-Team owns it as far as I know. Not sure if @Reedy also can apply it.

Reedy added a comment.Fri, May 8, 12:17 PM

Thank you - once the change is merged I can start with testwiki and ping you here for more testing.
And yes, I believe Release-Engineering-Team owns it as far as I know. Not sure if @Reedy also can apply it.

If the schema change patches have been merged, along with the mysqlupdater patches in MW, beta cluster should be upto date already, as that does run update.php

I can do manual stuff if I breaks though

Thank you - once the change is merged I can start with testwiki and ping you here for more testing.
And yes, I believe Release-Engineering-Team owns it as far as I know. Not sure if @Reedy also can apply it.

If the schema change patches have been merged, along with the mysqlupdater patches in MW, beta cluster should be upto date already, as that does run update.php

Or, right. Obviously.

So this means that beta will be up to date as soon as the updater patch merges. We hit a small snag there, but I think this will happen later today. @Reedy you can speed things up be reviewing https://gerrit.wikimedia.org/r/c/mediawiki/core/+/595139 :)

Pchelolo added a subscriber: Pchelolo.EditedFri, May 8, 3:44 PM

So this means that beta will be up to date as soon as the updater patch merges. We hit a small snag there, but I think this will happen later today. @Reedy you can speed things up be reviewing https://gerrit.wikimedia.org/r/c/mediawiki/core/+/595139 :)

And it has been merged after a few more bumps on the road.

So this means that beta will be up to date as soon as the updater patch merges. We hit a small snag there, but I think this will happen later today. @Reedy you can speed things up be reviewing https://gerrit.wikimedia.org/r/c/mediawiki/core/+/595139 :)

And it has been merged after a few more bumps on the road.

\o/ yay!
So I will start with testwiki database next week, ping you guys in case you want to do some testing, leave it for a few days and start with s6.

Bstorm added a comment.Fri, May 8, 4:00 PM

@daniel I've got this definition in the views now for rev_comment_id: if(rev_deleted&2,0,coalesce(revcomment_comment_id,0)) as rev_comment_id and for rev_actor: if(rev_deleted&4,0,coalesce(revactor_actor,0)) as rev_actor. Those are referencing revision_comment_temp and revision_actor_temp, which quite possibly are being populated to my knowledge?

I am eagerly awaiting when the join to those temp tables can go away because it causes much higher load on the replica servers and frustration for our users because it plays havoc on the indexes. Is there a change in store for that setup?

@daniel I've got this definition in the views now for rev_comment_id: if(rev_deleted&2,0,coalesce(revcomment_comment_id,0)) as rev_comment_id and for rev_actor: if(rev_deleted&4,0,coalesce(revactor_actor,0)) as rev_actor. Those are referencing revision_comment_temp and revision_actor_temp, which quite possibly are being populated to my knowledge?

revision_comment_temp and revision_actor_temp are still in use, yes.

I am eagerly awaiting when the join to those temp tables can go away because it causes much higher load on the replica servers and frustration for our users because it plays havoc on the indexes. Is there a change in store for that setup?

Not yet. Progress on this has been thrown back by Brad's departure. I'm trying to get it on the roadmap, but getting cleanup prioritized is always tricky. You can help with that by flagging this as problematic to your team, perhaps bring it up to @MNadrofsky.

Mentioned in SAL (#wikimedia-operations) [2020-05-19T11:12:17Z] <marostegui> Deploy schema change on db2124 (frwiki, jawiki, ruwiki) T238966

Marostegui moved this task from Next to In progress on the DBA board.Tue, May 19, 1:36 PM

I have done the first alter on s6, on db2124 (frwiki, jawiki and ruwiki).
Will leave it running till tomorrow, to make sure replication doesn't get broken (which would mean we still have inserts there).

If nothing breaks, I will be altering eqiad (which gets production reads) for testwiki as agreed.

Marostegui updated the task description. (Show Details)Tue, May 19, 1:40 PM
Marostegui updated the task description. (Show Details)

s6 progress

  • labsdb1012
  • labsdb1011
  • labsdb1010
  • labsdb1009
  • dbstore1005
  • db2129
  • db2124
  • db2117
  • db2114
  • db2097
  • db2095
  • db2089
  • db2087
  • db2076
  • db1139
  • db1131
  • db1125
  • db1113
  • db1098
  • db1096
  • db1093
  • db1088
  • db1085

We need to make sure that any of the above indexes that will be dropped are being FORCED somewhere.

@daniel I won't proceed with testwiki till the FORCE is removed in code, just in case this generate more noise as testwiki is more used.

Just so we don't pass on an opportunity here: should the rev_actor_timestamp index be changed to include the revision ID as well (and become unique)? This is a question to the database folks. Given that the timestamp is already highly selective, would it be beneficial to the query to make the index unique? Or would the extra bytes in the index do more harm than good?

Background: there is a desire to improve the pagination logic for user contributions queries, see T200259: ContribsPager should use a unique criterion for paging. We currently select by rev_actor (emulated via revision_actor_temp) and range on rev_timestamp (denormalized to revision_actor_timestamp), ordering by timestamp. This however results in ambiguous paging, causing revisions to be skipped when iterating over contributions. To fix this, we would have to select by rev_actor (emulated via revision_actor_temp), a range on rev_timestamp (denormalized to revision_actor_timestamp), and a range on rev_id (denormalized to revision_actor_revision), ordering by timestamp and revision id.

Can we do this without modifying the index? Also, can we do it with the temporary tables still in place, or does it have to wait for T215466: Remove revision_comment_temp and revision_actor_temp?

The reason I bring this up here is this: if we need to include the revision ID the index in order to allow unambiguous pagination for contributions, we should do this now, when creating the rev_actor_timestamp index, rather than change it later.

Just so we don't pass on an opportunity here: should the rev_actor_timestamp index be changed to include the revision ID as well (and become unique)? This is a question to the database folks. Given that the timestamp is already highly selective, would it be beneficial to the query to make the index unique? Or would the extra bytes in the index do more harm than good?

You mean adding rev_id to rev_actor_timestamp (rev_actor,rev_timestamp)?
If you can give me an existing query we can check what's the efficiency of the current index vs the index with rev_id on enwiki for instance.

Can we do this without modifying the index? Also, can we do it with the temporary tables still in place, or does it have to wait for T215466: Remove revision_comment_temp and revision_actor_temp?

That's probably something Core Platform Team know better than us, I have no input there really.

daniel added a comment.EditedFri, May 29, 12:36 PM

You mean adding rev_id to rev_actor_timestamp (rev_actor,rev_timestamp)?
If you can give me an existing query we can check what's the efficiency of the current index vs the index with rev_id on enwiki for instance.

Yes, that's what I mean.
See T200259#6176088 for the current query (only using the timestamp), and T200259#6176100 for the proposed query (using timestamp and id together).

Note that the query plans I gave there are for the current vanilla schema, which already has the changes for MCR, actor and comments applied to the revision table. The question is which version of the new index would be better for the proposed - with rev_id, or without rev_id?

Can we do this without modifying the index? Also, can we do it with the temporary tables still in place, or does it have to wait for T215466: Remove revision_comment_temp and revision_actor_temp?

That's probably something Core Platform Team know better than us, I have no input there really.

Well, the answer to this question depends entirely on db performance considerations. If it's all the same to you, I would say we a) change the index to include rev_id, and b) fix the paging to use rev_id as well. We won't benefit from the new index until we no longer use the temporary tables, but I'd just live with that. My question to you is whether we actually *can* live with that, and if yes, whether the smaller index (as defined now, without rev_id) would actually be preferable for some reason.

Thanks - I will try both queries on a host with the new index next week.
At first, on enwiki it doesn't give any results, but it does in Commonswiki and wikidata.
Both queries with the current table structure run pretty much instantly (0.02) seconds even on a "cold" host in codfw, so that's a good start - even if it scans around 10M rows.

I will modify the index there next week and see what the optimizer does and come back with the results.

By the way, all usages of the old indexes have been removed from live code, per T253289. Mentions in documentation still need to be cleaned up.

daniel added a comment.EditedFri, May 29, 12:57 PM

Thanks - I will try both queries on a host with the new index next week.
At first, on enwiki it doesn't give any results, but it does in Commonswiki and wikidata.
Both queries with the current table structure run pretty much instantly (0.02) seconds even on a "cold" host in codfw, so that's a good start - even if it scans around 10M rows.

The example query has the user ID set to 2, and a date from today. I'd be surprised if that yielded any results on a live system. To get results from production, you'd have to modify the timestamps and actor ID in the conditional.

I will modify the index there next week and see what the optimizer does and come back with the results.

Ok. To me, this sounds like we can use the new pagination strategy with the existing index definition. I just want to make extra sure that we don't trigger this problem again when we do that: T221380: Wikidata: Special:Contributions times out for many high-activity users. This is also the query that is involved in this issue: T234450: Some Special:Contributions requests cause "Error: 0" from database or WMFTimeoutException. Can you perhaps try the query for a user with a lot of edits, with a high limit?

Excellent!
To recap on the status:

  • @Marostegui to check if rev_id is worth to be added to the rev_actor_timestamp timestamp: T238966#6176187
  • @Marostegui to alter testwiki on s3 so @daniel and team can double check if all goes work - this needs to avoid being done on db1112 as that host is the master for labs hosts and it is blocked on @Bstorm
  • @Bstorm to complete the work on the wikireplicas' views: T238966#6118001 (this blocks the deployment of this schema change on eqiad).

Heh - you are too qick, I just edited my comment with additional info ;)

I just want to make extra sure that we don't trigger this problem again when we do that: T221380: Wikidata: Special:Contributions times out for many high-activity users. This is also the query that is involved in this issue: T234450: Some Special:Contributions requests cause "Error: 0" from database or WMFTimeoutException. Can you perhaps try the query for a user with a lot of edits, with a high limit?

Sure, if you can provide me the exact query, that will speed up things :)

Will try to dig that up later today.

Thank you - much appreciated

Mentioned in SAL (#wikimedia-operations) [2020-06-01T05:03:55Z] <marostegui@cumin1001> dbctl commit (dc=all): 'Depool enwiki db2071 slave to test new index - T238966', diff saved to https://phabricator.wikimedia.org/P11338 and previous config saved to /var/cache/conftool/dbconfig/20200601-050354-marostegui.json

I am altering db2071 (enwiki) with this schema change and also adding rev_id to the rev_actor_timestamp index for testing: T238966#6176840

Marostegui updated the task description. (Show Details)Mon, Jun 1, 5:08 AM
daniel added a comment.EditedMon, Jun 1, 2:06 PM

I constructed an example for a user with many edits: QEDKbot, user_id 17963495, actor_id 165644.

Here is the query without rev_id (current, broken pagination) to be run against enwiki:

SELECT  rev_id,rev_page,rev_timestamp,rev_minor_edit,rev_deleted,rev_len,rev_parent_id,rev_sha1,
  comment_rev_comment.comment_text AS `rev_comment_text`,comment_rev_comment.comment_data AS `rev_comment_data`,comment_rev_comment.comment_id AS `rev_comment_cid`,
  actor_rev_user.actor_user AS `rev_user`,actor_rev_user.actor_name AS `rev_user_text`,temp_rev_user.revactor_actor AS `rev_actor`,
  page_namespace,page_title,page_id,page_latest,page_is_redirect,page_len,
  user_name,page_is_new,revactor_timestamp,
  ( SELECT  GROUP_CONCAT(ctd_name SEPARATOR ',')
    FROM `change_tag`
    JOIN `change_tag_def` ON ((ct_tag_id=ctd_id))
    WHERE ct_rev_id=rev_id  ) AS `ts_tags`
FROM `revision`
JOIN `revision_comment_temp` `temp_rev_comment` ON ((temp_rev_comment.revcomment_rev = rev_id))
JOIN `comment` `comment_rev_comment` ON ((comment_rev_comment.comment_id = temp_rev_comment.revcomment_comment_id))
JOIN `revision_actor_temp` `temp_rev_user` FORCE INDEX (actor_timestamp) ON ((temp_rev_user.revactor_rev = rev_id))
JOIN `actor` `actor_rev_user` ON ((actor_rev_user.actor_id = temp_rev_user.revactor_actor))
JOIN `page` ON ((page_id = rev_page))
LEFT JOIN `user` ON ((actor_rev_user.actor_user != 0) AND (user_id = actor_rev_user.actor_user))   
WHERE ((temp_rev_user.revactor_actor = 165644))
  AND ((rev_deleted & 4) = 0)
  AND (((revactor_timestamp<'20200601133558')))
ORDER BY revactor_timestamp DESC,revactor_rev DESC
LIMIT 500

Here's the fixed query:

SELECT  rev_id,rev_page,rev_timestamp,rev_minor_edit,rev_deleted,rev_len,rev_parent_id,rev_sha1,
  comment_rev_comment.comment_text AS `rev_comment_text`,comment_rev_comment.comment_data AS `rev_comment_data`,comment_rev_comment.comment_id AS `rev_comment_cid`,
  actor_rev_user.actor_user AS `rev_user`,actor_rev_user.actor_name AS `rev_user_text`,temp_rev_user.revactor_actor AS `rev_actor`,
  page_namespace,page_title,page_id,page_latest,page_is_redirect,page_len,
  user_name,page_is_new,revactor_timestamp,revactor_rev,
  ( SELECT  GROUP_CONCAT(ctd_name SEPARATOR ',')
    FROM `change_tag`
    JOIN `change_tag_def`
    ON ((ct_tag_id=ctd_id))
    WHERE ct_rev_id=rev_id  ) AS `ts_tags`
FROM `revision`
JOIN `revision_comment_temp` `temp_rev_comment` ON ((temp_rev_comment.revcomment_rev = rev_id))
JOIN `comment` `comment_rev_comment` ON ((comment_rev_comment.comment_id = temp_rev_comment.revcomment_comment_id))
JOIN `revision_actor_temp` `temp_rev_user` FORCE INDEX (actor_timestamp) ON ((temp_rev_user.revactor_rev = rev_id))
JOIN `actor` `actor_rev_user` ON ((actor_rev_user.actor_id = temp_rev_user.revactor_actor))
JOIN `page` ON ((page_id = rev_page))
LEFT JOIN `user` ON ((actor_rev_user.actor_user != 0) AND (user_id = actor_rev_user.actor_user))   
WHERE ((temp_rev_user.revactor_actor = 165644))
  AND ((rev_deleted & 4) = 0)
  AND (((revactor_timestamp<'20200601133558'))
    OR ((revactor_timestamp='20200601133558') AND (revactor_rev<'960168318')))
ORDER BY revactor_timestamp DESC,revactor_rev DESC,revactor_rev
DESC LIMIT 500

It would be interesting to know how they both fare.

Note however that we are still using the temporary association in revision_actor_temp (in production, and in this query). The thing that we really want to know is whether we want to change the definition of the new rev_actor_timestamp index on the revision table, but all we can test right now is the behavior of the existing actor_timestamp index on the revision_actor_temp table. So that index would have to have revactor_rev added for testing.

EDIT: both queries return in 0.02sec for me on db1106 (a vslow host with little traffic).

I constructed an example for a user with many edits: QEDKbot, user_id 17963495, actor_id 165644.

Thank you, I will post the results once I've got the revision table altered, it's been running for 8h now :-)

Note however that we are still using the temporary association in revision_actor_temp (in production, and in this query). The thing that we really want to know is whether we want to change the definition of the new rev_actor_timestamp index on the revision table, but all we can test right now is the behavior of the existing actor_timestamp index on the revision_actor_temp table. So that index would have to have revactor_rev added for testing.

This is getting very confusing and the task is getting a massive amount of different data and discussions and I am worried all this may lead to even more confusion in knowing what to alter and what to test.

To be clear what I am doing now:

Is there anything else you want to test?

daniel added a comment.Mon, Jun 1, 2:36 PM

This is getting very confusing and the task is getting a massive amount of different data and discussions and I am worried all this may lead to even more confusion in knowing what to alter and what to test.

I agree, and I'm sorry to add complexity. Would it be better to move the question of the index change to a sub-task?

To clarify what I wrote above:
Right now, I'm trying to understand whether we can make pagination for user contributions be based on timestamp+id instead of just timestamp. The index that is used for this right now (I believe) is the actor_timestamp index on the revision_actor_temp table. This will continue to be the case until we have populated the new rev_actor field and removed the revision_actor_temp table, per T215466. So the question relevant for operation right now is whether adding this field to the query causes performance issues, and if yes, whether changing the index to include the revision id (revactor_rev) would fix that.

The connection to this task here is that IF revactor_rev needs to be added to the actor_timestamp index on the revision_actor_temp table, this implies that the new rev_actor_timestamp index on the revision table also needs to change to include the rev_id field. However, we can't test the behavior of the rev_actor_timestamp index itself yet, because we can't use rev_actor in queries, since it has not yet been populated (you are just about to create that field).

Is there anything else you want to test?

If the new contributions query runs fine with the existing actor_timestamp index on revision_actor_temp, then we can just continue with the schema changes as planned (unless you anticipate trouble from that query in the future). If you do anticipate such trouble, or if the query performance is bad already, then we should test whether that can be fixed by adding revactor_rev to the actor_timestamp index on revision_actor_temp. If it does, we should change the definition of the corresponding index in the new schema, rev_actor_timestamp on revision, to include rev_id.

Marostegui added a comment.EditedMon, Jun 1, 2:54 PM

I have sync'ed up with @daniel on IRC to make sure we were on the same page, so the idea is:

I have sync'ed up with @daniel on IRC to make sure we were on the same page, so the idea is:

The schema changes finished and this is how revision looks like:

CREATE TABLE `revision` (
  `rev_id` int(8) unsigned NOT NULL AUTO_INCREMENT,
  `rev_page` int(8) unsigned NOT NULL DEFAULT '0',
  `rev_comment_id` bigint(20) unsigned NOT NULL DEFAULT '0',
  `rev_actor` bigint(20) unsigned NOT NULL DEFAULT '0',
  `rev_timestamp` varbinary(14) NOT NULL DEFAULT '',
  `rev_minor_edit` tinyint(1) unsigned NOT NULL DEFAULT '0',
  `rev_deleted` tinyint(1) unsigned NOT NULL DEFAULT '0',
  `rev_len` int(8) unsigned DEFAULT NULL,
  `rev_parent_id` int(8) unsigned DEFAULT NULL,
  `rev_sha1` varbinary(32) NOT NULL DEFAULT '',
  PRIMARY KEY (`rev_id`),
  KEY `rev_timestamp` (`rev_timestamp`),
  KEY `page_timestamp` (`rev_page`,`rev_timestamp`),
  KEY `rev_page_id` (`rev_page`,`rev_id`),
  KEY `rev_page_actor_timestamp` (`rev_page`,`rev_actor`,`rev_timestamp`),
  KEY `rev_actor_timestamp` (`rev_actor`,`rev_timestamp`)
) ENGINE=InnoDB AUTO_INCREMENT=960296570 DEFAULT CHARSET=binary

And this is how revision_actor_temp looks like:

CREATE TABLE `revision_actor_temp` (
  `revactor_rev` int(10) unsigned NOT NULL,
  `revactor_actor` bigint(20) unsigned NOT NULL,
  `revactor_timestamp` binary(14) NOT NULL DEFAULT '\0\0\0\0\0\0\0\0\0\0\0\0\0\0',
  `revactor_page` int(10) unsigned NOT NULL,
  PRIMARY KEY (`revactor_rev`,`revactor_actor`),
  UNIQUE KEY `revactor_rev` (`revactor_rev`),
  UNIQUE KEY `revactor_actor` (`revactor_actor`,`revactor_timestamp`,`revactor_rev`),
  KEY `page_actor_timestamp` (`revactor_page`,`revactor_actor`,`revactor_timestamp`)
) ENGINE=InnoDB DEFAULT CHARSET=binary

I have tried both queries on db2071 (altered host) and db2072 (non touched host).

The query:

SELECT  rev_id,rev_page,rev_timestamp,rev_minor_edit,rev_deleted,rev_len,rev_parent_id,rev_sha1,
  comment_rev_comment.comment_text AS `rev_comment_text`,comment_rev_comment.comment_data AS `rev_comment_data`,comment_rev_comment.comment_id AS `rev_comment_cid`,
  actor_rev_user.actor_user AS `rev_user`,actor_rev_user.actor_name AS `rev_user_text`,temp_rev_user.revactor_actor AS `rev_actor`,
  page_namespace,page_title,page_id,page_latest,page_is_redirect,page_len,
  user_name,page_is_new,revactor_timestamp,
  ( SELECT  GROUP_CONCAT(ctd_name SEPARATOR ',')
    FROM `change_tag`
    JOIN `change_tag_def` ON ((ct_tag_id=ctd_id))
    WHERE ct_rev_id=rev_id  ) AS `ts_tags`
FROM `revision`
JOIN `revision_comment_temp` `temp_rev_comment` ON ((temp_rev_comment.revcomment_rev = rev_id))
JOIN `comment` `comment_rev_comment` ON ((comment_rev_comment.comment_id = temp_rev_comment.revcomment_comment_id))
JOIN `revision_actor_temp` `temp_rev_user` FORCE INDEX (actor_timestamp) ON ((temp_rev_user.revactor_rev = rev_id))
JOIN `actor` `actor_rev_user` ON ((actor_rev_user.actor_id = temp_rev_user.revactor_actor))
JOIN `page` ON ((page_id = rev_page))
LEFT JOIN `user` ON ((actor_rev_user.actor_user != 0) AND (user_id = actor_rev_user.actor_user))   
WHERE ((temp_rev_user.revactor_actor = 165644))
  AND ((rev_deleted & 4) = 0)
  AND (((revactor_timestamp<'20200601133558')))
ORDER BY revactor_timestamp DESC,revactor_rev DESC
LIMIT 500

(Without the FORCE INDEX runs fast on the altered host, and picks the new index by default (revactor_actor).)
There is no significant change on the query time on both hosts (same for the query plan) - they both run in 0.03-0.04 or so.

Regarding the second query:

SELECT  rev_id,rev_page,rev_timestamp,rev_minor_edit,rev_deleted,rev_len,rev_parent_id,rev_sha1,
  comment_rev_comment.comment_text AS `rev_comment_text`,comment_rev_comment.comment_data AS `rev_comment_data`,comment_rev_comment.comment_id AS `rev_comment_cid`,
  actor_rev_user.actor_user AS `rev_user`,actor_rev_user.actor_name AS `rev_user_text`,temp_rev_user.revactor_actor AS `rev_actor`,
  page_namespace,page_title,page_id,page_latest,page_is_redirect,page_len,
  user_name,page_is_new,revactor_timestamp,revactor_rev,
  ( SELECT  GROUP_CONCAT(ctd_name SEPARATOR ',')
    FROM `change_tag`
    JOIN `change_tag_def`
    ON ((ct_tag_id=ctd_id))
    WHERE ct_rev_id=rev_id  ) AS `ts_tags`
FROM `revision`
JOIN `revision_comment_temp` `temp_rev_comment` ON ((temp_rev_comment.revcomment_rev = rev_id))
JOIN `comment` `comment_rev_comment` ON ((comment_rev_comment.comment_id = temp_rev_comment.revcomment_comment_id))
JOIN `revision_actor_temp` `temp_rev_user` FORCE INDEX (actor_timestamp) ON ((temp_rev_user.revactor_rev = rev_id))
JOIN `actor` `actor_rev_user` ON ((actor_rev_user.actor_id = temp_rev_user.revactor_actor))
JOIN `page` ON ((page_id = rev_page))
LEFT JOIN `user` ON ((actor_rev_user.actor_user != 0) AND (user_id = actor_rev_user.actor_user))   
WHERE ((temp_rev_user.revactor_actor = 165644))
  AND ((rev_deleted & 4) = 0)
  AND (((revactor_timestamp<'20200601133558'))
    OR ((revactor_timestamp='20200601133558') AND (revactor_rev<'960168318')))
ORDER BY revactor_timestamp DESC,revactor_rev DESC,revactor_rev
DESC LIMIT 500

They both run fine on the altered host, however, we'd need to remove the FORCE INDEX from the code before deploying this schema change, as the actor_timestamp timestamp index would no longer exist and the query would fail.
Without the FORCE, both hosts (altered and non altered) pick the right index, so it shouldn't be a problem.

I have also run that query without the FORCE across all enwiki slaves and they all pick actor_timestamp index, so there is no need to do not need FORCE index anymore while we do the transition from that index to the new UNIQUE index:

labsdb1012.eqiad.wmnet:3306
1	PRIMARY	temp_rev_user	range	PRIMARY,revactor_rev,actor_timestamp	actor_timestamp	26	NULL	115735	Using where; Using index
labsdb1011.eqiad.wmnet:3306
1	PRIMARY	temp_rev_user	range	PRIMARY,revactor_rev,actor_timestamp	actor_timestamp	26	NULL	52313	Using where; Using index
labsdb1010.eqiad.wmnet:3306
1	PRIMARY	temp_rev_user	range	PRIMARY,revactor_rev,actor_timestamp	actor_timestamp	26	NULL	120849	Using where; Using index
labsdb1009.eqiad.wmnet:3306
1	PRIMARY	temp_rev_user	range	PRIMARY,revactor_rev,actor_timestamp	actor_timestamp	26	NULL	121005	Using where; Using index
dbstore1003.eqiad.wmnet:3311
1	PRIMARY	temp_rev_user	range	PRIMARY,revactor_rev,actor_timestamp	actor_timestamp	26	NULL	110611	Using where; Using index
db2130.codfw.wmnet:3306
1	PRIMARY	temp_rev_user	range	PRIMARY,revactor_rev,actor_timestamp	actor_timestamp	26	NULL	111463	Using where; Using index
db2116.codfw.wmnet:3306
1	PRIMARY	temp_rev_user	range	PRIMARY,revactor_rev,actor_timestamp	actor_timestamp	26	NULL	111463	Using where; Using index
db2112.codfw.wmnet:3306
1	PRIMARY	temp_rev_user	range	PRIMARY,revactor_rev,actor_timestamp	actor_timestamp	26	NULL	111463	Using where; Using index
db2103.codfw.wmnet:3306
1	PRIMARY	temp_rev_user	range	PRIMARY,revactor_rev,actor_timestamp	actor_timestamp	26	NULL	96233	Using where; Using index
db2097.codfw.wmnet:3311
1	PRIMARY	temp_rev_user	range	PRIMARY,revactor_rev,actor_timestamp	actor_timestamp	26	NULL	111463	Using where; Using index
db2094.codfw.wmnet:3311
1	PRIMARY	temp_rev_user	range	PRIMARY,revactor_rev,actor_timestamp	actor_timestamp	26	NULL	110579	Using where; Using index
db2092.codfw.wmnet:3306
1	PRIMARY	temp_rev_user	range	PRIMARY,revactor_rev,actor_timestamp	actor_timestamp	26	NULL	111445	Using where; Using index
db2088.codfw.wmnet:3311
1	PRIMARY	temp_rev_user	range	PRIMARY,revactor_rev,actor_timestamp	actor_timestamp	26	NULL	117927	Using where; Using index
db2085.codfw.wmnet:3311
1	PRIMARY	temp_rev_user	range	PRIMARY,revactor_rev,actor_timestamp	actor_timestamp	26	NULL	118703	Using where; Using index
db2072.codfw.wmnet:3306
1	PRIMARY	temp_rev_user	range	PRIMARY,revactor_rev,actor_timestamp	actor_timestamp	26	NULL	111445	Using where; Using index
db2071.codfw.wmnet:3306
1	PRIMARY	temp_rev_user	range	PRIMARY,revactor_rev,revactor_actor	revactor_actor	26	NULL	108803	Using where; Using index
db1141.eqiad.wmnet:3306
ERROR 2003 (HY000): Can't connect to MySQL server on 'db1141.eqiad.wmnet' (111 "Connection refused")
db1139.eqiad.wmnet:3311
1	PRIMARY	temp_rev_user	range	PRIMARY,revactor_rev,actor_timestamp	actor_timestamp	26	NULL	121185	Using where; Using index
db1134.eqiad.wmnet:3306
1	PRIMARY	temp_rev_user	range	PRIMARY,revactor_rev,actor_timestamp	actor_timestamp	26	NULL	112449	Using where; Using index
db1124.eqiad.wmnet:3311
1	PRIMARY	temp_rev_user	range	PRIMARY,revactor_rev,actor_timestamp	actor_timestamp	26	NULL	110579	Using where; Using index
db1119.eqiad.wmnet:3306
1	PRIMARY	temp_rev_user	range	PRIMARY,revactor_rev,actor_timestamp	actor_timestamp	26	NULL	112449	Using where; Using index
db1118.eqiad.wmnet:3306
1	PRIMARY	temp_rev_user	range	PRIMARY,revactor_rev,actor_timestamp	actor_timestamp	26	NULL	111445	Using where; Using index
db1107.eqiad.wmnet:3306
1	PRIMARY	temp_rev_user	range	PRIMARY,revactor_rev,actor_timestamp	actor_timestamp	26	NULL	111597	Using where; Using index
db1106.eqiad.wmnet:3306
1	PRIMARY	temp_rev_user	range	PRIMARY,revactor_rev,actor_timestamp	actor_timestamp	26	NULL	112113	Using where; Using index
db1105.eqiad.wmnet:3311
1	PRIMARY	temp_rev_user	range	PRIMARY,revactor_rev,actor_timestamp	actor_timestamp	26	NULL	117859	Using where; Using index
db1099.eqiad.wmnet:3311
1	PRIMARY	temp_rev_user	range	PRIMARY,revactor_rev,actor_timestamp	actor_timestamp	26	NULL	119729	Using where; Using index
db1089.eqiad.wmnet:3306
1	PRIMARY	temp_rev_user	range	PRIMARY,revactor_rev,actor_timestamp	actor_timestamp	26	NULL	112113	Using where; Using index
db1083.eqiad.wmnet:3306
1	PRIMARY	temp_rev_user	range	PRIMARY,revactor_rev,actor_timestamp	actor_timestamp	26	NULL	112449	Using where; Using index
db1080.eqiad.wmnet:3306
1	PRIMARY	temp_rev_user	range	PRIMARY,revactor_rev,actor_timestamp	actor_timestamp	26	NULL	111575	Using where; Using index

@daniel any other query you suspect that might change with this index being changed? If not, probably we should proceed to create the index as UNIQUE with revactor_rev included, as altering this table will take around 2h per host so it will be a bit painful to do this on a separate change in a few weeks/months if needed.

If you are good with these changes, I will revert all the changes on db2071 (to leave it consistent with the rest of the servers on that section) and once the patch for the UNIQUE creation is merged and the FORCE INDEX is removed from code (please check if that index is being forced somewhere else) I will resume with the original plan described at: T238966#6148541

daniel added a comment.EditedTue, Jun 2, 12:07 PM

Thanks for testing this!

They both run fine on the altered host, however, we'd need to remove the FORCE INDEX from the code before deploying this schema change, as the actor_timestamp timestamp index would no longer exist and the query would fail.

As far as I can see, the actor_timestamp index would continue to exist until we get rid of the revision_actor_temp table some time in the future. This patch only drops user_timestamp. Am I missing something?

I'm all for dropping FORCE INDEX wherever we can. However, note the comment on the patch that introduced it for this query: "For now at least this will avoid a filesort for some cases.". The change was prompted by this bug report: T189026

@daniel any other query you suspect that might change with this index being changed?

Well, there is the query that was reported as timing out in T189026. I have adjusted the subquery for ts_tags to match the current schema:

SELECT  rev_id,rev_page,rev_text_id,rev_timestamp,rev_minor_edit,rev_deleted,rev_len,rev_parent_id,rev_sha1,COALESCE( comment_rev_comment.comment_text, rev_comment ) AS `rev_comment_text`,comment_rev_comment.comment_data AS `rev_comment_data`,comment_rev_comment.comment_id AS `rev_comment_cid`,rev_user,rev_user_text,NULL AS `rev_actor`,rev_content_format,rev_content_model,page_namespace,page_title,page_id,page_latest,page_is_redirect,page_len,user_name,page_is_new,
( SELECT  GROUP_CONCAT(ctd_name SEPARATOR ',')
    FROM `change_tag`
    JOIN `change_tag_def`
    ON ((ct_tag_id=ctd_id))
    WHERE ct_rev_id=rev_id  ) AS `ts_tags`,ores_damaging_cls.oresc_probability AS `ores_damaging_score`,0.412 AS `ores_damaging_threshold`
FROM `revision`
LEFT JOIN `revision_comment_temp` `temp_rev_comment` ON ((temp_rev_comment.revcomment_rev = rev_id))
LEFT JOIN `comment` `comment_rev_comment` ON ((comment_rev_comment.comment_id = temp_rev_comment.revcomment_comment_id))
INNER JOIN `page` ON ((page_id = rev_page))
LEFT JOIN `user` ON ((rev_user != 0) AND (user_id = rev_user))
LEFT JOIN `ores_classification` `ores_damaging_cls` ON (ores_damaging_cls.oresc_model = '9' AND (ores_damaging_cls.oresc_rev=rev_id) AND ores_damaging_cls.oresc_class = '1')
WHERE ((rev_user = '2752938')) AND (rev_parent_id = 0) AND (page_namespace = '0') AND ((rev_deleted & 4) = 0)  ORDER BY rev_timestamp DESC LIMIT 51

I tried running it against wikidatawiki on db1087, and it comes back in 0.04 seconds. That seems fine, but something about it apparently caused problems in the past.

I'm not sure we'd be seeing exactly this query in production any more. Things have probably changed in some way since then. The query comes from the same code that also produces the other example queries, namely ContribsPager. The logic for composing the query depends on many parameters though. We can't really anticipate the behavior of all possible combinations, especially not with varying data distribution on different wikis. Wikidata doesn't have that many active users, but some bots with very very high edit counts and frequency.

If not, probably we should proceed to create the index as UNIQUE with revactor_rev included, as altering this table will take around 2h per host so it will be a bit painful to do this on a separate change in a few weeks/months if needed.

On the revision table, the field would be rev_id, so we'd have: ADD UNIQUE INDEX /*i*/rev_actor_timestamp (rev_actor,rev_timestamp,rev_id).

Change 601713 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] make rev_actor_timestamp index unique.

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

Thanks for testing this!

They both run fine on the altered host, however, we'd need to remove the FORCE INDEX from the code before deploying this schema change, as the actor_timestamp timestamp index would no longer exist and the query would fail.

As far as I can see, the actor_timestamp index would continue to exist until we get rid of the revision_actor_temp table some time in the future. This patch only drops user_timestamp. Am I missing something?

The change at T238966#6181873 modifies actor_timestamp which unless given the same name, will fail with that query FORCING. If that was just a test and we are not going to modify it on production, then nothing is needed.

I'm all for dropping FORCE INDEX wherever we can. However, note the comment on the patch that introduced it for this query: "For now at least this will avoid a filesort for some cases.". The change was prompted by this bug report: T189026

@daniel any other query you suspect that might change with this index being changed?

Well, there is the query that was reported as timing out in T189026. I have adjusted the subquery for ts_tags to match the current schema:

SELECT  rev_id,rev_page,rev_text_id,rev_timestamp,rev_minor_edit,rev_deleted,rev_len,rev_parent_id,rev_sha1,COALESCE( comment_rev_comment.comment_text, rev_comment ) AS `rev_comment_text`,comment_rev_comment.comment_data AS `rev_comment_data`,comment_rev_comment.comment_id AS `rev_comment_cid`,rev_user,rev_user_text,NULL AS `rev_actor`,rev_content_format,rev_content_model,page_namespace,page_title,page_id,page_latest,page_is_redirect,page_len,user_name,page_is_new,
( SELECT  GROUP_CONCAT(ctd_name SEPARATOR ',')
    FROM `change_tag`
    JOIN `change_tag_def`
    ON ((ct_tag_id=ctd_id))
    WHERE ct_rev_id=rev_id  ) AS `ts_tags`,ores_damaging_cls.oresc_probability AS `ores_damaging_score`,0.412 AS `ores_damaging_threshold`
FROM `revision`
LEFT JOIN `revision_comment_temp` `temp_rev_comment` ON ((temp_rev_comment.revcomment_rev = rev_id))
LEFT JOIN `comment` `comment_rev_comment` ON ((comment_rev_comment.comment_id = temp_rev_comment.revcomment_comment_id))
INNER JOIN `page` ON ((page_id = rev_page))
LEFT JOIN `user` ON ((rev_user != 0) AND (user_id = rev_user))
LEFT JOIN `ores_classification` `ores_damaging_cls` ON (ores_damaging_cls.oresc_model = '9' AND (ores_damaging_cls.oresc_rev=rev_id) AND ores_damaging_cls.oresc_class = '1')
WHERE ((rev_user = '2752938')) AND (rev_parent_id = 0) AND (page_namespace = '0') AND ((rev_deleted & 4) = 0)  ORDER BY rev_timestamp DESC LIMIT 51

I tried running it against wikidatawiki on db1087, and it comes back in 0.04 seconds. That seems fine, but something about it apparently caused problems in the past.

Yeah, looked like another case of the mariadb optimizer being just silly and then getting fixed on following mariadb versions.

If not, probably we should proceed to create the index as UNIQUE with revactor_rev included, as altering this table will take around 2h per host so it will be a bit painful to do this on a separate change in a few weeks/months if needed.

On the revision table, the field would be rev_id, so we'd have: ADD UNIQUE INDEX /*i*/rev_actor_timestamp (rev_actor,rev_timestamp,rev_id).

Got it

Change 601949 had a related patch set uploaded (by Marostegui; owner: Marostegui):
[operations/puppet@production] mariadb: Reimage db2071

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

Change 601949 merged by Marostegui:
[operations/puppet@production] mariadb: Reimage db2071

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

Script wmf-auto-reimage was launched by marostegui on cumin1001.eqiad.wmnet for hosts:

['db2071.codfw.wmnet']

The log can be found in /var/log/wmf-auto-reimage/202006030518_marostegui_16535.log.

Completed auto-reimage of hosts:

['db2071.codfw.wmnet']

and were ALL successful.

Self note, db2124 from T238966#6148579 needs to get the extra index once the new patch is merged at https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/601713/

Change 601713 merged by jenkins-bot:
[mediawiki/core@master] make rev_actor_timestamp index cover the rev_id field.

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

Marostegui updated the task description. (Show Details)Wed, Jun 3, 11:07 AM

Change 601713 merged by jenkins-bot:
[mediawiki/core@master] make rev_actor_timestamp index cover the rev_id field.

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

As this has been merged, I have added it to the original task description as part of the schema changes needed

Mentioned in SAL (#wikimedia-operations) [2020-06-03T11:08:51Z] <marostegui> Add rev_id to revision table on db2124 - T238966

Self note, db2124 from T238966#6148579 needs to get the extra index once the new patch is merged at https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/601713/

Done.

@daniel this is db2124 schema after applying the changes for frwiki, jawiki and ruwiki.

Please confirm this looks good to you:

*************************** 1. row ***************************
       Table: archive
Create Table: CREATE TABLE `archive` (
  `ar_id` int(10) unsigned NOT NULL AUTO_INCREMENT,
  `ar_namespace` int(11) NOT NULL DEFAULT 0,
  `ar_title` varbinary(255) NOT NULL DEFAULT '',
  `ar_comment_id` bigint(20) unsigned NOT NULL,
  `ar_actor` bigint(20) unsigned NOT NULL,
  `ar_timestamp` varbinary(14) NOT NULL DEFAULT '',
  `ar_minor_edit` tinyint(1) NOT NULL DEFAULT 0,
  `ar_rev_id` int(10) unsigned NOT NULL,
  `ar_deleted` tinyint(1) unsigned NOT NULL DEFAULT 0,
  `ar_len` int(8) unsigned DEFAULT NULL,
  `ar_page_id` int(10) unsigned DEFAULT NULL,
  `ar_parent_id` int(10) unsigned DEFAULT NULL,
  `ar_sha1` varbinary(32) NOT NULL DEFAULT '',
  PRIMARY KEY (`ar_id`),
  UNIQUE KEY `ar_revid_uniq` (`ar_rev_id`),
  KEY `name_title_timestamp` (`ar_namespace`,`ar_title`,`ar_timestamp`),
  KEY `ar_actor_timestamp` (`ar_actor`,`ar_timestamp`)
) ENGINE=InnoDB AUTO_INCREMENT=10937155 DEFAULT CHARSET=binary ROW_FORMAT=COMPRESSED KEY_BLOCK_SIZE=8
*************************** 1. row ***************************
       Table: revision
Create Table: CREATE TABLE `revision` (
  `rev_id` int(8) unsigned NOT NULL AUTO_INCREMENT,
  `rev_page` int(8) unsigned NOT NULL DEFAULT 0,
  `rev_comment_id` bigint(20) unsigned NOT NULL DEFAULT 0,
  `rev_actor` bigint(20) unsigned NOT NULL DEFAULT 0,
  `rev_timestamp` varbinary(14) NOT NULL DEFAULT '',
  `rev_minor_edit` tinyint(1) unsigned NOT NULL DEFAULT 0,
  `rev_deleted` tinyint(1) unsigned NOT NULL DEFAULT 0,
  `rev_len` int(8) unsigned DEFAULT NULL,
  `rev_parent_id` int(8) unsigned DEFAULT NULL,
  `rev_sha1` varbinary(32) NOT NULL DEFAULT '',
  PRIMARY KEY (`rev_id`),
  KEY `rev_timestamp` (`rev_timestamp`),
  KEY `page_timestamp` (`rev_page`,`rev_timestamp`),
  KEY `rev_page_id` (`rev_page`,`rev_id`),
  KEY `rev_page_actor_timestamp` (`rev_page`,`rev_actor`,`rev_timestamp`),
  KEY `rev_actor_timestamp` (`rev_actor`,`rev_timestamp`,`rev_id`)
) ENGINE=InnoDB AUTO_INCREMENT=171619139 DEFAULT CHARSET=binary ROW_FORMAT=COMPRESSED KEY_BLOCK_SIZE=8
daniel added a comment.Wed, Jun 3, 2:50 PM

Self note, db2124 from T238966#6148579 needs to get the extra index once the new patch is merged at https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/601713/

Done.

@daniel this is db2124 schema after applying the changes for frwiki, jawiki and ruwiki.

Please confirm this looks good to you:

Yes, LGTM

T252219 is done (only pending double checking from @Bstorm) so I am going to proceed with testwiki in eqiad

Mentioned in SAL (#wikimedia-operations) [2020-06-04T10:42:02Z] <marostegui> Deploy schema change on s3 (only testwiki) codfw - T238966

Mentioned in SAL (#wikimedia-operations) [2020-06-04T10:46:56Z] <marostegui> Deploy schema change on s3 (only testwiki) eqiad - T238966

Marostegui updated the task description. (Show Details)Thu, Jun 4, 11:06 AM

@daniel - testwiki has been altered in production (all hosts, including the master and active slaves), if you want do double check everything that's good.
I will monitor logstash to see if we run into errors. My idea would be to leave this like that until Monday and start with s6 on Monday (frwiki jawiki ruwiki).

@daniel the new columns can be replicated safely to labsdb hosts?

rev_comment_id
rev_actor

We used to have the following triggers on sanitarium (which need to be dropped while we keep deploying this schema change):

| revision_insert         | INSERT | revision         | SET NEW.rev_text_id = 0
| revision_update         | UPDATE | revision         | SET NEW.rev_text_id = 0

The original description of the task says that they are good and the only thing needed is the work on the views: T238966#6118001 but just to confirm.

Marostegui updated the task description. (Show Details)Thu, Jun 4, 12:48 PM