WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 weeks ago

#31071 closed defect (bug) (fixed)

media / post_mime_type related queries are very slow on larger sites

Reported by: archon810 Owned by: jnylen0
Milestone: 4.7.4 Priority: normal
Severity: normal Version: 4.1
Component: Media Keywords: has-patch commit fixed-major
Focuses: performance Cc:

Description

Hey guys,

Remember #27985? It was about a slow query that did probably a full table scan, which was then replaced with two queries:

SELECT ID
		FROM wp_posts
		WHERE post_type = 'attachment'
		AND post_mime_type LIKE 'video%'
		LIMIT 1
SELECT ID
		FROM wp_posts
		WHERE post_type = 'attachment'
		AND post_mime_type LIKE 'audio%'
		LIMIT 1

On a busy server with 285k entries in wp_posts, this query is taking 5-7s each. And it takes forever to save/open an edit post screen.

The main problem is that there's not a proper index for it to use.

Running EXPLAIN shows this:

+----+-------------+----------+------+------------------+------------------+---------+-------+--------+-------------+
| id | select_type | table    | type | possible_keys    | key              | key_len | ref   | rows   | Extra       |
+----+-------------+----------+------+------------------+------------------+---------+-------+--------+-------------+
|  1 | SIMPLE      | wp_posts | ref  | type_status_date | type_status_date | 62      | const | 161195 | Using where |
+----+-------------+----------+------+------------------+------------------+---------+-------+--------+-------------+
1 row in set

I'm not really sure why such an unoptimized query would be put in place, but the index on (post_type, post_mime_type) fixes it:

+----+-------------+----------+-------+--------------------------------------+---------------------+---------+------+------+-------------+
| id | select_type | table    | type  | possible_keys                        | key                 | key_len | ref  | rows | Extra       |
+----+-------------+----------+-------+--------------------------------------+---------------------+---------+------+------+-------------+
|  1 | SIMPLE      | wp_posts | range | type_status_date,post_type_mime_type | post_type_mime_type | 364     | NULL |    1 | Using where |
+----+-------------+----------+-------+--------------------------------------+---------------------+---------+------+------+-------------+
1 row in set

The query now runs in 0.2s.

Attachments (11)

31071.diff (897 bytes) - added by wonderboymusic 2 years ago.
31071b.diff (1.1 KB) - added by philipjohn 18 months ago.
Fix the mime type queries
31071c.diff (1.2 KB) - added by philipjohn 18 months ago.
Updated patch includes query escaping and working mime types array
31071-fix-wp_enqueue_media-performance.patch (1.4 KB) - added by tha_sun 17 months ago.
media.php.diff (1.7 KB) - added by sboisvert 13 months ago.
31071-d.diff (940 bytes) - added by jamesmehorter 13 months ago.
Adds an index on the post_type and post_mime_type columns
WP Core Trac #31071 MySQL Queries.xlsx (47.3 KB) - added by jamesmehorter 13 months ago.
Spreadsheet of affected queries before/after new index
31071.2.diff (3.2 KB) - added by jnylen0 3 weeks ago.
Allow overriding slow media queries via filters
31071.3.diff (3.4 KB) - added by jnylen0 3 weeks ago.
Documentation improvements
31071.4.diff (2.8 KB) - added by jnylen0 3 weeks ago.
Further improvements for 4.7.4
31071.5.diff (3.0 KB) - added by jnylen0 3 weeks ago.
Fix the issue for 4.8

Download all attachments as: .zip

Change History (88)

#1 @archon810
2 years ago

I'm noticing a side effect of this new index - MySQL is being dumb and is using this new index on this query, which now runs really slow (15s):

SELECT DISTINCT YEAR( post_date ) AS year, MONTH( post_date ) AS month
		FROM wp_posts use index (type_status_date)
		WHERE post_type = 'attachment'
		ORDER BY post_date DESC;

