Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#37184 closed defect (bug) (fixed)

Stop FOUND_ROWS running in comment query is cached

Reported by: spacedmonkey's profile spacedmonkey Owned by: boonebgorges's profile boonebgorges
Milestone: 4.6 Priority: high
Severity: normal Version: 4.4
Component: Comments Keywords: has-patch needs-unit-tests
Focuses: Cc:


In #36906 the comment group was made to be a persistent object cache group. This means that comment queries using wp_comment_query are now cached. However, if query is cached and the no_found_rows param is set to true, the SELECT FOUND_ROWS() query still runs. This query will return the incorrect value as the comment query with SQL_CALC_FOUND_ROWS will not have run. Likely the found rows will fall back to the main query on the page, giving the incorrect value of found comments. Found comments will be used for pagination of multi page comments, and it will only show the first page of comments.

Attachments (1)

37184.diff (3.3 KB) - added by spacedmonkey 8 years ago.

Download all attachments as: .zip

Change History (10)

8 years ago

#1 @spacedmonkey
8 years ago

  • Component changed from General to Comments
  • Keywords has-patch added

This style of caching / bug fix was applied to wp_site_query here [37868].

#2 @spacedmonkey
8 years ago

Please ignore 37184.2.diff . Uploaded to wrong ticket.

#3 @ocean90
8 years ago

  • Version changed from trunk to 4.4

Introduced in [34544].

#4 @boonebgorges
8 years ago

  • Keywords needs-unit-tests added
  • Milestone changed from Awaiting Review to 4.6
  • Owner set to boonebgorges
  • Status changed from new to reviewing

Introduced in [34544] but more obvious with persistent comment caching. This should be fixed for 4.6.

I don't think there's any need to cache 'max_num_pages' since it can be calculated from found_comments.

#5 @spacedmonkey
8 years ago

We save max pages as it is only generated when no_found_rows is set to false. Please see this comment [37868] where we have done the same in WP_Site_Query.

#6 @ocean90
8 years ago

  • Priority changed from normal to high

#7 @boonebgorges
8 years ago

  • Status changed from reviewing to accepted

We save max pages as it is only generated when no_found_rows is set to false. Please see this comment [37868] where we have done the same in WP_Site_Query.

The cache should hold as little information as possible, to avoid cache bloat and potential bugs related to improper invalidation. Values that can be computed arithmetically from known values should not be cached for these reasons. IMO [37868] sets a bad precedent in this regard.

#8 @boonebgorges
8 years ago

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

In 38001:

Comments: Cache results of SELECT FOUND_ROWS() query.

When comment IDs are fetched from the cache rather than the database,
the subsequent SELECT FOUND_ROWS() query will not return the correct value.
To avoid unnecessary queries, we cache the results of the found_comments
query alongside the comment IDs.

Props spacedmonkey.
Fixes #37184.

This ticket was mentioned in Slack in #core-multisite by ocean90. View the logs.

8 years ago

Note: See TracTickets for help on using tickets.