Make WordPress Core

#57012 closed defect (bug) (fixed)

WP_Query caching discards `posts_fields` and `posts_clauses['fields']` filters.

Reported by: peterwilsoncc's profile peterwilsoncc Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: 6.1.1 Priority: normal
Severity: normal Version: 6.1
Component: Query Keywords: has-patch has-unit-tests commit fixed-major
Focuses: performance Cc:

Description

When generating the cache key, WP_Query uses a consistent value for the fields requested in the SELECT clause.

In most cases, this is problem free as WP_Query uses the same cache regardless of the fields requested. When a plugin is filtering the fields returned, this can be problematic as the cache key could cause collisions in some circumstances.

When generating the cache key, WP_Query should consider the fields value if it has been modified by a plugin. This may also affect what needs to be cached.

I'm still investigating this so will add further notes to the ticket. I'm putting this on the 6.1.1 milestone for visibility.

Attachments (1)

57012.diff (6.2 KB) - added by SergeyBiryukov 19 months ago.

Download all attachments as: .zip

Change History (19)

#1 @peterwilsoncc
19 months ago

I've created a mini-plugin to modify the fields requested by WP_Query:

<?php
add_action( 'init', function() {
        add_filter( 'posts_fields', function( $fields ) {
                $fields .= ', 1 as test_post_fields';
                return $fields;
        } );

        add_filter( 'posts_clauses', function( $clauses ) {
                $clauses['fields'] .= ', 2 as test_post_clauses';
                return $clauses;
        } );

        $query2 = new WP_Query(
                array(
                        'post_type' => 'post',
                        'posts_per_page' => 1,
                        'fields' => 'id=>parent',
                        'cache_results' => false,
                )
        );

        var_dump( $query2->posts );

        exit;
}, 999 );

When initializing WP_Query( 'fields' => $value ) the following happens in 6.0:

  • $value = 'ids': The posts property is an array if integers. The 6.1 behaviour matches.
  • $value = 'all' /* or '' */: The posts property contains an array of WP_Post objects with the addition of test_post_fields and test_post_clauses. The 6.1 behaviour does not match.
  • $vallue = 'id=>parent': The posts property contains an array of stdClass with the properties ID, post_parent, test_post_fields and test_post_clauses. The 6.1 behaviour does not match.

I see two options:

  • bypass caching if the fields are not one of the three expected values WP_Query handles by default
  • include the SQL fields in the cache key if the fields are not one of the three expected values

Either way, tests will need to be added to ensure changing the fields includes the additional values in the event the posts property is an object.

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


19 months ago
#2

  • Keywords has-patch has-unit-tests added

https://core.trac.wordpress.org/ticket/57012

First commit is some tests that pass on 6.0 but not on 6.1

#3 @peterwilsoncc
19 months ago

Props due @pypwalters, @claytoncollie, @johnwatkins0 for helping diagnose this issue.

#4 @peterwilsoncc
19 months ago

In the linked pull request:

  • Added some tests to ensure modified SELECTs return the same values on the first and subsequent requests
  • Bypass the main cache (ie, revert to 6.0 behavior) if the SELECT has been modified from an expected form

I considered caching modified SELECTs but figured some additional care would need to be taken to account for SELECTing many values that could blow up the size of the cached value.

@TimothyBlynJacobs commented on PR #3574:


19 months ago
#5

This seemse like a sensible fix for a minor to me. 6.2 could explore generating a more comprehensive cache key.

@peterwilsoncc commented on PR #3574:


19 months ago
#6

Props due @costdev for reviewing names and tests via Slack DM.

#7 @spacedmonkey
19 months ago

  • Keywords commit added

Patch looks good to me. Adding commit, ahead of merge deadline in 6.1.1.

#8 @spacedmonkey
19 months ago

  • Focuses performance added

#9 @peterwilsoncc
19 months ago

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

In 54768:

Query: Bypass caching for filtered SELECTs.

Bypass caching within WP_Query when the SELECT clause has been modified via a filter. This prevents both cache key collisions and the returning of incomplete or unexpected results when the SELECT clause has been modified by an extender.

Props pypwalters, claytoncollie, johnwatkins0, TimothyBlynJacobs, costdev, spacedmonkey, peterwilsoncc.
Fixes #57012.

#10 @peterwilsoncc
19 months ago

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

Reopening merge for to the 6.1 branch.

#12 @SergeyBiryukov
19 months ago

In 54777:

Coding Standards: Use consistent spelling for "cacheable" in WP_Query::get_posts().

Follow-up to [53941], [54768].

See #57012.

#13 @desrosj
19 months ago

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

In 54780:

Query: Bypass caching for filtered SELECTs.

Bypass caching within WP_Query when the SELECT clause has been modified via a filter. This prevents both cache key collisions and the returning of incomplete or unexpected results when the SELECT clause has been modified by an extender.