instead of the original type_status_date index which runs about 30 times faster for me (using USE INDEX (type_status_date).

It's quite frustrating, because using type_status_date results in Using where; Using index; Using temporary; Using filesort whereas using post_type_mime_type results in Using where; Using temporary; Using filesort - 'using index' is no longer there. It examines fewer rows, but the performance hit is huge. I'm surprised MySQL isn't smart enough here.

I'm going to play around with this some more to see if I can make an even better index to satisfy this query.

#2 @archon810
2 years ago

OK, without resorting to giving MySQL hints in the form of USE INDEX or FORCE INDEX, adding an index on (post_type, post_date) finally made it automatically use it to resolve the query in #comment:1 using an index. In fact, there's an improvement - "using filesort" which was there before in both cases is now gone: "Using where; Using index; Using temporary".

The query runs fast now.

#3 @mboynes
2 years ago

I'm currently building a site with about 1.6 million images and I've run into this issue again as well.

Adding the (post_type, post_mime_type) index did work for me as well, and drops each query from about 30s (!) to 1ms. MySQL 5.5.41 didn't use the wrong index for me in this query:

SELECT DISTINCT YEAR( post_date ) AS year, MONTH( post_date ) AS month
FROM wp_posts
WHERE post_type = 'attachment'
ORDER BY post_date DESC

However, that query is still very slow (~1.5s) for me.

I'm not sure there's a way to optimize this query, so I think we need to look elsewhere to fix it. It looks like the purpose is to populate the date filtering dropdown in the "Insert Media" dialog, which to be honest I've never even noticed. My suggestions would be to:

  1. replace this query with one to find the oldest date (MIN(post_date)), then add an entry for every month/year since then;
  2. similar to (1), run one query to find the oldest year (YEAR(MIN(post_date))), and split the filter into two dropdowns, one for year and one for month;
  3. add a layer of caching to this; or
  4. make this optionally filterable so small/medium sites can continue to use this as-is, and large sites can opt out of it.

I think I lean toward (2), as that dropdown can be unwieldy anyway for active sites with longstanding archives. (3) is probably the best option to maintain the existing functionality; this data could be stored in an option, and when a new image is added, it could be checked for that year-month combination and add it if need be.

#4 @Denis-de-Bernardy
2 years ago

Fwiw, this type of query (all section index generating types of queries, really):

SELECT DISTINCT YEAR( post_date ) AS year, MONTH( post_date ) AS month
		FROM wp_posts
		WHERE post_type = 'attachment'
		ORDER BY post_date DESC;

Can be rewritten by completing it in two separate steps.

In a first fetch the two bounds:

select min(post_date) as min_date, max(post_date) as max_date from wp_posts where post_type = 'attachment';

Then use php to generate all possible months:

$months = array('2014-01-01', '2014-02-01', ...);

And then use these months to rewrite the query like:

select year(min_bound), month(min_bound)
from (
  select cast('2014-01-01' as date) as min_bound, cast('2014-02-01' as date) as max_bound
  union all
  select cast('2014-02-01' as date) as min_bound, cast('2014-02-01' as date) as max_bound
  union all
  ...
) as bounds
where exists (
  select 1
  from wp_posts
  where post_type = 'attachment'
  and post_date >= min_bound
  and post_date < max_bound
)
order by min_bound desc

The general idea, in other words, is to get a query plan that does N reads within an index.

(With lateral queries and set generators, you can write an equivalent query in one go, but MySQL doesn't offer lateral queries or proper generators.)

#5 @wonderboymusic
2 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

@wonderboymusic
2 years ago

#6 @wonderboymusic
2 years ago

  • Keywords has-patch commit added; needs-patch removed
  • Milestone changed from Future Release to 4.3
  • Owner set to pento
  • Status changed from new to assigned

31071.diff adds the index, I think I did it correctly. @pento, please code-review this.

To recap:
When you have a LOT of attachments, these queries require what is essentially a full table scan because there is no index containing post_mime_type. I tested this by adding over 1 million rows to the post table with attachment as post_type and a variety of image mime-types as post_mime_type. I then changed ONE (1) row to have a post_mime_type of audio/mp3 at ID 700,000.

Before index:
Queries took ~2.5-3 seconds each

After index:
Queries took 0.7 milliseconds each

#7 @pento
2 years ago

Because it's a new index, there's no need to put it in upgrade.php - we just need to modify schema.php, bump the db_version, and dbDelta() will take care of it.

Given the purpose of this query, I'm fine with that performance test.

I am a little concerned about the report in comment #1, that adding this index causes other queries to run slow. @archon810, what version of MySQL were you seeing this in? Are you still able to reproduce it, without the extra (post_type,post_date) index?

#8 @archon810
2 years ago

@pento I was on 5.6.17 and 5.5.31. I'm removing post_type_date just for you guys to test this out again.

#9 @archon810
2 years ago

Yeah, it's still a problem for me, on both of the above versions.

Without post_type_date:

explain SELECT SQL_NO_CACHE DISTINCT YEAR( post_date ) AS year, MONTH( post_date ) AS month
		FROM wp_posts 
		WHERE post_type = 'attachment'
		ORDER BY post_date DESC;

+----+-------------+----------+------+------------------------------------------------+---------------------+---------+-------+--------+---------------------------------------------------------------------+
| id | select_type | table    | type | possible_keys                                  | key                 | key_len | ref   | rows   | Extra                                                               |
+----+-------------+----------+------+------------------------------------------------+---------------------+---------+-------+--------+---------------------------------------------------------------------+
|  1 | SIMPLE      | wp_posts | ref  | type_status_date,post_date,post_type_mime_type | post_type_mime_type | 62      | const | 150322 | Using index condition; Using where; Using temporary; Using filesort |
+----+-------------+----------+------+------------------------------------------------+---------------------+---------+-------+--------+---------------------------------------------------------------------+
1 row in set

mysql> show profiles;
+----------+------------+--------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| Query_ID | Duration   | Query                                                                                                                                                              |
+----------+------------+--------------------------------------------------------------------------------------------------------------------------------------------------------------------+
|        1 | 1.74982625 | SELECT SQL_NO_CACHE DISTINCT YEAR( post_date ) AS year, MONTH( post_date ) AS month
		FROM wp_posts
		WHERE post_type = 'attachment'
		ORDER BY post_date DESC |
+----------+------------+--------------------------------------------------------------------------------------------------------------------------------------------------------------------+
1 row in set

#10 follow-up: @pento
2 years ago

  • Keywords commit removed

Thank you for confirming that, @archon810. :-)

@wonderboymusic: Until we have a solid fix for that query going bad (I'd prefer a query rewrite over index tweaks, so that we're not making too many index changes), let's hold off on this new index.

#11 @archon810
2 years ago

  • Keywords commit added

With post_type_date:

mysql> explain SELECT SQL_NO_CACHE DISTINCT YEAR( post_date ) AS year, MONTH( post_date ) AS month
		FROM wp_posts 
		WHERE post_type = 'attachment'
		ORDER BY post_date DESC;
+----+-------------+----------+------+---------------------------------------------------------------+----------------+---------+-------+--------+-----------------------------------------------------------+
| id | select_type | table    | type | possible_keys                                                 | key            | key_len | ref   | rows   | Extra                                                     |
+----+-------------+----------+------+---------------------------------------------------------------+----------------+---------+-------+--------+-----------------------------------------------------------+
|  1 | SIMPLE      | wp_posts | ref  | type_status_date,post_date,post_type_mime_type,post_type_date | post_type_date | 62      | const | 177603 | Using where; Using index; Using temporary; Using filesort |
+----+-------------+----------+------+---------------------------------------------------------------+----------------+---------+-------+--------+-----------------------------------------------------------+
1 row in set
mysql> show profiles;
+----------+------------+----------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| Query_ID | Duration   | Query                                                                                                                                                                      |
+----------+------------+----------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
|        3 | 0.58336925 | SELECT SQL_NO_CACHE DISTINCT YEAR( post_date ) AS year, MONTH( post_date ) AS month
		FROM wp_posts
		WHERE post_type = 'attachment'
		ORDER BY post_date DESC         |
+----------+------------+----------------------------------------------------------------------------------------------------------------------------------------------------------------------------+

It's not ideal, but the query in this case runs 3x faster. I've seen cases where the query without post_type_date present ran in 10+s though, depending on server conditions and caching, whereas with post_type_date, it seems to be pretty consistently fast (still not fast enough, which is why I think it should be abolished).

#12 @archon810
2 years ago

  • Keywords commit removed

This ticket was mentioned in Slack in #core by jorbin. View the logs.


22 months ago

#14 in reply to: ↑ 10 ; follow-up: @jorbin
22 months ago

  • Milestone changed from 4.3 to Future Release

As pento points out, we need a solid fix for the query going bad. Punting from 4.3 while that solution is still being worked on.

#15 in reply to: ↑ 14 @bigmike7801
20 months ago

Replying to jorbin:

As pento points out, we need a solid fix for the query going bad. Punting from 4.3 while that solution is still being worked on.

Is there a way for us to disable these queries in the mean time using something like pre_get_posts?

#16 @philipjohn
18 months ago

  • Keywords dev-feedback added

After chatting with @pento I worked on this today with @thomaswigley and we've improved the queries here. The patch I'm about to attach does the following:

  • Grabs a list of mime types
  • Filters that list for audio* and video*
  • Uses IN instead of LIKE for the query, along with the list of mime types

In my testing on one of the sites affected by the slowness on WordPress.com the queries went from taking ~700ms down to 0.3ms.

The IN portion of the query probably needs escaping - feedback appreciated. ($wpdb->prepare?)

Note that the months query is still bad regardless but we should probably deal with that separately, and get these mime_type queries improved as a first step. (I'm happy to work on the months query also).

@philipjohn
18 months ago

Fix the mime type queries

#17 @archon810
18 months ago

Very nice improvements! Keep it up @philipjohn.

#18 @pento
18 months ago

  • Keywords dev-feedback removed

I like this direction alot.

For escaping, "'" . implode( "', '", esc_sql( $audio_mime_types ) ) . "'" will do the job.

For performance testing, please test with MySQL 5.0 and 5.7, with InnoDB and MyISAM. If there are any discrepancies, we can dig deeper into differences between versions or engines.

#19 @philipjohn
18 months ago

Thanks @archon810 !

I've tested with MySQL 5.7 and both InnoDB and MyISAM on trunk with the patch applied and have seen no issues. Both queries are consistently taking fewer than 0.5ms to complete.

However, I'm struggling to test on 5.0. I'm mostly trying to change the version within VVV. If anyone can point me to a reliable method for running tests on versions as old as 5.0 that'd be appreciated.

@philipjohn
18 months ago

Updated patch includes query escaping and working mime types array

#20 @pento
18 months ago

I mentioned this in Slack, but for Trac posterity: I use MySQL Sandbox for installing different versions of MySQL.

@philipjohn, could you please upload the script you're using for performance testing to this ticket?

#21 @philipjohn
18 months ago

I tested 5.7 by upgrading MySQL from within the VVV VM, rather than a script, and checked query performance using Query Monitor. I'm as yet unsuccessful in getting a 5.0.x version of MySQL set up with MySQL sandbox so I've not yet tested that I'm afraid.

#22 @philipjohn
18 months ago

Further testing makes me suspect the patch isn't making a material difference. There's also a curious difference between the speed of the audio and video queries.

On a live site with a substantial posts table I've seen the following:

Without patch:

  • audio query: ~3ms
  • video query: ~700ms

With patch:

  • audio query: ~3ms
  • video query: ~700-2000ms

Both queries return an empty result set.

So;

  1. My patch does nothing to improve the queries
  2. It's not clear to me why the video query is consistently much slower

I've also attempted to use a more convoluted "OR" query as opposed to "IN" for the video query but there was no difference in speed.

Where does this leave us? I'm not sure of other ways we could improve this query, so does that bring us back to indexes?

Last edited 18 months ago by philipjohn (previous) (diff)

#23 @wonderboymusic
18 months ago

because it's finding audio files before video files when doing the full table scan - purely coincidental based on the order they were added

#24 @philipjohn
18 months ago

Both queries return an empty result set.

This was incorrect - the audio query I'm testing doesn't return an empty result set, but the video one does.

Trying with other sites I'm seeing the queries that return an empty result as very slow. If there *are* results, the query is nice and quick.

Does that point us back to the index as a solution?

#25 @pento
18 months ago

Because the query doesn't have an ORDER BY, it returns as soon as it finds the first result, via table scan. If there are no results, it needs to scan the entire table to determine that.

To avoid that scan, I suspect an index is going to be the solution.

That brings us back to the previous problem, adding this index causes MySQL to incorrectly choose it for this query, over the type_status_date index:

SELECT DISTINCT YEAR( post_date ) AS year, MONTH( post_date ) AS month
FROM $wpdb->posts
WHERE post_type = 'attachment'
ORDER BY post_date DESC

It's not using the index we want it to, because the query doesn't reference the post_status column.

If we rewrite the query like so, MySQL should choose the correct index:

SELECT DISTINCT YEAR( post_date ) AS year, MONTH( post_date ) AS month
FROM $wpdb->posts
WHERE post_type = 'attachment'
AND post_status = 'inherit'
ORDER BY post_date DESC

@archon810, can you see if that query works better after removing your post_type_date index?

#26 @jamesmehorter
17 months ago

  • Keywords dev-feedback reporter-feedback 2nd-opinion added; has-patch removed

Hi everyone,

I created a new multi-column index for post_type and post_mime_type as done by @archon810; and I can confirm A) the audio and video queries are now blazing fast. And B) per @pento's request I also tested adding a post_status clause in the months query; the addition of the post_status clause does indeed ensure the query utilizes the type_status_date index (not the new mime type index) and does not require a filesort. However, please note that the filesort was used even before the new index was added. That query is simply able to run without a filesort when a post_status clause is present. Regardless of what we do with indexes that query should be optimized to include a post_status clause and a caching layer.

