WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#14080 closed defect (bug) (fixed)

Needless wp_cache_delete() inside set_theme_mod()

Reported by: shidouhikari Owned by:
Milestone: 3.1 Priority: normal
Severity: normal Version:
Component: Cache API Keywords:
Focuses: Cc:

Description

In /wp-includes/theme.php, the function set_theme_mod() calls update_option("mods_$theme" and then wp_cache_delete("mods_$theme", 'options').

Is this cache delete really needed? Inside update_option() I see that it uses wp_cache_set( 'alloptions' or wp_cache_set( $option.

I believe update_option(), add_option(), delete_option() etc should handle all option caches, from group 'options', and no other code anywhere else should need to bother with it, to avoid inconsistences.

If set_theme_mod() doesn't need to edit cache and update_option() handles it, maybe wp_cache_delete() should be removed. And if set_theme_mod() indeed needs to handle it, maybe update_option() and other related functions should be fixed to handle it and avoid cache inconsistences.

Also, since "mods_$theme" value is already known and accessible, a wp_cache_set() would be better than just deleting it.

Change History (9)

#1 @mikeschinkel
7 years ago

  • Cc mikeschinkel@… added

+1

#2 @nacin
6 years ago

  • Milestone changed from Awaiting Review to 3.1

Wondering if the wp_cache_delete() is just cruft.

#3 follow-up: @gazouteast
6 years ago

  • Cc gazouteast added

Be careful here - cross refer the wp_options table persistent bloating issue, now showing as persisting under Simplepie when originally blamed on MagPie - there may be clues here regarding crossover contributories.

#4 in reply to: ↑ 3 ; follow-up: @nacin
6 years ago

Replying to gazouteast:

Be careful here - cross refer the wp_options table persistent bloating issue, now showing as persisting under Simplepie when originally blamed on MagPie - there may be clues here regarding crossover contributories.

Please, stop. This has nothing to do with your tickets.

#5 in reply to: ↑ 4 @gazouteast
6 years ago

Replying to nacin:

Replying to gazouteast:

Be careful here - cross refer the wp_options table persistent bloating issue, now showing as persisting under Simplepie when originally blamed on MagPie - there may be clues here regarding crossover contributories.

Please, stop. This has nothing to do with your tickets.

Nacin - think about it - your knee-jerk reaction was to label the cache delete as "cruft" ... and yet, lack of expired cache deletion is the exact root symptom of the database bloat in wp_options. It's like you believe nothing should be deleted, ever, and that everyone has got trillions of terabytes of database capacity.

Right now, anything that relates to deleting cache getting removed from cache is a bad move - I'm not alone in experiencing the wp_options bloat.

Apologies to shidouhikari - I'm not trying to hijack - related to this is that I also have some wp_options table bloat from non-deleted, expired, themes version check entries - it's minor in the overall bloat, but it's there - it's a contributory. I'd make a fairly safe guess that others have it too.

To me it all has a relationship, and the wider effect of knocking out "cruft" has to be explored before banging ahead with such a solution.

#6 @nacin
6 years ago

The cache deletion doesn't remove it from the database. It's not adding more things to the database. All it is doing is forcing the next call of get_theme_mod() to skip the object cache and go straight for the new DB option. Though I think the OP is right that update_option() updates the object cache in this situation, therefore rendering the delete_cache() call unnecessary.

#7 @nacin
6 years ago

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

(In [16029]) Remove unnecessary wp_cache_delete calls. props shidouhikari, fixes #14080.

#8 @gazouteast
6 years ago

Huh?

Do What?

So the cache is saved to the database for recall, but cache delete does not remove it from the database?

That's a bit like the garbage collectors coming round, looking in your garbage bin, then walking away without emptying it.

Maybe I was right - maybe that is exactly the cause of the wp_options bloat and it was nothing at all to do with MagPie?

In terms of having parallel update and delete calls, yes I can see where an update should override a delete, but then there are also times when a delete should override an update.

Seems to me, what is needed is a conditional periodic purge function (not just at version upgrade) -

  • Check table for expired cache objects
  • identify creating function or plugin from cached object
  • check for function or plugin existence
  • if function or plugin does not exist, delete cache transient

Some pretty heavyweight plugins fail to clean up behind themselves - Popularity Contest being one of them. It now looks like even core dashboard widgets do that too. This problem is bigger than it first appears.

Suggest - reopen ticket and leave for community feedback.

#9 @shidouhikari
6 years ago

Thanks nacin.

gazouteast, I understand your agony, I also have some wp_options rows that are bloating database and I don't have the time to hunt them.

But it's not the case here, hold your anger. Where did you see that cache is stored in database? Normally, cache is temporary stored in RAM during a page load and lost when the page finished loading. There are some plugins that persistently stores cache in HD and even in RAM servers, but even so cache is used to avoid database queries.

When update_option() is called it updates the cache, and next time get_option() is called and data isn't in cache it's added there, so there's no reason to have a wp_delete_cache() after a update_option().

The best solution for our database bloating issue is everybody be responsible for each option added, handle its deletion when adequate, and provide users the possibility of deleting them before uninstalling a plugin/theme/widget while not deleting them during a simple deactivation.

My plugins for exemple have an uninstall form that lists EVERY database presence and easily deletes EVERYTHING when admin requires.

If you want a core feature to delete old options, please develop it. The problem is that options are meant to presist and today there's no feature to bind an option to the code that added it, so we can't easily know if an option that wasn't edited for long time (we can't even know when it was last edited) can be deleted or if it's still being used now and then and just don't need to be edited.

Maybe you are trying to talk about transient data. Those one I agree that may bloat database if created and never queried again. But in this case, just install a persistent cache plugin and they will stop being stored in database (and you'll still need to manually delete old ones that were created before the plugin was installed).

Note: See TracTickets for help on using tickets.