Make WordPress Core

Opened 18 months ago

Closed 17 months ago

Last modified 17 months ago

#56802 closed defect (bug) (fixed)

Query: Post IDs cached for search and other 'LIKE' queries are unreachable

Reported by: dlh's profile dlh Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: 6.1 Priority: high
Severity: normal Version: 6.1
Component: Query Keywords: has-patch has-unit-tests needs-testing
Focuses: performance Cc:

Description (last modified by dlh)

[53941] added caching for the post ID database query in WP_Query. The cache key for the post IDs is determined by hashing (among other details) the $request property of the query, which contains the generated SQL.

At the time that this cache key is generated, the SQL in $request still contains escaped placeholders from wpdb::add_placeholder_escape(). For example:

	SELECT SQL_CALC_FOUND_ROWS  wp_posts.*
	FROM wp_posts
	WHERE 1=1  AND (((wp_posts.post_title LIKE '{6bf47a1ba3fa507a5e63620d433b08dd0a8c3664854a451088b2fd023cb1d0f0}hello{6bf47a1ba3fa507a5e63620d433b08dd0a8c3664854a451088b2fd023cb1d0f0}') OR (wp_posts.post_excerpt LIKE '{6bf47a1ba3fa507a5e63620d433b08dd0a8c3664854a451088b2fd023cb1d0f0}hello{6bf47a1ba3fa507a5e63620d433b08dd0a8c3664854a451088b2fd023cb1d0f0}') OR (wp_posts.post_content LIKE '{6bf47a1ba3fa507a5e63620d433b08dd0a8c3664854a451088b2fd023cb1d0f0}hello{6bf47a1ba3fa507a5e63620d433b08dd0a8c3664854a451088b2fd023cb1d0f0}')))  AND ((wp_posts.post_type = 'post' AND (wp_posts.post_status = 'publish'
OR wp_posts.post_status = 'private')) OR (wp_posts.post_type = 'page' AND (wp_posts.post_status = 'publish'
OR wp_posts.post_status = 'private')) OR (wp_posts.post_type = 'attachment' AND (wp_posts.post_status = 'publish'
OR wp_posts.post_status = 'private')))

	ORDER BY wp_posts.post_title LIKE '{6bf47a1ba3fa507a5e63620d433b08dd0a8c3664854a451088b2fd023cb1d0f0}hello{6bf47a1ba3fa507a5e63620d433b08dd0a8c3664854a451088b2fd023cb1d0f0}' DESC, wp_posts.post_date DESC
	LIMIT 0, 10

wpdb generates this placeholder once per request using uniqid(), making it unlikely that the placeholder will be the same across multiple requests.

As a result, the same search will generate different cache keys on each request, since the SQL will differ along with the placeholder. The IDs will be cached, but that cache will probably never be found.

From what I can tell, this issue affects search queries (with 's') and meta queries that use 'compare' => 'LIKE' or 'compare_key' => 'LIKE' (or NOT LIKE). Presumably, any other queries that include placeholders and use wpdb::prepare() would also be affected.

I'm not quite sure what the best course for the issue would be. My first thought was to expand, in WP_Query,

$id_query_is_cacheable = ! str_contains( strtoupper( $orderby ), ' RAND(' );

to include

&& ! str_contains( $this->request, $wpdb->placeholder_escape() )

However, calling wpdb::placeholder_escape() has a side-effect of adding a filter to query, and that might not be desirable.

Perhaps a naive check like ! str_contains( $this->request, " LIKE '{" ) would do the trick, but it's possible that that would catch unrelated queries.

Change History (39)

#1 @dlh
18 months ago

  • Description modified (diff)

#2 @peterwilsoncc
18 months ago

  • Focuses performance added
  • Milestone changed from Awaiting Review to 6.1

Thanks David. As caching was added during the current release cycle, I've moved it on to the milestone for investigation.

The placeholder value is stored in a static variable so I am not sure how to test for this.