mysql  Ver 14.14 Distrib 5.5.46, for debian-linux-gnu (i686) using readline 6.2

mysql> alter table wp_14_posts add index type_mime (post_type, post_mime_type);

mysql> show indexes from wp_14_posts;
+-------------+------------+------------------+--------------+----------------+-----------+-------------+----------+--------+------+------------+---------+---------------+
| Table       | Non_unique | Key_name         | Seq_in_index | Column_name    | Collation | Cardinality | Sub_part | Packed | Null | Index_type | Comment | Index_comment |
+-------------+------------+------------------+--------------+----------------+-----------+-------------+----------+--------+------+------------+---------+---------------+
| wp_14_posts |          0 | PRIMARY          |            1 | ID             | A         |     2111190 |     NULL | NULL   |      | BTREE      |         |               |
| wp_14_posts |          1 | type_status_date |            1 | post_type      | A         |          14 |     NULL | NULL   |      | BTREE      |         |               |
| wp_14_posts |          1 | type_status_date |            2 | post_status    | A         |          14 |     NULL | NULL   |      | BTREE      |         |               |
| wp_14_posts |          1 | type_status_date |            3 | post_date      | A         |     1055595 |     NULL | NULL   |      | BTREE      |         |               |
| wp_14_posts |          1 | type_status_date |            4 | ID             | A         |     2111190 |     NULL | NULL   |      | BTREE      |         |               |
| wp_14_posts |          1 | post_parent      |            1 | post_parent    | A         |      301598 |     NULL | NULL   |      | BTREE      |         |               |
| wp_14_posts |          1 | post_author      |            1 | post_author    | A         |          14 |     NULL | NULL   |      | BTREE      |         |               |
| wp_14_posts |          1 | post_name        |            1 | post_name      | A         |     2111190 |      191 | NULL   |      | BTREE      |         |               |
| wp_14_posts |          1 | type_mime        |            1 | post_type      | A         |         165 |     NULL | NULL   |      | BTREE      |         |               |
| wp_14_posts |          1 | type_mime        |            2 | post_mime_type | A         |         165 |     NULL | NULL   |      | BTREE      |         |               |
+-------------+------------+------------------+--------------+----------------+-----------+-------------+----------+--------+------+------------+---------+---------------+

mysql> EXPLAIN SELECT DISTINCT YEAR( post_date ) AS year, MONTH( post_date ) AS month FROM wp_14_posts WHERE post_type = 'attachment' ORDER BY post_date DESC;
+----+-------------+-------------+------+----------------------------+------------------+---------+-------+--------+-----------------------------------------------------------+
| id | select_type | table       | type | possible_keys              | key              | key_len | ref   | rows   | Extra                                                     |
+----+-------------+-------------+------+----------------------------+------------------+---------+-------+--------+-----------------------------------------------------------+
|  1 | SIMPLE      | wp_14_posts | ref  | type_status_date,type_mime | type_status_date | 82      | const | 969628 | Using where; Using index; Using temporary; Using filesort |
+----+-------------+-------------+------+----------------------------+------------------+---------+-------+--------+-----------------------------------------------------------+

mysql> EXPLAIN SELECT DISTINCT YEAR( post_date ) AS year, MONTH( post_date ) AS month FROM wp_14_posts WHERE post_type = 'attachment' AND post_status = 'inherit' ORDER BY post_date DESC;
+----+-------------+-------------+------+----------------------------+------------------+---------+-------------+--------+-------------------------------------------+
| id | select_type | table       | type | possible_keys              | key              | key_len | ref         | rows   | Extra                                     |
+----+-------------+-------------+------+----------------------------+------------------+---------+-------------+--------+-------------------------------------------+
|  1 | SIMPLE      | wp_14_posts | ref  | type_status_date,type_mime | type_status_date | 164     | const,const | 969628 | Using where; Using index; Using temporary |
+----+-------------+-------------+------+----------------------------+------------------+---------+-------------+--------+-------------------------------------------+

That said, I think this may be the wrong direction for this ticket.. hear me out :)

Our current index setup is not intuitive or performant. We've got a mix of single-column indexes and multi-column indexes, which do not include some commonly used columns—ideally we should have an index for any column used in a where or order by statement.

As shown with the issue above (mysql using the wrong index), we're relying on mysql's index merge optimization to choose which indexes it can use instead of structuring our posts table in a way that instructs mysql.

