WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 17 months ago

#7237 closed enhancement (wontfix)

Multiple clean_post_cache() calls when saving a post due to post revisions

Reported by: mdawaffe Owned by: ryan
Milestone: Priority: lowest
Severity: minor Version: 2.6
Component: Revisions Keywords: reporter-feedback
Focuses: Cc:

Description

When a post is saved, clean_post_cache() is called for that post. Immediately afterward, the post revision is inserted and clean_post_cache() is called again for that new post row.

Since the current implementation of post revisions does not allow revision edit, has no taxonomy associated with revisions, has no post meta, and shows up in no archive, we can get away with never calling clean_post_cache() during a post revision insert.

If a plugin does any of those things, it can still call clean_post_cache() from any of a few available API actions.

Attached.

I don't know if this is worth fixing. I don't know how much of a performance impact this "bug" has.

Attachments (1)

7237.diff (1.4 KB) - added by mdawaffe 6 years ago.

Download all attachments as: .zip

Change History (16)

mdawaffe6 years ago

comment:1 ryan6 years ago

  • Keywords has-patch added

comment:2 ryan6 years ago

  • Milestone changed from 2.7 to 2.8

I'll work this into the wp_suspend_cache_invalidation() stuff for 2.8. I think it can ride until then.

comment:3 ryan6 years ago

  • Owner changed from anonymous to ryan

comment:4 jacobsantos5 years ago

  • Milestone changed from 2.8 to 2.9

comment:5 jacobsantos5 years ago

Moving since Ryan has noted that he has done work for 2.8 that should help with this ticket.

comment:6 Denis-de-Bernardy5 years ago

a slightly different approach might work too?

if is_post_revision() then do nothing, elseif is_page() then clear page cache, else clear post cache

comment:7 Denis-de-Bernardy5 years ago

  • Component changed from General to Optimization
  • Milestone changed from 2.9 to Future Release

comment:8 Denis-de-Bernardy5 years ago

  • Milestone changed from Future Release to 2.9
  • Type changed from defect (bug) to enhancement

comment:9 hakre5 years ago

  • Keywords has-patch removed
  • Milestone changed from 2.9 to Future Release

looks more like a bug to me then an enhancement. the hint to wp_suspend_cache_invalidation() is misleading as well (does not fix anything, just only a bad workaround if used for this. patch does not apply any longer anyway.

comment:10 hakre3 years ago

  • Keywords reporter-feedback added

@mdawaffe - any chance to see some traction on this one? Does it still applies? I remember some changes to the cache of posts in the version since 2.6.

Version 0, edited 3 years ago by hakre (next)

comment:11 westi18 months ago

  • Keywords revisions-3.6 added

This may or may not have been improved by the changes in 3.6 to the caching.

We should review this as part of the Revisions work in 3.6

comment:12 westi18 months ago

  • Component changed from Optimization to Revisions
  • Keywords revisions-3.6 removed
  • Milestone changed from Future Release to 3.6

Including this in our Revisions Sweep for 3.6, Step 1 investigate the current improved code.

comment:13 ethitter18 months ago

  • Cc erick@… added

comment:14 adamsilverstein18 months ago

  • Cc adamsilverstein@… added

comment:15 nacin17 months ago

  • Milestone 3.6 deleted
  • Resolution set to wontfix
  • Status changed from new to closed

Here's what a typical execution looks like, for saving an existing post:

  • wp_insert_post() triggers get_post(), fetching from the DB the current row of the post
  • The revision gets created, triggering clean_post_cache(), which (since only a post ID is passed) fetches the row from the DB
  • As the cache is cleared, the ensuing get_post() then fetches the revision's row from the DB again
  • The main post's cache is cleared, then the post is again pulled from the DB

The only thing that could be avoided is avoiding some clean_post_cache() work when we are inserting a new post. Not even all of it, because we'll probably still want to make sure that the hook fires and certain things get cleared, like wp_get_archives, get_pages, etc (these are all being moved to incrementors, but still).

But is it really a bad thing to force things like wp_cache_delete( $post->ID, 'posts' );? Just to make sure there is absolutely no cache pollution.

So I think this is wontfix. In 3.5, removed the double clean_post_cache() calls (it used to happen twice in wp_insert_post(), one of them on the save_post hook) and prevented children caching clearing, which alleviated the vast majority of this.

Also, revisions are getting more powerful. They're eventually going to have metadata, and a lot of plugins (like Edit Flow and Post Forking) have an interest in strengthening revisions. Given there is barely any performance impact, I'm closing this.

Note: See TracTickets for help on using tickets.