Marking as resolved as this fix should address this. If logstash still shows the entries after this fix is deployed to WMF wikis, this can be re-opened.

I would suggest that QA isn't needed for this task and the way this is verified to work is that logstash is inspected next week to ensure these errors are no longer being triggered.

The last change (https://gerrit.wikimedia.org/r/c/mediawiki/extensions/MediaModeration/+/977215/) shouldn't need QA, so this can be marked as resolved.

Seems resolved based on logstash (other exceeds of limits are from other code).

Marking this as resolved as I don't see any more of these in the last few days.

This will need to wait until either:

1. The three relevant changes (https://gerrit.wikimedia.org/r/c/mediawiki/extensions/MediaModeration/+/979167/, https://gerrit.wikimedia.org/r/c/mediawiki/extensions/MediaModeration/+/974687/, https://gerrit.wikimedia.org/r/c/mediawiki/extensions/MediaModeration/+/978155) are backported
2. The 12-18th of December (depending on the specific wiki), so that the train with the above changes in has been normally carried out

Suggested QA steps (requires local wiki to test - cannot be tested on betawikis as CheckUser is not enabled):

1. Install CheckUser, if required
2. Make one testing edit using Google Chrome
3. Open the DB
4. Run the following SQL:

SELECT MIN(uachm_reference_id) FROM cu_useragent_clienthints_map;

5. If the above query returns 1, then do the following (otherwise skip forward to step 6):
   1. Run the SQL listed after step 5C
   2. Make another testing edit
   3. Go back to step 4

TRUNCATE TABLE cu_changes;
TRUNCATE TABLE cu_useragent_clienthints_map;

6. Run the following SQL, replacing <lower uachm_reference_id> with a integer that is smaller than the one obtained in step 4. You may repeat this step with different values for <lower uachm_reference_id> each time as long as they are still smaller than the integer from step 4. However, do not run this more than 100 times.

INSERT INTO cu_useragent_clienthints_map (uachm_reference_id, uachm_reference_type, uachm_uach_id) VALUES (<lower uachm_reference_id>, 0, 1), (<lower uachm_reference_id>, 0, 1), (<lower uachm_reference_id>, 0, 1);

7. Close the DB (or open a new console window)
8. Run the purgeOldData.php maintenance script
9. Verify that the output of the maintenance script has on each line after cu_changes, cu_log_event, or cu_private_event the text like Purged 0 rows and Y client hint mapping rows purged. where Y is the number of times you repeated step 6.
10. Open the DB, if necessary
11. Repeat step 4 and make sure the result was the same.

Suggested QA steps for betawikis:

1. Make sure you have access to betawiki DBs, and if not get access
2. Go to https://meta.wikimedia.beta.wmflabs.org/wiki/Special:SiteMatrix and choose a wikipedia from the list
3. Go to that betawiki and load Special:ListFiles. Check that images appear in that list (ignore audio/video). If not, repeat step 2 to choose a different beta wikipedia.
4. Connect to the betawiki over ssh (e.g. ssh deployment-deploy03.deployment-prep.eqiad1.wikimedia.cloud)
5. Open the DB for your chosen wikipedia (e.g. sql dewiki)
6. Run the following SQL and keep a note of the output:

SELECT COUNT(*) FROM mediamoderation_scan;

7. Run the maintenance script (this can be done via mwscript extensions/MediaModeration/maintenance/importExistingFilesToScanTable.php --wiki=dewiki replacing dewiki with the name of the wiki you chosen
8. Make sure the maintenance script runs without any errors
9. Repeat steps 5 and 6.
10. Verify that the second time you ran the query a larger count was present

One addendum to the QA steps is that you should use a midi file that hasn't been uploaded to the wiki before. This means choosing a new one from https://commons.wikimedia.org/wiki/Category:MIDI_files that you have not used for testing before.

Thanks @DatGuy for this patch. I'm closing this as resolved as the acceptance criteria have been completed.

@DatGuy can this be closed now?

I will write suggested QA steps for betawikis by tommorrow. QA should wait until QA on T350323 an T352234 is completed.

QA should not be needed for the change being made to core (as this only affects method documentation).

In T351944#9365023, @Ladsgroup wrote:

In T351944#9364059, @Dreamy_Jazz wrote:

In T351944#9363967, @Ladsgroup wrote:

And on the opposite direction, I have been working to remove indexes that are only used by maint scripts as these script don't have a time constraint and can split the work into different chunks and do it differently (the one I will do soon is index on cl_collation in categorylinks). So if it's possible to split deletion into chunks and go through them, it would be much better.

The issue I have is that the "atomic unit" for a query using the method that I went with takes over a minute. If the vslow group was used, could the query take over a minute to run?

Usually it's fine to run one-minute-long queries in maint scripts as long as it's only on vslow and it's not like 1 million one-minute-long queries. I also do wonder if there is a way to chop it to let's says 60 queries that take 1s. Drop me the query somewhere and I try to see what can be done.

The query is the first listed in the task description.

I propose the following to fix this:

• Make MediaModeration only accept files that have File::getMediaType return MEDIATYPE_BITMAP or MEDIATYPE_DRAWING
• Ensure that MediaModerationFileProcessor::canScanFile is always called before attempting to scan file (even if the DB has an entry for it)
• (potentially) Write a maintenance script to remove files from the scan table that do not have File::getMediaType return MEDIATYPE_BITMAP or MEDIATYPE_DRAWING.

Copying @Tchanders alternative implementation method from the declined subtask to here:

In T351944#9360422, @Tchanders wrote:

Do we need the index?

I'm wondering if we really need the new indexes. Since the CheckUser tables are quite large (I didn't have the permissions to check on production but they're some of the largest locally), it's a fair lift storing and recalculating the new indexes, so we should tread carefully when it comes to adding new ones.

From reading the code, it seems we want to know the smallest reference ID in the map table that isn't orphaned, working on assumption that reference IDs for a given reference type increase over time. So can we do this the other way round:

• Find the reference ID from the oldest row in the changes table, e.g. SELECT cuc_this_oldid FROM cu_changes WHERE cuc_this_oldid != 0 ORDER BY cuc_timestamp LIMIT 1
• Then delete all map rows for the same type with a lower uachm_reference_id

Does the increasing reference ID assumption hold?

Not completely, I think due to sharding - e.g. here are the oldest on enwiki:

(enwiki)> SELECT cuc_this_oldid FROM cu_changes ORDER BY cuc_timestamp LIMIT 10;
+----------------+
| cuc_this_oldid |
+----------------+
|     1172847017 |
|     1172847022 |
|     1172847019 |
|     1172847030 |
|     1172847028 |
|     1172847021 |
|     1172847024 |
|     1172847026 |
|     1172847027 |
|     1172847018 |
+----------------+
10 rows in set (0.001 sec)

This affects both the implementation from https://gerrit.wikimedia.org/r/c/mediawiki/extensions/CheckUser/+/977267 and the method suggested in this comment.

We could adapt the query above to look for the lowest ID in the first n rows sorted by timestamp instead, to ensure we almost definitely get the lowest ID. And/or we could decide we're OK with missing a very small amount of client hints data for the oldest changes, or with sometimes keeping a few data points for a few extra days.

I think it's clear that these indexes will not be added for the time being. I will explore different ways to avoid the need for these indexes in the parent task.

In T351944#9363967, @Ladsgroup wrote:

Sorry I wanted to respond sooner but got sick (and today we had an outage). I actually wanted to say the exact same thing as Thalia. We are really short of space in core dbs and don't have much room to breath. These tables are big already (e.g. in wikidata it's 12GB which is not small but not super large either, enwiki it's 6.4GB).

Thanks for the thoughts and no worries on any delay.

In T351944#9360422, @Tchanders wrote:

Do we need the index?

I'm wondering if we really need the new indexes. Since the CheckUser tables are quite large (I didn't have the permissions to check on production but they're some of the largest locally), it's a fair lift storing and recalculating the new indexes, so we should tread carefully when it comes to adding new ones.

From reading the code, it seems we want to know the smallest reference ID in the map table that isn't orphaned, working on assumption that reference IDs for a given reference type increase over time. So can we do this the other way round:

• Find the reference ID from the oldest row in the changes table, e.g. SELECT cuc_this_oldid FROM cu_changes WHERE cuc_this_oldid != 0 ORDER BY cuc_timestamp LIMIT 1
• Then delete all map rows for the same type with a lower uachm_reference_id

Does the increasing reference ID assumption hold?

Not completely, I think due to sharding - e.g. here are the oldest on enwiki:

(enwiki)> SELECT cuc_this_oldid FROM cu_changes ORDER BY cuc_timestamp LIMIT 10;
+----------------+
| cuc_this_oldid |
+----------------+
|     1172847017 |
|     1172847022 |
|     1172847019 |
|     1172847030 |
|     1172847028 |
|     1172847021 |
|     1172847024 |
|     1172847026 |
|     1172847027 |
|     1172847018 |
+----------------+
10 rows in set (0.001 sec)

This affects both the implementation from https://gerrit.wikimedia.org/r/c/mediawiki/extensions/CheckUser/+/977267 and the method suggested in this comment.

We could adapt the query above to look for the lowest ID in the first n rows sorted by timestamp instead, to ensure we almost definitely get the lowest ID. And/or we could decide we're OK with missing a very small amount of client hints data for the oldest changes, or with sometimes keeping a few data points for a few extra days.

I had not thought of this approach and in theory it seems better than trying to add new indexes. However, I see some more problems that would need solving:

• To make it possible to implement T170148, we could not rely on the fact that (within a small margin of error) the timestamp correlates to the ID associated with the entry in the cu_log_event table. This has received legal approval and is now technically possible to implement. IMO this would be useful for checkuser investigations
• If we ever implement searching by Client Hints data, these indexes will be needed. The equivalent task for user agent strings (T146837) seemed to be desired by CUs.

@Tchanders yes. As such I will move it to done.

Would welcome any thoughts from DBA on the proposed patch and in general about adding indexes.

With assistance from @kostajh, I was able to determine that a single query on wikidatawiki to determine if a given revision ID is in the cu_changes table can take over a minute to run. Considering this could be a maximum of 100 of these queries per maintenance script run and for the first few runs the script will be reaching the maximum, this will be too inefficient without an index.