Expectation (readQueryTime <= 5) by MediaWiki\Actions\ActionEntryPoint::execute not met (actual: {actualSeconds}) in trx #{trxId}:
{query}| Status | Subtype | Assigned | Task | |
|---|---|---|---|---|
| · · · | ||||
| Open | Ladsgroup | T28741 Migrate file tables to a modern layout (image/oldimage; file/filerevision; add primary keys) | ||
| Resolved | Security | • Zabe | T423654 Expectation (readQueryTime <= 5) by MediaWiki\Actions\ActionEntryPoint::execute not met (actual: {actualSeconds}) in trx #{trxId}:{query} | |
| · · · |
This is so bad, it could easily be exploided as a DOS vector. Reproducable by visiting https://en.wikipedia.org/wiki/Special:ListFiles.
wikiadmin2023@10.64.0.172(enwiki)> explain SELECT fr_timestamp,file_name,fr_size,CASE WHEN file_latest = fr_id THEN 'yes' ELSE 'no' END AS `top`,comment_fr_description.comment_text AS `fr_description_text`,comment_fr_description.comment_data AS `fr_description_data`,comment_fr_description.comment_id AS `fr_description_cid`,'fr_description' AS `description_field`,actor_user,actor_name FROM `file` JOIN `filerevision` ON ((fr_file=file_id)) JOIN `actor` ON ((actor_id=fr_actor)) JOIN `comment` `comment_fr_description` ON ((comment_fr_description.comment_id = fr_description_id)) WHERE file_deleted = 0 AND fr_deleted = 0 AND (file_latest = fr_id) ORDER BY fr_timestamp DESC,file_name DESC LIMIT 51 ; +------+-------------+------------------------+--------+------------------------------------+---------+---------+---------------------------------------+--------+----------------------------------------------+ | id | select_type | table | type | possible_keys | key | key_len | ref | rows | Extra | +------+-------------+------------------------+--------+------------------------------------+---------+---------+---------------------------------------+--------+----------------------------------------------+ | 1 | SIMPLE | file | ALL | PRIMARY,file_latest | NULL | NULL | NULL | 985386 | Using where; Using temporary; Using filesort | | 1 | SIMPLE | filerevision | eq_ref | PRIMARY,fr_actor_timestamp,fr_file | PRIMARY | 8 | enwiki.file.file_latest | 1 | Using where | | 1 | SIMPLE | actor | eq_ref | PRIMARY | PRIMARY | 8 | enwiki.filerevision.fr_actor | 1 | | | 1 | SIMPLE | comment_fr_description | eq_ref | PRIMARY | PRIMARY | 8 | enwiki.filerevision.fr_description_id | 1 | | +------+-------------+------------------------+--------+------------------------------------+---------+---------+---------------------------------------+--------+----------------------------------------------+ 4 rows in set (0.001 sec) wikiadmin2023@10.64.0.172(enwiki)>
This is how the read old query looks like.
wikiadmin2023@10.64.16.22(enwiki)> explain SELECT img_timestamp,img_name,img_size,'yes' AS `top`,comment_img_description.comment_text AS `img_description_text`,comment_img_description.comment_data AS `img_description_data`,comment_img_description.comment_id AS `img_description_cid`,'img_description' AS `description_field`,actor_user,actor_name FROM `image` JOIN `actor` ON ((actor_id=img_actor)) JOIN `comment` `comment_img_description` ON ((comment_img_description.comment_id = img_description_id)) ORDER BY img_timestamp DESC,img_name DESC LIMIT 51 ; +------+-------------+-------------------------+--------+---------------------+---------------+---------+---------------------------------+------+-------+ | id | select_type | table | type | possible_keys | key | key_len | ref | rows | Extra | +------+-------------+-------------------------+--------+---------------------+---------------+---------+---------------------------------+------+-------+ | 1 | SIMPLE | image | index | img_actor_timestamp | img_timestamp | 14 | NULL | 51 | | | 1 | SIMPLE | actor | eq_ref | PRIMARY | PRIMARY | 8 | enwiki.image.img_actor | 1 | | | 1 | SIMPLE | comment_img_description | eq_ref | PRIMARY | PRIMARY | 8 | enwiki.image.img_description_id | 1 | | +------+-------------+-------------------------+--------+---------------------+---------------+---------+---------------------------------+------+-------+ 3 rows in set (0.001 sec) wikiadmin2023@10.64.16.22(enwiki)>
The fix is similar to https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1270055. IMO it should be fine to push it through gerrit.
LGTM. My only concern is about straight join since it allows for filtering based on username[1] so in some cases actor should be the first table and straight join could break things. Would you mind testing the patch in mw-experimental? If it doesn't break the join order on those cases, consider it a +2 from my side.
[1] That filtering doesn't make sense. https://test.wikipedia.org/wiki/Special:NewFiles?user=Ladsgroup is the same as https://test.wikipedia.org/wiki/Special:ListFiles/Ladsgroup future-me problem.
I also agree a patch in gerrit with an innocuous message ("improving performance") is okay.
Sure. It seems fine. I am only straight joining the file table, so case of doing a user filter, it still queries the actor table first.
e.g. the query for https://en.wikipedia.org/wiki/Special:ListFiles?limit=50&user=Ladsgroup using the patch:
wikiadmin2023@10.64.16.22(enwiki)> explain SELECT fr_timestamp,file_name,fr_id,fr_size,CASE WHEN file_latest = fr_id THEN 'yes' ELSE 'no' END AS `top`,comment_fr_description.comment_text AS `fr_description_text`,comment_fr_description.comment_data AS `fr_description_data`,comment_fr_description.comment_id AS `fr_description_cid`,'fr_description' AS `description_field`,actor_user,actor_name FROM `filerevision` STRAIGHT_JOIN `file` ON ((fr_file=file_id)) JOIN `actor` ON ((actor_id=fr_actor)) JOIN `comment` `comment_fr_description` ON ((comment_fr_description.comment_id = fr_description_id)) WHERE file_deleted = 0 AND fr_deleted = 0 AND actor_name = 'Ladsgroup' AND (file_latest = fr_id) ORDER BY fr_timestamp DESC,fr_id DESC LIMIT 51 ; +------+-------------+------------------------+--------+------------------------------------+--------------------+---------+---------------------------------------+------+-------------+ | id | select_type | table | type | possible_keys | key | key_len | ref | rows | Extra | +------+-------------+------------------------+--------+------------------------------------+--------------------+---------+---------------------------------------+------+-------------+ | 1 | SIMPLE | actor | const | PRIMARY,actor_name | actor_name | 257 | const | 1 | | | 1 | SIMPLE | filerevision | ref | PRIMARY,fr_actor_timestamp,fr_file | fr_actor_timestamp | 8 | const | 4 | Using where | | 1 | SIMPLE | file | eq_ref | PRIMARY,file_latest | PRIMARY | 8 | enwiki.filerevision.fr_file | 1 | Using where | | 1 | SIMPLE | comment_fr_description | eq_ref | PRIMARY | PRIMARY | 8 | enwiki.filerevision.fr_description_id | 1 | | +------+-------------+------------------------+--------+------------------------------------+--------------------+---------+---------------------------------------+------+-------------+ 4 rows in set (0.001 sec) wikiadmin2023@10.64.16.22(enwiki)>
But I found a different issue, when going to https://en.wikipedia.org/wiki/Special:ListFiles?limit=50&user=&ilshowall=1 using the patch:
wikiadmin2023@10.64.16.22(enwiki)> explain SELECT fr_timestamp,file_name,fr_id,fr_size,CASE WHEN file_latest = fr_id THEN 'yes' ELSE 'no' END AS `top`,comment_fr_description.comment_text AS `fr_description_text`,comment_fr_description.comment_data AS `fr_description_data`,comment_fr_description.comment_id AS `fr_description_cid`,'fr_description' AS `description_field`,actor_user,actor_name FROM `filerevision` STRAIGHT_JOIN `file` ON ((fr_file=file_id)) JOIN `actor` ON ((actor_id=fr_actor)) JOIN `comment` `comment_fr_description` ON ((comment_fr_description.comment_id = fr_description_id)) WHERE file_deleted = 0 AND fr_deleted = 0 ORDER BY file_name DESC LIMIT 51 ; +------+-------------+------------------------+--------+----------------------------+---------+---------+---------------------------------------+---------+----------------------------------------------+ | id | select_type | table | type | possible_keys | key | key_len | ref | rows | Extra | +------+-------------+------------------------+--------+----------------------------+---------+---------+---------------------------------------+---------+----------------------------------------------+ | 1 | SIMPLE | filerevision | ALL | fr_actor_timestamp,fr_file | NULL | NULL | NULL | 1363393 | Using where; Using temporary; Using filesort | | 1 | SIMPLE | file | eq_ref | PRIMARY | PRIMARY | 8 | enwiki.filerevision.fr_file | 1 | Using where | | 1 | SIMPLE | actor | eq_ref | PRIMARY | PRIMARY | 8 | enwiki.filerevision.fr_actor | 1 | | | 1 | SIMPLE | comment_fr_description | eq_ref | PRIMARY | PRIMARY | 8 | enwiki.filerevision.fr_description_id | 1 | | +------+-------------+------------------------+--------+----------------------------+---------+---------+---------------------------------------+---------+----------------------------------------------+ 4 rows in set (0.001 sec) wikiadmin2023@10.64.16.22(enwiki)>
Maybe in those cases making the order be fr_file would be better? Otherwise, order should be id or timestamp.
Ok so apparently this specifically is caused by a hack to prevent slow queries in read old (https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/core/+/cf098948baef163688b3f3fa42613882c68aa07b/includes/Specials/Pager/ImageListPager.php#277) which was added due to a missing oi_timestamp index. The index then got added in T279982, but apparently no one ever remembered to remove the hack.
So I will just go ahead and disable this hack in read new. This fixes the regular query for the case described above. But it still does cause issues if the user manually sorts by the name. The issue is caused by the straight join because in this case we actually want to query the file table first. So we need to make the straight join conditional.
Change #1273011 had a related patch set uploaded (by Zabe; author: Zabe):
[mediawiki/core@master] ImageListPager: Make sure file and filerevision are in correct order
I tested the new patch with all combination shown above on enwiki and it seems to no longer produce bad queries.
Change #1273011 merged by jenkins-bot:
[mediawiki/core@master] ImageListPager: Make sure file and filerevision are in correct order
Change #1273787 had a related patch set uploaded (by Jforrester; author: Zabe):
[mediawiki/core@wmf/1.46.0-wmf.24] ImageListPager: Make sure file and filerevision are in correct order
Change #1273787 abandoned by Jforrester:
[mediawiki/core@wmf/1.46.0-wmf.24] ImageListPager: Make sure file and filerevision are in correct order