I'm looking at the code in cache_clear_all in memcache.inc here:

  // Memcache logic is simpler because memcache doesn't have a minimum cache
  // lifetime consideration (it handles it internally), and doesn't support
  // wildcards.
  if (empty($cid) || $cid == '*') {
    dmemcache_flush($table);
  }
  else {
    dmemcache_delete($cid, $table);
  }

and the question comes to mind, what happens when someone sends a wildcard to this API. For example, the passed parameters could be: $cid = '1993::', $table = 'cache_node', $wildcard = TRUE. The documentation says that memcache doesn't support wildcards, this would mean that such a set of parameters would try to clear a non-existent object and nothing would be cleared. Would it not be safer to have the following:

  if (empty($cid) || $cid == '*' || $wildcard === TRUE) {
    dmemcache_flush($table);
  }
  else {
    dmemcache_delete($cid, $table);
  }

While you might clear more of the cache than you want, you won't leave cache that should be cleared, uncleared.

CommentFileSizeAuthor
#12 199483.patch2.51 KBfirebus

Comments

reg’s picture

reg commented

This issue ties in with this AdvCache issue: http://drupal.org/node/199465

firebus’s picture

firebus commented
Title: How is this working correctly? » better handling of wildcards in cache_clear_all better

+1. i agree that this is better default behavior of memcache for the reasons Reg provided.

i'd also suggest calling watchdog if memcache gets a cache_clear_all where wildcard == TRUE and cid != '*', to make it easier for devs to find wildcards they missed.

if ($wildcard && $cid != '*') {
  watchdog('memcache', "illegal wildcard in cache_clear_all - flushing entire bin. table: $table. cid: $cid", WATCHDOG_WARNING);
}

finally, consider http://drupal.org/node/186368 as well - if cid == '*' and wildcard != TRUE, then memcache should do nothing.

firebus’s picture

firebus commented
Title: better handling of wildcards in cache_clear_all better » better handling of wildcards in cache_clear_all
reg’s picture

reg commented

Those two together would leave us with something like:

  if (empty($cid) || ($cid == '*' && $wildcard !== TRUE)) {
    # don't do anything if cid is unset. this matches the default drupal behavior...
  }
  else if ($cid == '*' || $wildcard === TRUE) {
    dmemcache_flush($table);
  }
  else {
    dmemcache_delete($cid, $table);
  }

or compacted

  if (empty($cid) || ($cid == '*' && $wildcard !== TRUE))
    # don't do anything if cid is unset. this matches the default drupal behavior...
  else if ($cid == '*' || $wildcard === TRUE)
    dmemcache_flush($table);
  else
    dmemcache_delete($cid, $table);
robertdouglass’s picture

robertdouglass commented

@firebus:

i'd also suggest calling watchdog if memcache gets a cache_clear_all where wildcard == TRUE and cid != '*', to make it easier for devs to find wildcards they missed.

Let's not do this.

Reading Reg's (non-compacted) code and it looks good. Needs testing and a patch.

reg’s picture

reg commented

Just a thought:

if (empty($cid) || ($cid == '*' && $wildcard !== TRUE)) {
  # don't do anything if cid is unset. this matches the default drupal behavior...
  /* ADMIN's, uncomment this code to help debug suspected caching performance issues
  if ($wildcard && $cid != '*') {
    watchdog('memcache', "illegal wildcard in cache_clear_all - flushing entire bin. table: $table. cid: $cid", WATCHDOG_WARNING);
  } */
}
else if ($cid == '*' || $wildcard === TRUE) {
  dmemcache_flush($table);
}
else {
  dmemcache_delete($cid, $table);
}
robertdouglass’s picture

robertdouglass commented

in that case I'd prefer:

if (variable_get('memcache_debug', FALSE)) {
  watchdog...
}

This allows people to set the variable in settings.php instead of futzing with their source code.

reg’s picture

reg commented

Personally, I think if someone is working on whether the cache could need tweaking then they should be able to uncomment a line or two of code. Commenting it keeps one more piece of code from needing to execute in an area where the whole purpose is speed improvement. However, at the end of the day, I don't think matters one way or the other, go for it.

robertdouglass’s picture

robertdouglass commented

@Reg: in an ideal Drupal, the only file that you edit is settings.php. If you have to change code in your installation, it should be via a patch.

reg’s picture

reg commented

You must be a formal developer. I just use it and supply code when I can. Like I said, go for it and write the code.

firebus’s picture

firebus commented
Status: Active » Needs review
StatusFileSize
new 199483.patch2.51 KB

i like the idea of making the watchdog call contingent on a variable.

i've closed my issue http://drupal.org/node/186368 as a dupe in favor of this one and have uploaded a patch.

i changed the if/else structure to match the structure in cache.inc, so it's easier to compare what core does vs. what we do.

since we use the exact same block in memcache.inc and memcache.db.inc, it might be worth factoring out this code into a function in memcache.module.

@Reg: could you test this and let me know if it works for you?

@robertDouglass: http://drupal.org/node/186332 affects the same function - would you mind taking a look and letting me know if i'm wrong about it when you have a chance?

slantview’s picture

slantview commented
Status: Needs review » Fixed

Fixed in memcache.inc v1.25 and memcache.db.inc v1.4 in HEAD.

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.

reg’s picture

reg commented

Sorry I was so slow in getting back. This certainly fixes the logical error which is a fix for everyone. However I brought this up in conjunction with issue http://drupal.org/node/199465 in Advanced Cache and it really doesn't solve that issue but I will elaborate there as that would be more appropriate.