Props pypwalters, claytoncollie, johnwatkins0, TimothyBlynJacobs, costdev, spacedmonkey, peterwilsoncc.
Merges [54768] to the 6.1 branch.
Fixes #57012.

#14 @desrosj
19 months ago

In 54781:

Coding Standards: Use consistent spelling for "cacheable" in WP_Query::get_posts().

Follow-up to [53941], [54768].

Props SergeyBiryukov.
Merges [57012] to the 6.1 branch.
See #57012.

#15 @SergeyBiryukov
19 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

After [54768], seeing some test failures on MariaDB due to indeterminate sort order, see these test runs for example:

I can also reproduce these failures locally with MariaDB 10.6.8.

57012.diff aims to stabilize the tests by using assertEqualSets() instead of assertEquals(), since testing the order is out of their scope. Includes removing array_unshift() and array_reverse() calls as no longer needed.

#16 @peterwilsoncc
19 months ago

Thanks @SergeyBiryukov, the patch looks good based on a visual review.

As an educated guess, this is caused by hitting undefined behavior due to create_many creating several posts with the same date/time fields.

WP_Query is generating the DB Query below and each engine refines the order further using a different index.

SELECT SQL_CALC_FOUND_ROWS  wptests_posts.ID, wptests_posts.post_parent
FROM wptests_posts 
WHERE 1=1
        AND ((wptests_posts.post_type = 'wptests_pt'
        AND (wptests_posts.post_status = 'publish')))
ORDER BY wptests_posts.post_date DESC
LIMIT 0, 10

See also #42936, #56991.

#17 @SergeyBiryukov
18 months ago

In 54829:

Tests: Resolve WP_Query test failures on MariaDB due to indeterminate sort order.

[54768] added a few tests to verify that caching within WP_Query is bypassed when the SELECT clause has been modified via a filter, to avoid cache key collisions and the returning of incomplete or unexpected results.

However, creating several posts with the same date/time fields can result in inconsistent sort ordering between MySQL and MariaDB, as each engine refines the order further using a different index.

This commit aims to stabilize the tests by using assertEqualSets() instead of assertEquals(), since testing the order is out of their scope. Includes removing array_unshift() and array_reverse() calls as no longer needed.

This resolves a few test failures on MariaDB along the lines of:

Tests_Query_FieldsClause::test_should_limit_fields_to_id_and_parent_subset
Posts property for first query is not of expected form.
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
     0 => stdClass Object (
-        'ID' => 36019
+        'ID' => 36015
         'post_parent' => 0
     )
     1 => stdClass Object (
-        'ID' => 36018
+        'ID' => 36016
         'post_parent' => 0
     )
     2 => stdClass Object (...)
     3 => stdClass Object (
-        'ID' => 36016
+        'ID' => 36018
         'post_parent' => 0
     )
     4 => stdClass Object (
-        'ID' => 36015
+        'ID' => 36019
         'post_parent' => 0
     )
 )

/tmp/wp-test-runner/tests/phpunit/tests/query/fieldsClause.php:67
/tmp/wp-test-runner/phpunit-5.7.phar:598

Follow-up to [54768].

Props peterwilsoncc, SergeyBiryukov.
See #57012.

#18 @SergeyBiryukov
18 months ago

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

In 54830:

Tests: Resolve WP_Query test failures on MariaDB due to indeterminate sort order.

[54768] added a few tests to verify that caching within WP_Query is bypassed when the SELECT clause has been modified via a filter, to avoid cache key collisions and the returning of incomplete or unexpected results.

However, creating several posts with the same date/time fields can result in inconsistent sort ordering between MySQL and MariaDB, as each engine refines the order further using a different index.

This commit aims to stabilize the tests by using assertEqualSets() instead of assertEquals(), since testing the order is out of their scope. Includes removing array_unshift() and array_reverse() calls as no longer needed.

This resolves a few test failures on MariaDB along the lines of:

Tests_Query_FieldsClause::test_should_limit_fields_to_id_and_parent_subset
Posts property for first query is not of expected form.
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
     0 => stdClass Object (
-        'ID' => 36019
+        'ID' => 36015
         'post_parent' => 0
     )
     1 => stdClass Object (
-        'ID' => 36018
+        'ID' => 36016
         'post_parent' => 0
     )
     2 => stdClass Object (...)
     3 => stdClass Object (
-        'ID' => 36016
+        'ID' => 36018
         'post_parent' => 0
     )
     4 => stdClass Object (
-        'ID' => 36015
+        'ID' => 36019
         'post_parent' => 0
     )
 )

/tmp/wp-test-runner/tests/phpunit/tests/query/fieldsClause.php:67
/tmp/wp-test-runner/phpunit-5.7.phar:598

Follow-up to [54768].

Props peterwilsoncc, SergeyBiryukov.
Merges [54829] to the 6.1 branch.
Fixes #57012.

Note: See TracTickets for help on using tickets.