Page MenuHomePhabricator

Schema change for renaming several indexes in logging table
Closed, ResolvedPublic

Description

Part of T270033: Fix and enforce table prefix usage in columns and indexes in core

  1. ALTERs to run: https://gerrit.wikimedia.org/r/c/mediawiki/core/+/648817/3/maintenance/archives/patch-logging-rename-indexes.sql
  2. Where to run those changes: all.dblist
  3. When to run those changes: At any time
  4. If the schema change is backwards compatible: Yes
  5. If the schema change has been tested already on some of the test/beta wikis: Tested in beta cluster.
  6. if the data should be made available on the labs replicas and/or dumps: Yes, data in this table is public

Progress

  • s1
    • eqiad
    • codfw
  • s2
    • eqiad
    • codfw
  • s3
    • eqiad
    • codfw
  • s4
    • eqiad
    • codfw
  • s5
    • eqiad
    • codfw
  • s7
    • eqiad
    • codfw
  • s8
    • eqiad
    • codfw
  • labswiki
  • labtestwiki

Event Timeline

LSobanski triaged this task as Medium priority.Dec 21 2020, 12:37 PM
LSobanski moved this task from Triage to Ready on the DBA board.
LSobanski added a subscriber: LSobanski.

The first link doesn't matter as it's not deployed in our systems AFAIK

The second one actually has changed to accommodates this change. See https://gerrit.wikimedia.org/r/c/mediawiki/extensions/CheckUser/+/652400

I'll contact Manual re the first one shortly.

@RhinosF1 and myself just sync and it is fine to proceed as examknow extension isn't enabled, as Amir says, and it is likely it won't be in the future anyways.
Thank you both!

Deployed this change on db1180 and db1096:3316, if nothing goes wrong on the query front, we can deploy it everywhere on codfw (or on eqiad if eqiad is passive already)

s6 eqiad progress

  • dbstore1005
  • db1180
  • db1173
  • db1168
  • db1165
  • db1155
  • db1140
  • db1131
  • db1113
  • db1098
  • db1096
  • clouddb1021
  • clouddb1019
  • clouddb1015

@Ladsgroup I have seen this query:

SELECT /* IndexPager::buildQueryInfo (LogPager) */ log_id, log_type, log_action, log_timestamp, log_namespace, log_title, log_params, log_deleted, user_id, user_name, user_editcount, log_actor, logging_actor.actor_user AS `log_user`, logging_actor.actor_name AS `log_user_text`, comment_log_comment.comment_text AS `log_comment_text`, comment_log_comment.comment_data AS `log_comment_data`, comment_log_comment.comment_id AS `log_comment_cid`, (SELECT GROUP_CONCAT(ctd_name SEPARATOR ', ') FROM `change_tag` JOIN `change_tag_def` ON ((ct_tag_id=ctd_id)) WHERE ct_log_id=log_id ) AS `ts_tags` FROM `logging` IGNORE INDEX (log_times) JOIN `actor` `logging_actor` ON ((actor_id=log_actor)) LEFT JOIN `user` ON ((user_id=logging_actor.actor_user)) JOIN `comment` `comment_log_comment` ON ((comment_log_comment.comment_id = log_comment_id)) JOIN `change_tag` ON ((ct_log_id=log_id)) WHERE (log_type NOT IN ('titleblacklist', 'urlshortener', 'abusefilterprivatedetails', 'oath', 'suppress')) AND log_type = 'create' AND ct_tag_id = 99 ORDER BY log_timestamp DESC LIMIT 51 /* 263df222a3933ac11fa72cdc4073333d db1096:3316 frwiki 13s */

That query is specifically ignoring log_times index, which is not present on any of the hosts apart from the two we've done in s6. We might want to remove this before proceeding.
The optimizer doesn't seem to be silly anymore, so the IGNORE might not be needed:

