Make WordPress Core

Opened 16 years ago

Closed 11 years ago

#7237 closed enhancement (wontfix)

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

Reported by: mdawaffe's profile mdawaffe Owned by: ryan's profile 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 16 years ago.

Download all attachments as: .zip

Change History (16)

@mdawaffe
16 years ago

#1 @ryan
15 years ago

  • Keywords has-patch added

#2 @ryan
15 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.

#3 @ryan
15 years ago

  • Owner changed from anonymous to ryan

#4 @jacobsantos
15 years ago

  • Milestone changed from 2.8 to 2.9

#5 @jacobsantos
15 years ago

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

#6 @Denis-de-Bernardy
15 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

#7 @Denis-de-Bernardy
15 years ago

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

#8 @Denis-de-Bernardy
15 years ago

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

#9 @hakre
15 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.

#10 @hakre
13 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 versions since 2.6.

Last edited 13 years ago by hakre (previous) (diff)

#11 @westi
11 years 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

#12 @westi
11 years 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.

#13 @ethitter
11 years ago

  • Cc erick@… added

#14 @adamsilverstein
11 years ago

  • Cc adamsilverstein@… added

#15 @nacin
11 years 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.