This may be slightly radical.. but what if we considered replacing all of our current indexes with one, single multi-column index. I've tested this structure, and it speeds up the queries mentioned here (including the months query).. and probably many more. Doing so would requires no changes to any queries as @pento pointed out (though these should still be adjusted to include a post_status clause as mentioned above). And using a single multi-column index has been proven to be quicker than multiple indexes (https://www.percona.com/blog/2014/01/03/multiple-column-index-vs-multiple-indexes-with-mysql-56/).

The only downside I can see is the time it would take to build the new index during an update routine. I clocked the index creation on a table with 2.2m posts at 1min (code/output below). And during that time reads and writes are blocked by default. However, since mysql 5.6 the add index command can be instructed to operate asynchronously, allowing reads and writes while the index is created with the algorithm and lock arguments (http://stackoverflow.com/a/21842589/655837).

Thoughts? Once we agree on an approach I'm happy to help supply some patches.

mysql> alter table wp_14_posts add index posts (ID, post_author, post_date, post_status, comment_status, ping_status, post_name, post_modified, post_parent, menu_order, post_type, post_mime_type, comment_count);
Query OK, 0 rows affected, 2 warnings (1 min 1.33 sec)
Records: 0  Duplicates: 0  Warnings: 2

mysql> show indexes from wp_14_posts;
+-------------+------------+----------+--------------+----------------+-----------+-------------+----------+--------+------+------------+---------+---------------+
| Table       | Non_unique | Key_name | Seq_in_index | Column_name    | Collation | Cardinality | Sub_part | Packed | Null | Index_type | Comment | Index_comment |
+-------------+------------+----------+--------------+----------------+-----------+-------------+----------+--------+------+------------+---------+---------------+
| wp_14_posts |          0 | PRIMARY  |            1 | ID             | A         |     1697812 |     NULL | NULL   |      | BTREE      |         |               |
| wp_14_posts |          1 | posts    |            1 | ID             | A         |         186 |     NULL | NULL   |      | BTREE      |         |               |
| wp_14_posts |          1 | posts    |            2 | post_author    | A         |         186 |     NULL | NULL   |      | BTREE      |         |               |
| wp_14_posts |          1 | posts    |            3 | post_date      | A         |         186 |     NULL | NULL   |      | BTREE      |         |               |
| wp_14_posts |          1 | posts    |            4 | post_status    | A         |         186 |     NULL | NULL   |      | BTREE      |         |               |
| wp_14_posts |          1 | posts    |            5 | comment_status | A         |         186 |     NULL | NULL   |      | BTREE      |         |               |
| wp_14_posts |          1 | posts    |            6 | ping_status    | A         |         186 |     NULL | NULL   |      | BTREE      |         |               |
| wp_14_posts |          1 | posts    |            7 | post_name      | A         |         186 |      191 | NULL   |      | BTREE      |         |               |
| wp_14_posts |          1 | posts    |            8 | post_modified  | A         |         186 |     NULL | NULL   |      | BTREE      |         |               |
| wp_14_posts |          1 | posts    |            9 | post_parent    | A         |         186 |     NULL | NULL   |      | BTREE      |         |               |
| wp_14_posts |          1 | posts    |           10 | menu_order     | A         |         186 |     NULL | NULL   |      | BTREE      |         |               |
| wp_14_posts |          1 | posts    |           11 | post_type      | A         |         186 |     NULL | NULL   |      | BTREE      |         |               |
| wp_14_posts |          1 | posts    |           12 | post_mime_type | A         |         186 |     NULL | NULL   |      | BTREE      |         |               |
| wp_14_posts |          1 | posts    |           13 | comment_count  | A         |         186 |     NULL | NULL   |      | BTREE      |         |               |
+-------------+------------+----------+--------------+----------------+-----------+-------------+----------+--------+------+------------+---------+---------------+

#27 @archon810
17 months ago

I'm a fan of multi-column indexes and use them whenever possible, but it's not so simple in complex applications because order matters in multi-column indexes, and crafting a single index is usually not possible unless all the queries are relatively simple and use the same fields in a similar manner.

The art of index creation is very delicate and sometimes just flipping the order of two columns in an index can result in a 10x performance gain, even though you thought the original index was already pretty awesome.

I wish a single multi-column index would be a panacea for all trouble related to query optimization, but you will soon find that it's not the case, otherwise everyone would just include that index by default.

#28 @jamesmehorter
17 months ago

Thanks for pointing that out @archon810! Would you agree that a patch for this ticket should then contain:

1) The post_type, post_mime_type index added the way @pento suggested (https://core.trac.wordpress.org/ticket/31071#comment:7)

2) The addition of "AND post_status = 'inherit'" to the months query. I see 3 variations on that query:
wp-admin/export.php:134, wp-admin/includes/class-wp-list-table.php:563, and wp-includes/media.php:3213

3) If agreeable, I'd like to also add a caching layer around each of the audio, video, and months queries.

Last edited 17 months ago by jamesmehorter (previous) (diff)

#29 @tha_sun
17 months ago

  • Keywords has-patch added
  • Summary changed from post_mime_type related queries still slow on larger sites to media / post_mime_type related queries are very slow on larger sites

This issue causes two slow queries with a duration of 12 seconds (each) on one of our latest sites currently, on every load of the administrative post add/edit page. The site merely has 40k posts.

I agree that a multi-column index is not the right solution, because column order matters (a lot). That's why the other mentioned query immediately cropped up.

Replacing the LIKE with discrete values does not help either, because there is no index for the column, so the table scan happens anyway. That's expected.

Caching results in individual transients is a terrible idea for use-cases like wp_enqueue_media(), because caching application state (affecting the user interface) in a magic transient adds a complex layer of abstraction, for which WordPress core and especially the vast majority of contrib plugins and themes do not seem to be prepared at this point in time.

I agree on 1) we simply need an index. But the index can and should be optimized in size for its primary use-case by limiting its length.

We don't need to modify the query as suggested in 2) because in the rare case the MySQL query optimizer chooses a wrong index, there's a native solution for that: index hints, as already demonstrated in #comment:1.

And we don't want 3) for reasons explained above.

Therefore, attached patch does the following:

  1. Adds an index to wp_posts for post_type, post_mime_type.
  2. Increases $wp_db_version, so the schema change is picked up by dbDelta().
  3. Instructs MySQL's query optimizer to IGNORE INDEX (post_type_mime_type). Not explicitly suggesting the old one, because it's not the most ideal and a custom index might have been added, which MySQL should still choose. Thus, we just ignore this new index.

Any chance to move forward with this?

This ticket was mentioned in Slack in #core by philipjohn. View the logs.


17 months ago

#31 @tha_sun
17 months ago

Is there anything else I can do to help move this issue forward?

#32 @philipjohn
17 months ago

@tha_sun If you could provide some benchmarking data that shows the patch improves the situation, I think that'd really help us to get this through.

I'm as keen as you to see this resolved :)

#33 @jamesmehorter
17 months ago

@tha_sun Using IGNORE INDEX (post_type_mime_type) seems like it could fail in the future if other indexes are added. I'd rather see a hint to USE INDEX or a post_status clause as @pento suggested.

#34 @AramZS
16 months ago

@philipjohn What would be the best methodology to test this to report on the results? I've got a very large database on a non production server that would be perfect for it.

#35 @tha_sun
16 months ago

Sorry for the delay. I'll try to provide numbers (EXPLAIN before vs. after) based on the latest patch + the most recent site that suffers from this issue (again!) as soon as possible. If anyone else beats me to it, go for it!