The issue is limited to sites with a persistent cache, potentially stuffing them with keys they can never access again.

I like the options you provide: protentially the best way forward is to skip LIKE queries for now and revisit in 6.2.

cc @spacedmonkey

This ticket was mentioned in PR #3448 on WordPress/wordpress-develop by @spacedmonkey.


18 months ago
#3

  • Keywords has-patch has-unit-tests added

#4 @spacedmonkey
18 months ago

Thanks for the flag @dlh

The solution is extremely simply. I have created a PR with unit tests here.

I personally think that this small fix could make it way into 6.1 RC2 or 6.1.1. I don't think can or should wait until 6.2.

Thoughts @desrosj, @peterwilsoncc ?

#5 @dlh
18 months ago

It looks like there's a second scenario where placeholders can interfere with the cache key: During search queries, the prepared search terms are included with the search_orderby_title query variable, e.g.

'search_orderby_title' => [
	'wp_posts.post_title LIKE \'{8b8e7d5ae92dd4a249779814885fb4bdb2051b6bef993769f8b4352e46de15ad}hello{8b8e7d5ae92dd4a249779814885fb4bdb2051b6bef993769f8b4352e46de15ad}\'',
	'wp_posts.post_title LIKE \'{8b8e7d5ae92dd4a249779814885fb4bdb2051b6bef993769f8b4352e46de15ad}world{8b8e7d5ae92dd4a249779814885fb4bdb2051b6bef993769f8b4352e46de15ad}\'',
]

so even if placeholders are removed from $request, they'll still be in $cache_args.

It seems pretty safe to me to unset $cache_args['search_orderby_title'] along with suppress_filters, fields, etc. Or, wpdb::remove_placeholder_escape() could be run against these additional strings like it is in the PR.

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


18 months ago

#7 @chaion07
18 months ago

  • Keywords needs-testing added

@dlh thank you for reporting this. We reviewed this ticket during a recent bug-scrub session. Based on the feedback we are adding the keyword needs-testing to ensure this keyword is only removed when the fix has been verified.

Cheers!

Props to @costdev & @mukesh27

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


18 months ago

#10 @spacedmonkey
18 months ago

The first PR, isn't specially tidy. I have worked on another approach. See #3462. This adds a filter, which, I think might be useful for developers.

#11 @peterwilsoncc
18 months ago

Although it's not major, it would be good to get PR#3462 in to 6.1. This will avoid filling persistent caches with unreachable keys & potentially having reachable keys dropped as a result.

#12 @spacedmonkey
18 months ago

@peterwilsoncc @desrosj Are you happy for me to commit then?

#14 @peterwilsoncc
18 months ago

  • Keywords commit dev-feedback added

Marking for commit pending sign-off by a second committer.

#15 @davidbaumwald
18 months ago

  • Keywords dev-reviewed added; dev-feedback removed

Looks good to me!

#16 @spacedmonkey
18 months ago

  • Owner set to spacedmonkey
  • Resolution set to fixed
  • Status changed from new to closed

In 54634:

Query: Remove placeholder from query cache key.

Remove escape placeholder from query cache key, as placeholders are a based on a unique id on every request. This means that it is impossible for a cache to be reused, making queries that use escape placeholders such as LIKE searches, unable to be cached.

Props dhl, spacedmonkey, peterwilsoncc, desrosj, chaion07, davidbaumwald, mukesh27.
Fixes #56802.

#18 @jorbin
18 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening in order to encourage discussion of the filter added. See this long thread https://wordpress.slack.com/archives/C02KGN5K076/p1666106492041839

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


18 months ago

#21 @audrasjb
18 months ago

  • Priority changed from normal to high

As per today's bugscrub,

We have a patch but it still needs review and dev feedback.

Adding high priority, given we're 3 days before RC3 :)

#22 @peterwilsoncc
17 months ago

  • Keywords commit dev-reviewed removed