root@db1098.eqiad.wmnet[frwiki]> explain SELECT /* IndexPager::buildQueryInfo (LogPager) */ log_id, log_type, log_action, log_timestamp, log_namespace, log_title, log_params, log_deleted, user_id, user_name, user_editcount, log_actor, logging_actor.actor_user AS `log_user`, logging_actor.actor_name AS `log_user_text`, comment_log_comment.comment_text AS `log_comment_text`, comment_log_comment.comment_data AS `log_comment_data`, comment_log_comment.comment_id AS `log_comment_cid`, (SELECT GROUP_CONCAT(ctd_name SEPARATOR ', ') FROM `change_tag` JOIN `change_tag_def` ON ((ct_tag_id=ctd_id)) WHERE ct_log_id=log_id ) AS `ts_tags` FROM `logging` JOIN `actor` `logging_actor` ON ((actor_id=log_actor)) LEFT JOIN `user` ON ((user_id=logging_actor.actor_user)) JOIN `comment` `comment_log_comment` ON ((comment_log_comment.comment_id = log_comment_id)) JOIN `change_tag` ON ((ct_log_id=log_id)) WHERE (log_type NOT IN ('titleblacklist', 'urlshortener', 'abusefilterprivatedetails', 'oath', 'suppress')) AND log_type = 'create' AND ct_tag_id = 99 ORDER BY log_timestamp DESC LIMIT 51 /* 263df222a3933ac11fa72cdc4073333d db1096:3316 frwiki 13s */
    -> ;
+------+--------------------+---------------------+--------+------------------------------------------------------------------+---------------+---------+---------------------------------+--------+-----------------------------------------------------------+
| id   | select_type        | table               | type   | possible_keys                                                    | key           | key_len | ref                             | rows   | Extra                                                     |
+------+--------------------+---------------------+--------+------------------------------------------------------------------+---------------+---------+---------------------------------+--------+-----------------------------------------------------------+
|    1 | PRIMARY            | change_tag          | ref    | ct_log_tag_id,ct_tag_id_id                                       | ct_tag_id_id  | 4       | const                           | 950912 | Using where; Using index; Using temporary; Using filesort |
|    1 | PRIMARY            | logging             | eq_ref | PRIMARY,type_time,actor_time,log_actor_type_time,log_type_action | PRIMARY       | 4       | frwiki.change_tag.ct_log_id     | 1      | Using where                                               |
|    1 | PRIMARY            | logging_actor       | eq_ref | PRIMARY                                                          | PRIMARY       | 8       | frwiki.logging.log_actor        | 1      |                                                           |
|    1 | PRIMARY            | user                | eq_ref | PRIMARY                                                          | PRIMARY       | 4       | frwiki.logging_actor.actor_user | 1      | Using where                                               |
|    1 | PRIMARY            | comment_log_comment | eq_ref | PRIMARY                                                          | PRIMARY       | 8       | frwiki.logging.log_comment_id   | 1      |                                                           |
|    2 | DEPENDENT SUBQUERY | change_tag          | ref    | ct_log_tag_id,ct_tag_id_id                                       | ct_log_tag_id | 5       | frwiki.logging.log_id           | 7      | Using index                                               |
|    2 | DEPENDENT SUBQUERY | change_tag_def      | eq_ref | PRIMARY                                                          | PRIMARY       | 4       | frwiki.change_tag.ct_tag_id     | 1      |                                                           |
+------+--------------------+---------------------+--------+------------------------------------------------------------------+---------------+---------+---------------------------------+--------+-----------------------------------------------------------+
7 rows in set (0.012 sec)

And this is on enwiki:

+------+--------------------+---------------------+--------+------------------------------------------------------------------+-----------------------+---------+---------------------------------+------+-----------------------------------------------------------+
| id   | select_type        | table               | type   | possible_keys                                                    | key                   | key_len | ref                             | rows | Extra                                                     |
+------+--------------------+---------------------+--------+------------------------------------------------------------------+-----------------------+---------+---------------------------------+------+-----------------------------------------------------------+
|    1 | PRIMARY            | change_tag          | ref    | change_tag_log_tag_id,change_tag_tag_id_id                       | change_tag_tag_id_id  | 4       | const                           | 67   | Using where; Using index; Using temporary; Using filesort |
|    1 | PRIMARY            | logging             | eq_ref | PRIMARY,type_time,actor_time,log_actor_type_time,log_type_action | PRIMARY               | 4       | enwiki.change_tag.ct_log_id     | 1    | Using where                                               |
|    1 | PRIMARY            | logging_actor       | eq_ref | PRIMARY                                                          | PRIMARY               | 8       | enwiki.logging.log_actor        | 1    |                                                           |
|    1 | PRIMARY            | user                | eq_ref | PRIMARY                                                          | PRIMARY               | 4       | enwiki.logging_actor.actor_user | 1    | Using where                                               |
|    1 | PRIMARY            | comment_log_comment | eq_ref | PRIMARY                                                          | PRIMARY               | 8       | enwiki.logging.log_comment_id   | 1    |                                                           |
|    2 | DEPENDENT SUBQUERY | change_tag          | ref    | change_tag_log_tag_id,change_tag_tag_id_id                       | change_tag_log_tag_id | 5       | enwiki.logging.log_id           | 7    | Using index                                               |
|    2 | DEPENDENT SUBQUERY | change_tag_def      | eq_ref | PRIMARY                                                          | PRIMARY               | 4       | enwiki.change_tag.ct_tag_id     | 1    |                                                           |
+------+--------------------+---------------------+--------+------------------------------------------------------------------+-----------------------+---------+---------------------------------+------+-----------------------------------------------------------+
7 rows in set (0.012 sec)