@jamesmehorter:

Using IGNORE INDEX (post_type_mime_type) seems like it could fail in the future if other indexes are added. I'd rather see a hint to USE INDEX or a post_status clause as @pento suggested.

Happy to adjust if others agree with your stance, but as mentioned before, the current index that MySQL happens to choose is NOT the most ideal index for the query in question.

The chance of a random site owner (intentionally) adding a custom index that happens to be the ideal index for this query is equal to the chance of WordPress Core adding a new index that (unintentionally) happens to be preferred by the MySQL query optimizer (again). Both chances are pretty much close to zero, so I do accept your bets :-)

I'd go with USE INDEX if there was an ideal index to use. But WordPress Core does not ship with an ideal index, so IGNORE INDEX is the most logical thing to do. No?

#36 @jamesmehorter
16 months ago

Hey @tha_sun, thanks for pushing this forward some more..

Using IGNORE INDEX (post_type_mime_type) seems like it could fail in the future if other indexes are added. I'd rather see a hint to USE INDEX or a post_status clause as @pento suggested.

[...] the current index that MySQL happens to choose is NOT the most ideal index for the query in question. [...] The chance of a random site owner (intentionally) adding a custom index that happens to be the ideal index for this query is equal to the chance of WordPress Core adding a new index that (unintentionally) happens to be preferred by the MySQL query optimizer (again).

Hehe, that's a lot of chances :) We're adding a new index now, and I suppose there's always a chance we could choose to add another index in the future. Also, to your point, MySQL is choosing the wrong index—to me it makes sense for us to specify exactly which index to use. Though, like you I'll side with the majority here; that was just a suggestion on how I'd personally tackle the problem.

#37 @sboisvert
13 months ago

What are the thoughts on having a temporary solution in the form of a filter that enables the bypass of those queries?

Something like
https://github.com/WordPress/WordPress/pull/190/files
and https://core.trac.wordpress.org/attachment/ticket/31071/media.php.diff

@jamesmehorter
13 months ago

Adds an index on the post_type and post_mime_type columns

@jamesmehorter
13 months ago

Spreadsheet of affected queries before/after new index

#38 follow-up: @jamesmehorter
13 months ago

I tested these 3 queries on a table with 1.1m rows in MySQL 5.0.95, 5.5.31, and 5.6.17 (The versions @archon810 was using) with and without the new post_type/mime_type index. Those tests are documented here: (and attached to this ticket below) https://docs.google.com/spreadsheets/d/1F45r8niIqw5S7XHLA-i2M1nTzuO592QtyOO6HcEgUFs/edit?usp=sharing

However, the months/years query uses the type_status_date index regardless—The incorrect index usage @archon810 noticed in (comment 9) is not reproducible for me. @archon810 any chance that was a mistake? Mind sharing the data you're using?

It appears the months/years query is slow regardless of the new index—and unless @archon810 is able to provide reproducible steps I'm thinking we should ignore that query and proceed with the new index. As such, I've also attached a patch which simply adds the new index. Thoughts?

Patch: https://core.trac.wordpress.org/attachment/ticket/31071/31071-d.diff
Query Performance Spreadsheet: https://core.trac.wordpress.org/attachment/ticket/31071/WP%20Core%20Trac%20%2331071%20MySQL%20Queries.xlsx

I also like @sboisvert's idea. Though, I'd personally want them to be cacheable, not just bypassable—i.e. each query would also need an action after the queries execute to cache the results—so the results may be supplied back to those new filters.

Last edited 13 months ago by jamesmehorter (previous) (diff)

#39 @archon810
13 months ago

@jamesmehorter Glad you were able to confirm the original performance issues and much better performance after the new index.

As for the months/years query, I'm currently running MySQL 5.6.28 and I indeed can't reproduce the slowdown anymore. The site is the same, now with even more data. I'm using "IGNORE INDEX" to test what happens if post_type_date is ignored, and I'm no longer seeing post_type_mime_type index being used by MySQL automatically - instead it uses type_status_date.

The query that ignores post_type_date (i.e. the way WP is currently working) returns about 1.5x faster. Why it was using post_type_mime_type during earlier tests is beyond me - the MySQL query optimizer works in mysterious ways. But I wouldn't be surprised if it kicked in for other users with whatever data conditions that were causing post_type_mime_type to be used here before for me.

Results attached.

EXPLAIN SELECT SQL_NO_CACHE DISTINCT
	YEAR (post_date) AS YEAR,
	MONTH (post_date) AS MONTH
FROM
	wp_posts 
WHERE
	post_type = 'attachment'
ORDER BY
	post_date DESC;
+----+-------------+----------+------+---------------------------------------------------------------+----------------+---------+-------+--------+-----------------------------------------------------------+
| id | select_type | table    | type | possible_keys                                                 | key            | key_len | ref   | rows   | Extra                                                     |
+----+-------------+----------+------+---------------------------------------------------------------+----------------+---------+-------+--------+-----------------------------------------------------------+
|  1 | SIMPLE      | wp_posts | ref  | type_status_date,post_date,post_type_mime_type,post_type_date | post_type_date | 62      | const | 192321 | Using where; Using index; Using temporary; Using filesort |
+----+-------------+----------+------+---------------------------------------------------------------+----------------+---------+-------+--------+-----------------------------------------------------------+
1 row in set
EXPLAIN SELECT SQL_NO_CACHE DISTINCT
	YEAR (post_date) AS YEAR,
	MONTH (post_date) AS MONTH
FROM
	wp_posts IGNORE INDEX (post_type_date)
WHERE
	post_type = 'attachment'
ORDER BY
	post_date DESC;
+----+-------------+----------+------+---------------------------------------------------------------+------------------+---------+-------+--------+-----------------------------------------------------------+
| id | select_type | table    | type | possible_keys                                                 | key              | key_len | ref   | rows   | Extra                                                     |
+----+-------------+----------+------+---------------------------------------------------------------+------------------+---------+-------+--------+-----------------------------------------------------------+
|  1 | SIMPLE      | wp_posts | ref  | type_status_date,post_date,post_type_mime_type,post_type_date | type_status_date | 62      | const | 167406 | Using where; Using index; Using temporary; Using filesort |
+----+-------------+----------+------+---------------------------------------------------------------+------------------+---------+-------+--------+-----------------------------------------------------------+
1 row in set

#40 in reply to: ↑ 38 @sboisvert
13 months ago

I also like @sboisvert's idea. Though, I'd personally want them to be cacheable, not just bypassable—i.e. each query would also need an action after the queries execute to cache the results—so the results may be supplied back to those new filters.

The way the filter is setup it needs to be passed a non-false value to continue, this value would most likely come from a cache the user has setup (or an external commenting system hypothetically).

#41 @sboisvert
13 months ago

It would be pretty easy to abstract out the $months array to a separate function that calls it from a transient and we'd delete the transient on new attachment upload.
If people this this is a good idea I can whip up a patch.

#42 follow-up: @jamesmehorter
13 months ago

@archon810 Interesting! Are you then suggesting that we should include a fix for that months/year query regardless? Just in case someone's setup somehow triggers the new index to be used incorrectly—even if we can't reproduce it now?

Anyone else have thoughts on this?

#43 in reply to: ↑ 42 @archon810
13 months ago

Replying to jamesmehorter:

@archon810 Interesting! Are you then suggesting that we should include a fix for that months/year query regardless? Just in case someone's setup somehow triggers the new index to be used incorrectly—even if we can't reproduce it now?