Updating the keywords to account for the follow up PR.

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


17 months ago

#24 @audrasjb
17 months ago

As per today's bug scrub: let's keep it in the milestone as the proposed PR is under active review from various committers. This should be backported in the next few hours.

#25 @peterwilsoncc
17 months ago

  • Keywords commit dev-feedback added
  • Owner changed from spacedmonkey to peterwilsoncc
  • Status changed from reopened to assigned

@desrosj has given me the OK via Slack to put the linked follow up PR in to trunk. I shall do so shortly.

Marking for dev-feedback for backport consideration.

#26 @peterwilsoncc
17 months ago

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

In 54685:

Query: Move cache key generation to its own method.

Introduce WP_Query::generate_cache_key() for generating the cache key used by the main database query.

This removes the need for a filter to test that cache keys do not include the WPDB placeholder causing unreachable cache keys. The tests now call WP_Query::generate_cache_key() directly.

The filter wp_query_cache_key is removed as a hard deprecation. The filter was not included in a stable release.

Follow up to [54634].

Props spacedmonkey, jorbin, azaozz, hellofromtonya, mukesh27, peterwilsoncc, desrosj, audrasjb, adamsilverstein, flixos90, davidbaumwald, joedolson, sergeybiryukov.
Fixes #56802.

#27 @peterwilsoncc
17 months ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for backport to the 6.1 branch pending a second committers approval.

@peterwilsoncc commented on PR #3507:


17 months ago
#28

Merged in https://core.trac.wordpress.org/changeset/54685 / 8b3314ad2520432f44519edf5f1be6a3cb224fe3

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


17 months ago

#30 @chaion07
17 months ago

Thanks @dlh for reporting this. We reviewed this ticket during a recent bug-scrub session. Based on the feedback received we are expecting a second reviewer or a core committer to have a look at the ticket so that we can land it in 6.1.

Cheers!

Props to @kebbet & @robinwpdeveloper

#31 @kirasong
17 months ago

  • Keywords dev-reviewed added; dev-feedback removed

Thanks so much for everyone's work here!
Love the teamwork + communication in finding a better solution.

+1 for landing this in the 6.1 branch.

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


17 months ago

#33 @davidbaumwald
17 months ago

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

In 54692:

Query: Move cache key generation to its own method.

Introduce WP_Query::generate_cache_key() for generating the cache key used by the main database query.

This removes the need for a filter to test that cache keys do not include the WPDB placeholder causing unreachable cache keys. The tests now call WP_Query::generate_cache_key() directly.

The filter wp_query_cache_key is removed as a hard deprecation. The filter was not included in a stable release.

Follow up to [54685].

Props spacedmonkey, jorbin, azaozz, hellofromtonya, mukesh27, peterwilsoncc, desrosj, audrasjb, adamsilverstein, flixos90, davidbaumwald, joedolson, sergeybiryukov.
Reviewed by mikeschroder.
Merges [54685] to the 6.1 branch.
Fixes #56802.

#34 @davidbaumwald
17 months ago

  • Keywords commit dev-reviewed removed

Resetting the keywords after the 6.1 merge.

#35 @AlanP57
17 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I'm experiencing a similar issue and have just tested a like query with RC5. The result is an SQL error and the output from the prepare function is
like '%'my search text'%'. There is an extra set of quote marks around the search text.

For more info, see https://wordpress.org/support/topic/prepare-function-removes-percent-signs-from-like-sql-statement/.

#36 @spacedmonkey
17 months ago

@AlanP57 Can you provide more detail. I can't replicate this. Is there some code I can look at? Is there a reason why you are doing a raw SQL query over using WP_Query. There are going to other issues if you do this...

#37 @AlanP57
17 months ago

In some cases, I prefer not to use WP_Query when displaying data. This is the code from the plugin:

<?php
      $sql = $wpdb->prepare("(select $wpdb->posts.ID, post_title, {$wpdb->prefix}mgmlp_folders.folder_id, pm.meta_value as attached_file, 'a' as item_type 
from $wpdb->posts
LEFT JOIN {$wpdb->prefix}mgmlp_folders ON($wpdb->posts.ID = {$wpdb->prefix}mgmlp_folders.post_id)
LEFT JOIN $wpdb->postmeta AS pm ON (pm.post_id = $wpdb->posts.ID)
LEFT JOIN $wpdb->users AS us ON ($wpdb->posts.post_author = us.ID) 
where post_type = 'mgmlp_media_folder' and pm.meta_key = '_wp_attached_file' and SUBSTRING_INDEX(pm.meta_value, '/', -1) like '%%%s%%')
union all
(select $wpdb->posts.ID, post_title, {$wpdb->prefix}mgmlp_folders.folder_id, pm.meta_value as attached_file, 'b' as item_type
from $wpdb->posts 
LEFT JOIN {$wpdb->prefix}mgmlp_folders ON( $wpdb->posts.ID = {$wpdb->prefix}mgmlp_folders.post_id) 
LEFT JOIN $wpdb->postmeta AS pm ON (pm.post_id = $wpdb->posts.ID) 
LEFT JOIN $wpdb->users AS us ON ($wpdb->posts.post_author = us.ID) 
where post_type = 'attachment' and pm.meta_key = '_wp_attached_file' and SUBSTRING_INDEX(pm.meta_value, '/', -1) like '%%%s%%') order by attached_file", $search_string, $search_string);

And here is an example of the SQL:
(select wp_posts.ID, post_title, wp_mgmlp_folders.folder_id, pm.meta_value as attached_file, 'a' as item_type
from wp_posts
LEFT JOIN wp_mgmlp_folders ON(wp_posts.ID = wp_mgmlp_folders.post_id)
LEFT JOIN wp_postmeta AS pm ON (pm.post_id = wp_posts.ID)
LEFT JOIN wp_users AS us ON (wp_posts.post_author = us.ID)
where post_type = 'mgmlp_media_folder' and pm.meta_key = '_wp_attached_file' and SUBSTRING_INDEX(pm.meta_value, '/', -1) like '{48bf6209debff2ee81208ffaee83c0ccfe32af6953d915a72a2fd46f1d0be2e1}'my search text'{48bf6209debff2ee81208ffaee83c0ccfe32af6953d915a72a2fd46f1d0be2e1}')
union all
(select wp_posts.ID, post_title, wp_mgmlp_folders.folder_id, pm.meta_value as attached_file, 'b' as item_type
from wp_posts
LEFT JOIN wp_mgmlp_folders ON( wp_posts.ID = wp_mgmlp_folders.post_id)
LEFT JOIN wp_postmeta AS pm ON (pm.post_id = wp_posts.ID)
LEFT JOIN wp_users AS us ON (wp_posts.post_author = us.ID)
where post_type = 'attachment' and pm.meta_key = '_wp_attached_file' and SUBSTRING_INDEX(pm.meta_value, '/', -1) like '{48bf6209debff2ee81208ffaee83c0ccfe32af6953d915a72a2fd46f1d0be2e1}'my search text'{48bf6209debff2ee81208ffaee83c0ccfe32af6953d915a72a2fd46f1d0be2e1}') order by attached_file

This use to work in versions before 6.1.

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

#38 @desrosj
17 months ago

  • Keywords fixed-major removed
  • Resolution set to fixed
  • Status changed from reopened to closed

I hope you don't mind, @AlanP57, but I've gone and created #56933 on your behalf. I detailed some reasons why in a comment on that ticket.

I'm going to close this out as fixed again. If you were a participant on this ticket, please see how you can help push #56933 along and rule out the changesets related to this bug (especially with 6.1 due out in a ~24 hours).

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


17 months ago

Note: See TracTickets for help on using tickets.