Okay this is interesting. The code for this is actually explicitly set to check which index exists and ignore that instead. The comment for using this index is:

			// T223151, T237026: MariaDB's optimizer, at least 10.1, likes to choose a wildly bad plan for
			// some reason for these code paths. Tell it not to use the wrong index it wants to pick.

Maybe it doesn't apply anymore? maybe since logging table got smaller now MariaDB is not going haywire?

The code that checks is $index = $this->mDb->indexExists( 'logging', 'times', __METHOD__ ) ? 'times' : 'log_times';

I am going to double check across big wikis to see what the optimizer is doing, but yeah, likely it isn't needed anymore

Tested the following wikis:

  • enwiki
  • commonswiki
  • dewiki
  • eswiki
  • wikidatawiki

The optimizer is doing the right thing, so it is probably ok to remove that IGNORE.

Change 700515 had a related patch set uploaded (by Ladsgroup; author: Ladsgroup):

[mediawiki/core@master] Remove logging table index hint

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

Marostegui changed the task status from Open to Stalled.Jun 22 2021, 9:03 AM
Marostegui moved this task from In progress to Blocked on the DBA board.

Stalling this till the above is pushed

Change 700515 had a related patch set uploaded (by Ladsgroup; author: Ladsgroup):

[mediawiki/core@master] Remove logging table index hint

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

Can someone from PET review this patch? @daniel

Change 700515 merged by jenkins-bot:

[mediawiki/core@master] Remove logging table index hint

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

@Marostegui The index check has been removed and now deployed. I think this is good to go.

Marostegui changed the task status from Stalled to Open.Jul 5 2021, 6:27 AM

codfw s6

  • db2141
  • db2129
  • db2124
  • db2117
  • db2114
  • db2095
  • db2089
  • db2087
  • db2076
Marostegui updated the task description. (Show Details)

Altered two hosts in production in s6 to make sure we don't see anymore issues. If we don't, I will continue with all eqiad sections.

Marostegui changed the task status from Open to Stalled.Jul 21 2021, 5:12 AM
Marostegui updated the task description. (Show Details)

All eqiad is done - waiting for the switch back.

Marostegui changed the task status from Stalled to Open.Sep 15 2021, 5:31 AM

Mentioned in SAL (#wikimedia-operations) [2021-09-30T05:45:03Z] <marostegui> Deploy schema change on s2 codfw (lag will show up) T270620

Mentioned in SAL (#wikimedia-operations) [2021-09-30T05:45:35Z] <marostegui> Deploy schema change on s4 codfw (lag will show up) T270620

Mentioned in SAL (#wikimedia-operations) [2021-09-30T05:47:41Z] <marostegui> Deploy schema change on s5 codfw (lag will show up) T270620

Mentioned in SAL (#wikimedia-operations) [2021-09-30T05:52:09Z] <marostegui> Deploy schema change on s7 codfw (lag will show up) T270620

Mentioned in SAL (#wikimedia-operations) [2021-09-30T05:53:01Z] <marostegui> Deploy schema change on s3 codfw (lag will show up) T270620

Mentioned in SAL (#wikimedia-operations) [2021-09-30T06:01:31Z] <marostegui> Deploy schema change on s1 codfw (lag will show up) T270620

Mentioned in SAL (#wikimedia-operations) [2021-09-30T06:48:31Z] <marostegui> Deploy schema change on s8 codfw (lag will show up) T270620

Marostegui updated the task description. (Show Details)

This is all done