Anyone else have thoughts on this?

Unfortunately, it's tough for me to make that call as I'm not sure if it happens again for anyone. It could be a gamble to see if you start getting reports about it, or forcing a certain index or ignoring a certain other one could be an option.

#44 @tha_sun
13 months ago

Latest results from a new site running into this missing key issue again:

Stats: 800k posts (1.3M rows), 15k users

myslow-slow.log yields every few seconds:

# Query_time: 2.162829  Lock_time: 0.000109 Rows_sent: 40  Rows_examined: 713569
SET timestamp=1460618095;
SELECT SQL_CALC_FOUND_ROWS  wp_posts.ID FROM wp_posts  WHERE 1=1
  AND (wp_posts.post_mime_type LIKE 'image/%')
  AND wp_posts.post_type = 'attachment' AND ((wp_posts.post_status = 'inherit' OR wp_posts.post_status = 'private'))
  ORDER BY wp_posts.post_date DESC LIMIT 0, 40;

Explain original query:

mysql> EXPLAIN SELECT SQL_CALC_FOUND_ROWS  wp_posts.ID FROM wp_posts  WHERE 1=1
    ->   AND (wp_posts.post_mime_type LIKE 'image/%')
    ->   AND wp_posts.post_type = 'attachment' AND ((wp_posts.post_status = 'inherit' OR wp_posts.post_status = 'private'))
    ->   ORDER BY wp_posts.post_date DESC LIMIT 0, 40\G
*************************** 1. row ***************************
           id: 1
  select_type: SIMPLE
        table: wp_posts
         type: ref
possible_keys: type_status_date
          key: type_status_date
      key_len: 82
          ref: const
         rows: 640459
        Extra: Using where; Using filesort
1 row in set (0.00 sec)

Now executing:

mysql> ALTER TABLE wp_posts ADD INDEX post_type_mime_type (post_type, post_mime_type(10));

mysql> SHOW CREATE TABLE wp_posts\G
*************************** 1. row ***************************
       Table: wp_posts
Create Table: CREATE TABLE `wp_posts` (
...
  PRIMARY KEY (`ID`),
  KEY `type_status_date` (`post_type`,`post_status`,`post_date`,`ID`),
  KEY `post_parent` (`post_parent`),
  KEY `post_author` (`post_author`),
  KEY `post_name` (`post_name`(191)),
  KEY `guid` (`guid`(191)),
  KEY `post_type_mime_type` (`post_type`,`post_mime_type`(10))
) ENGINE=InnoDB AUTO_INCREMENT=1364346 DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci
1 row in set (0.00 sec)

still yields:

# Time: 160414  9:30:36
# Query_time: 2.867190  Lock_time: 0.000184 Rows_sent: 40  Rows_examined: 713573
SET timestamp=1460619036;
SELECT SQL_CALC_FOUND_ROWS  wp_posts.ID FROM wp_posts  WHERE 1=1
  AND (wp_posts.post_mime_type LIKE 'image/%')
  AND wp_posts.post_type = 'attachment' AND ((wp_posts.post_status = 'inherit' OR wp_posts.post_status = 'private'))
  ORDER BY wp_posts.post_date DESC LIMIT 0, 40;

because the new index was not actually used:

mysql> EXPLAIN SELECT SQL_CALC_FOUND_ROWS  wp_posts.ID FROM wp_posts  WHERE 1=1
    ->   AND (wp_posts.post_mime_type LIKE 'image/%')
    ->   AND wp_posts.post_type = 'attachment' AND ((wp_posts.post_status = 'inherit' OR wp_posts.post_status = 'private'))
    ->   ORDER BY wp_posts.post_date DESC LIMIT 0, 40\G
*************************** 1. row ***************************
           id: 1
  select_type: SIMPLE
        table: wp_posts
         type: ref
possible_keys: type_status_date,post_type_mime_type
          key: type_status_date
      key_len: 82
          ref: const
         rows: 640639
        Extra: Using where; Using filesort
1 row in set (0.00 sec)

Now forcing its usage via USE INDEX:

mysql> EXPLAIN SELECT SQL_CALC_FOUND_ROWS  wp_posts.ID FROM wp_posts
  USE INDEX (post_type_mime_type)
  WHERE 1=1
  AND (wp_posts.post_mime_type LIKE 'image/%')
  AND wp_posts.post_type = 'attachment' AND ((wp_posts.post_status = 'inherit' OR wp_posts.post_status = 'private'))
  ORDER BY wp_posts.post_date DESC LIMIT 0, 40\G
*************************** 1. row ***************************
           id: 1
  select_type: SIMPLE
        table: wp_posts
         type: ref
possible_keys: post_type_mime_type
          key: post_type_mime_type
      key_len: 82
          ref: const
         rows: 689670
        Extra: Using where; Using filesort
1 row in set (0.01 sec)

mysql> SELECT SQL_CALC_FOUND_ROWS  wp_posts.ID FROM wp_posts
  USE INDEX (post_type_mime_type)
  WHERE 1=1
  AND (wp_posts.post_mime_type LIKE 'image/%')
  AND wp_posts.post_type = 'attachment' AND ((wp_posts.post_status = 'inherit' OR wp_posts.post_status = 'private'))
  ORDER BY wp_posts.post_date DESC LIMIT 0, 40\G
...
40 rows in set (2.14 sec)

So back to square one...

mysql> ALTER TABLE wp_posts DROP INDEX post_type_mime_type;
Query OK, 0 rows affected (0.08 sec)
Records: 0  Duplicates: 0  Warnings: 0

mysql> ALTER TABLE wp_posts ADD INDEX post_type_mime_type_status  (post_mime_type(10), post_type, post_status);
Query OK, 0 rows affected (12.90 sec)
Records: 0  Duplicates: 0  Warnings: 0

mysql> EXPLAIN SELECT SQL_CALC_FOUND_ROWS  wp_posts.ID FROM wp_posts  WHERE 1=1
  AND (wp_posts.post_mime_type LIKE 'image/%')
  AND wp_posts.post_type = 'attachment' AND ((wp_posts.post_status = 'inherit' OR wp_posts.post_status = 'private'))
  ORDER BY wp_posts.post_date DESC LIMIT 0, 40\G
*************************** 1. row ***************************
           id: 1
  select_type: SIMPLE
        table: wp_posts
         type: ref
possible_keys: type_status_date,post_type_mime_type_status
          key: type_status_date
      key_len: 82
          ref: const
         rows: 640817
        Extra: Using where; Using filesort
1 row in set (0.00 sec)

mysql> ALTER TABLE wp_posts DROP INDEX post_type_mime_type_status;
Query OK, 0 rows affected (0.06 sec)
Records: 0  Duplicates: 0  Warnings: 0

mysql> ALTER TABLE wp_posts ADD INDEX type_status_mime_type (post_type, post_status, post_mime_type(10));
Query OK, 0 rows affected (12.52 sec)
Records: 0  Duplicates: 0  Warnings: 0

mysql> EXPLAIN SELECT SQL_CALC_FOUND_ROWS  wp_posts.ID FROM wp_posts  WHERE 1=1
  AND (wp_posts.post_mime_type LIKE 'image/%')
  AND wp_posts.post_type = 'attachment' AND ((wp_posts.post_status = 'inherit' OR wp_posts.post_status = 'private'))
  ORDER BY wp_posts.post_date DESC LIMIT 0, 40\G
