better handling of wildcards in cache_clear_all
Reg - December 10, 2007 - 12:18
| Project: | Memcache API and Integration |
| Version: | 5.x-1.6 |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
Description
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.
#1
This issue ties in with this AdvCache issue: http://drupal.org/node/199465
#2
+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.
#3
#4
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);
#5
@firebus:
Let's not do this.
Reading Reg's (non-compacted) code and it looks good. Needs testing and a patch.
#6
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);
}
#8
in that case I'd prefer:
<?phpif (variable_get('memcache_debug', FALSE)) {
watchdog...
}
?>
This allows people to set the variable in settings.php instead of futzing with their source code.
#9
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.
#10
@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.
#11
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.
#12
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?
#13
Fixed in memcache.inc v1.25 and memcache.db.inc v1.4 in HEAD.
#14
Automatically closed -- issue fixed for two weeks with no activity.
#15
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.