WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 4 years ago

#11431 closed enhancement

object cache's terms bucket potentially contains junk — at Version 6

Reported by: Denis-de-Bernardy Owned by: ryan
Milestone: 3.1 Priority: normal
Severity: normal Version:
Component: Cache API Keywords: early has-patch commit
Focuses: Cc:

Description (last modified by Denis-de-Bernardy)

We've a few occurrences of things such as:

wp_cache_add( $cache_key, $terms, 'terms' );

... in taxonomy.php. These are not flushed when the terms cache gets flushed, and end up stored in memcached indefinitely. We should at least add a timeout, maybe after a day.

Better yet, we should flush them properly.

Change History (7)

comment:1 Denis-de-Bernardy4 years ago

  • Keywords has-patch added

see also: r9102, r8225, etc.

Denis-de-Bernardy4 years ago

comment:2 Denis-de-Bernardy4 years ago

there potentially is a better way to do this. such as is done in get_pages():

	$key = md5( serialize( compact(array_keys($defaults)) ) );
	if ( $cache = wp_cache_get( 'get_pages', 'posts' ) ) {
		if ( is_array($cache) && isset( $cache[ $key ] ) ) {
			$pages = apply_filters('get_pages', $cache[ $key ], $r );
			return $pages;
		}
	}

comment:3 filosofo4 years ago

I don't think the get_pages() approach scales very well. I've noticed several times that sites with a large number of pages at points exhaust the available memory in one of the cache methods. Since for a typical WP site there are many more terms than pages, I'd expect this to crop up more often if used for terms.

Why not do something like I suggested a while ago: #4476. Delete cache items by group name. Then the clean_term_cache() could just flush all members of the "terms" group. Problem solved, right?

comment:4 ryan4 years ago

The last_changed method used by terms was done to avoid the problems of the pages style caching where the arrays could get huge. Currently, the last_changed method relies on old cache keys eventually being pushed out of the cache. Using a timeout to reclaim them faster seems fine.

I've yet to see flush by group (or even just flush) implemented solidly for all of the various cache backends for both WP and WPMU.

comment:5 Denis-de-Bernardy4 years ago

Replying to filosofo:

Why not do something like I suggested a while ago: #4476. Delete cache items by group name. Then the clean_term_cache() could just flush all members of the "terms" group. Problem solved, right?

It would, if memcached allowed to partially flush keys using wildcards. Trouble is, memcached offers only two option, mark this very specific key as obsolete, and mark all keys as obsolete.

there are a few bits in backpress' memcached that allow to to flush an entire bucket, but I'm 100% sure that it is not concurrent proof: (I interacted the pecl package's author last year in order to convince him to implement locks for memcached-based session handling. It's the only thing in the memcached pecl with any type of locking.)

@Ryan: the file I sent you the other day implements flush (as in, full flush) quite fine. But no partial flush.

comment:6 Denis-de-Bernardy4 years ago

  • Description modified (diff)
  • Keywords dev-feedback added

I've been working with the patch applied for a couple days. No side effects apart from the obvious extra query per day.

@Ryan: do you need it massaged any further?

Note: See TracTickets for help on using tickets.