*************************** 1. row ***************************
           id: 1
  select_type: SIMPLE
        table: wp_posts
         type: ref
possible_keys: type_status_date,type_status_mime_type
          key: type_status_date
      key_len: 82
          ref: const
         rows: 640817
        Extra: Using where; Using filesort
1 row in set (0.00 sec)

Sad panda. Looks like the server is running into some general resource limitations that I need to look into first.

#45 @sboisvert
12 months ago

Since for most sites there are more /wp-admin/ pageloads than file uploads and the index solution is proving more problematic than expected I'd suggest we fix the problem with caching.

Maybe something like this:
wp-includes\media.php ~3255

<?php
$has_audio = get_transient( 'media_has_audio' );
if ( false === $has_audio ){
        $has_audio = $wpdb->get_var( "
        SELECT ID
        FROM $wpdb->posts
        WHERE post_type = 'attachment'
        AND post_mime_type LIKE 'audio%'
        LIMIT 1
" );
        set_transient( $has_audio, 'media_has_audio' );
}

$has_video = get_transient( 'media_has_video' );
if ( false === $has_video ) {
        $has_video = $wpdb->get_var( "
                SELECT ID
                FROM $wpdb->posts
                WHERE post_type = 'attachment'
                AND post_mime_type LIKE 'video%'
                LIMIT 1
        " );
        set_transient( $has_video, 'media_has_video' );
}

$months = get_transient( 'media_months_array' );
if ( false === $months ) {
        $months = $wpdb->get_results( $wpdb->prepare( "
                SELECT DISTINCT YEAR( post_date ) AS year, MONTH( post_date ) AS month
                FROM $wpdb->posts
                WHERE post_type = %s
                ORDER BY post_date DESC
        ", 'attachment' ) );
        set_transient( $months, 'media_months_array' );
}

Note that get_results and get_var return null when nothing is found, this means that we handle the not found case (which is often the slowest one for the has_* queries).

We'd then need to add something like:

<?php
add_filter( 'wp_handle_upload', 'media_clear_caches', 10, 2 );
function media_clear_caches( $file_array, $type ) {
        delete_transient( 'media_months_array' );
        if ( 'video' == $file_array['type'] ){
                delete_transient( 'media_has_video' );
        } else if ( 'audio' == $file_array['type'] ) {
                delete_transient( 'media_has_audio' );
        }
}

Or just directly have the delete_transient inside _wp_handle_upload()

If there's interest we can test the patch on our end.

#46 follow-up: @mintindeed
12 months ago

I think the index is a more sound strategy. Logically I don't know why this index would cause more problems than any other index. There's also the idea that people are no more worse off with the index, and many will be better off, and those who are having problems with the index can help gather more data about why it doesn't work for some people.

If that's not enough, then I'd suggest instead of using a transient, set a flag. You'll know when an audio or video file gets uploaded, so you can unset the flag then. When deleting an audio or video file, you can then check to see if it's the only one left in the media library and re-set the flag at that time. Upgrades can check the media library during the database upgrade operation and set the initial flag accordingly.

#47 in reply to: ↑ 46 @sboisvert
12 months ago

Replying to mintindeed:

I think the index is a more sound strategy.

I'd tend to agree but from some of the reports up top there it's not always used efficiently by MySQL and I would rather perfect not be the enemy of better :).

If that's not enough, then I'd suggest instead of using a transient, set a flag.

Sure, I don't see a big difference either way. My reasoning for the transient was that on busy sites that have a object cache it would probably be faster than doing a separate db request, unless we make it an auto-load option (which I personally try to avoid)

#48 @STRML
7 months ago

The slow YEAR() and MONTH() in the second query is absolutely killing performance on a production website I maintain. We're on a host where we're unable to hack core. The transient solution is fitting in the absence of generating columns.

#49 @rickalee
4 months ago

Any hope of getting a filter or transient for this? Running up against 2 second query for site with over a million posts.

@sboisvert solution looks like a great start.

#50 @GunGeekATX
4 months ago

Was looking at this myself today and wonder if this might be part of the solution:

$defaults = array(
	'post' => null,
	'has_media' => null,
	'has_video' => null,
);
$args = wp_parse_args( $args, $defaults );

if ( null === $args['has_audio'] ) {
	// Run get_var() here.
}

If we allow the developer to pass in a true/false value for those (or make the args filterable), the database queries could be skipped altogether. On the huge site I'm working on, those values are always going to be true, so there's no need to run the database query, indexed or not. Developers could also handle caching those values themselves if they needed to.

Last edited 4 months ago by GunGeekATX (previous) (diff)

#51 @shakeels
4 months ago

@GunGeekATX That's the same issue that we have here. It's a large site and based on how we are doing things, we know that it's always going to be a certain way. There is no sense in even doing the lookup. A filter on it would probably be an ideal setup for us as well.

I basically ended up using @sboisvert patch that adds a filter. Like in @GunGeekATX case, we always know that the value will be constant because of how we are using it. In that case, for me, it's far easier to just bypass the entire query and just return false. For us, that works right now, but it would be nice to get this into core so I don't have to patch it.

Last edited 4 months ago by shakeels (previous) (diff)

#52 follow-up: @jnylen0
8 weeks ago

I think we should add filters to allow short-circuiting these queries, since there is not an obvious solution that will work for every site (at least it's not obvious to me from the discussion above).

media.php.diff from @sboisvert looks sound to me but needs some stylistic clean-up before commit.

Thoughts?

#53 @jnylen0
7 weeks ago

@wonderboymusic @jorbin any opposition to adding a filter here until we can work out a more complete solution?

#54 in reply to: ↑ 52 @adamsilverstein
7 weeks ago

Replying to jnylen0:

media.php.diff from @sboisvert looks sound to me but needs some stylistic clean-up before commit.

+1 i agree adding filters to enable bypassing these queries makes sense for now. Lets get this patch cleaned up and the filters documented so developers can at least opt out of the queries.

I looked into how these queries are used a bit further. The first two queries are used to set up wp.media.view.settings.attachmentCounts.video/audio that is used to in the 'Add New Media' modal to determine whether the library contains video/audio - adding two additional links 'Create Media Playlist' and Create Video Playlist in the sidebar.

Empty media library:

https://cl.ly/0c221u0m3U0X/Edit_Post__wpdev__WordPress_2017-03-13_16-51-59.jpg_363326_2017-03-13_16-57-58.jpg

W/audio:

https://cl.ly/2n1w3C1M280b/Edit_Post__wpdev__WordPress_2017-03-13_16-54-54.jpg
added a 'Create Audio Playlist' button.

w/video:

https://cl.ly/293D250n3f0m/Edit_Post__wpdev__WordPress_2017-03-13_16-56-10.jpg
added a 'Create Video Playlist' button

Bypassing these and always returning 1 doesn't really have much of an ill effect even when the media type is missing from the library - its similar to going to add a gallery when you have no images in the library. Can we bypass these entirely once a site has a certain number of media items?

Library with no audio, returning 1:

https://cl.ly/1U1V1v311X1i/Edit_Post__wpdev__WordPress_2017-03-13_17-08-23.jpg

Last edited 7 weeks ago by adamsilverstein (previous) (diff)

#55 follow-up: @sboisvert
3 weeks ago

@adamsilverstein

Bypassing these and always returning 1 doesn't really have much of an ill effect even when the media type is missing from the library - its similar to going to add a gallery when you have no images in the library. Can we bypass these entirely once a site has a certain number of media items?

Perhaps then we can remove them completely? It doesn't seem to add much to the UI (it might even confuse folks who are looking for buttons that they saw on another site but they are missing here) And the slowdowns are quite noticeable on many sites.

#56 in reply to: ↑ 55 ; follow-up: @adamsilverstein
3 weeks ago

@sboisvert

Perhaps then we can remove them completely?

I like that suggestion!

When I go to the media library, I'd like to see a 'Create Video Playlist' link that helps me discover that I _can_ create a video playlist. If I click on this link and haven't uploaded any videos, it would be especially helpful if the 'No items found' message was more specific: "Upload some videos to create a video playlist".

Some sites will likely want to keep the old behavior, perhaps they only allow images in the media library, so if we do change the default behavior here, we should still provide a filter for backward compatibility.

@jnylen0
3 weeks ago

Allow overriding slow media queries via filters

#57 follow-up: @jnylen0
3 weeks ago

  • Keywords needs-testing added; dev-feedback reporter-feedback removed
  • Milestone changed from Future Release to 4.7.4

To me, 31071.2.diff is the obvious change to make here: it preserves the default behavior while adding a way to customize it in any way desired. It's based on media.php.diff from @sboisvert with a couple of changes:

  • Filters default to null so that they can be overridden either way (the filters can decide whether to show or hide the "create playlist" buttons).
  • Filters have documentation.

If there is a consensus to always show the "create playlist" buttons, I'm not strongly oppposed, but it does make sense to me to avoid showing links to create audio/video playlists if we know they can't be used.

The patch is written for version 4.7.5 - I think we can get the filters in by then. If we do that, though, we should probably still leave this ticket open afterwards pending a more complete solution.

I'm milestoning as 4.7.4 for now because there's no 4.7.5 milestone yet, whether this gets into 4.7.4 or not is up to @swissspidy.

#58 in reply to: ↑ 57 @adamsilverstein
3 weeks ago

Seems reasonable, although perhaps not obvious. +1.

#59 @swissspidy
3 weeks ago

31071.2.diff looks reasonable as well. The inline docs might need some small improvements (e.g. @return bool|null Whether instead of @return bool|null whether …). See https://make.wordpress.org/core/handbook/best-practices/inline-documentation-standards/php/.

@jnylen0
3 weeks ago

Documentation improvements

#60 @jnylen0
3 weeks ago

  • Keywords 2nd-opinion removed

31071.3.diff improves the inline documentation and changes the @since version to 4.7.4.

#61 @wonderboymusic
3 weeks ago

+1 on latest patch

#62 @jnylen0
3 weeks ago

In 40382:

Media: Add filters to allow overriding slow media queries.

There are a couple of queries that do a full table scan of attachment posts to support features of the media library. Pending a more complete solution, allow overriding these queries via filters.

Props sboisvert, jnylen0.
See #31071.

#63 @jnylen0
3 weeks ago

  • Keywords fixed-major added; needs-testing removed

Thanks for the review. Adding the fixed-major keyword for 4.7.4 consideration, but let's leave this ticket open afterwards because this still isn't really solved.

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


3 weeks ago

#65 @ocean90
3 weeks ago

Filter docs don't have a @return tag, that should be @param instead. I think we also try using null === $var instead of is_null( $var )

#66 in reply to: ↑ 56 @joemcgill
3 weeks ago

Replying to adamsilverstein:

@sboisvert

Perhaps then we can remove them completely?

I like that suggestion!

When I go to the media library, I'd like to see a 'Create Video Playlist' link that helps me discover that I _can_ create a video playlist. If I click on this link and haven't uploaded any videos, it would be especially helpful if the 'No items found' message was more specific: "Upload some videos to create a video playlist".

I tend to agree with this. It seems like a bad idea to run extra queries just for the purpose of showing some UI here. Additionally, I like the idea of showing the UI elements even when there aren't any media items for discoverability as @adamsilverstein suggests.

As we discussed in today's Media meeting, the filters are probably still useful for anyone who wants to hide these elements but the names should be less generic. We mentioned something like media_library_show_(audio|video)_playlists.

Adding the filters for 4.7.4 is probably fine, but removing queries entirely should probably soak in Trunk a bit longer.

@jnylen0
3 weeks ago

Further improvements for 4.7.4

@jnylen0
3 weeks ago

Fix the issue for 4.8

#67 @jnylen0
3 weeks ago

  • Keywords dev-feedback added; fixed-major removed

Per above review feedback and Slack discussion, I've attached a couple more patches.

31071.4.diff is intended for the 4.7 branch and contains the following changes:

  • Make the filter names more specific.
  • Fix the inline documentation (use @param instead of @return).
  • Use null === instead of is_null to avoid an extra function call.
  • Rename the $has_audio and $has_video variables now that they actually represent something slightly different.

31071.5.diff is intended to fix this issue in the 4.8 branch by skipping these expensive queries by default and always showing the "Create Audio/Video Playlist" buttons.

#68 follow-up: @jnylen0
2 weeks ago

I'd like to commit 31071.4.diff in a couple of days, as we know that what is currently in trunk is not quite what we want. @joemcgill @ocean90 @adamsilverstein can you take a look?

#69 @swissspidy
2 weeks ago

  • Keywords commit added

#70 in reply to: ↑ 68 @adamsilverstein
2 weeks ago

Replying to jnylen0:

I'd like to commit 31071.4.diff in a couple of days,

Looks good to me. The filter names are much better here, also like the new variable names. 31071.5.diff also looks good for 4.8.

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


2 weeks ago

#72 @jnylen0
2 weeks ago

In 40421:

Media: Improve filters that allow overriding slow media queries.

As a follow-up to [40382], this commit makes the following improvements:

  • Make the filter names more specific.
  • Fix the inline documentation (use @param instead of `@return).
  • Use null === instead of is_null to avoid extra function calls.
  • Rename the $has_audio and $has_video variables now that they actually represent something slightly different.

See #31071.

#73 @jnylen0
2 weeks ago

  • Keywords fixed-major added; dev-feedback removed

I think [40382] and [40421] are ready for backport to the 4.7 branch.

Before committing 31071.5.diff for trunk / 4.8, I'd probably prefer to wait until 4.7.4 is out at least, to avoid making things more confusing.

Thoughts on timing there?

#74 @swissspidy
2 weeks ago

In 40425:

Media: Add filters to allow overriding slow media queries.

There are a couple of queries that do a full table scan of attachment posts to support features of the media library. Pending a more complete solution, allow overriding these queries via filters.

Props sboisvert, jnylen0.
See #31071.

Merges [40382] and [40421] to the 4.7 branch.

#75 @swissspidy
2 weeks ago

For the 4.7.4 it would definitely look cleaner if we could close this ticket and open a new one afterwards for the change of behaviour. Or, just go ahead and commit 31071.5.diff to trunk.

#76 @swissspidy
2 weeks ago

  • Owner changed from pento to jnylen0

#77 @jnylen0
2 weeks ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 40430:

Media: Default to always showing the "Create Audio/Video Playlist" buttons.

This avoids a couple of expensive queries that attempt to determine whether the media library contains any audio or video items, and also makes the UI for creating playlists more discoverable.

[40382] and [40421] added filters to allow overriding this behavior; this commit changes the default value of the filters to always show these UI buttons and never run the expensive queries. The old behavior can still be restored using the filters if desired.

Props sboisvert, adamsilverstein, joemcgill, jnylen0.
Fixes #31071.

Note: See TracTickets for help on using tickets.