node cache_clear_all routine does not scale for sites with many roles

i'll take another look at the code, and will check with robert to see if there was any reason (other than memcache support) not to do a wildcard.

if not, it probably makes sense to default to the wildcard clear, and use the original version for memcache support.

i've also thought about keeping track of which role combinations exist (in your site with 19 roles, you almost certainly have a much smaller number of actual role combinations), or adding a configuration setting for which role combos you want to cache (would help both node and comment cache) to improve this feature.

i noticed that!

on my site, which has fewer roles but over 100K users, node caching would not scale for me if the node cache was completely flushed on every node update :)

i think you're correct that http://drupal.org/node/199483 would improve the default behavior of memcache, however i don't think flushing the whole cache is a good solution for advcache.

That's interesting. The overhead of this is at least constant (can't balloon like 2^19 roles), and it's only on clearing cache, and not a terribly complicated query. I think I like the approach. Have time to write a patch?

SELECT DISTINCT(GROUP_CONCAT(rid ORDER BY rid ASC SEPARATOR '-')) FROM {users_roles} GROUP BY uid

Should do the trick.

Potential Issues

• This might be an expensive query for a user_roles database that represents many users and many roles. You'd want to do this once, or infrequently on a cron, and then store the result somewhere
• I have no idea if there is a PostgreSQL version of GROUP_CONCAT.
• This might produce very large strings if users have many roles, so it might be worth parsing the results and using the pow function or some other function to produce smaller unique identifiers for role combos to use with the cache keys

My gut feeling is still that the issue of role combinations should be exposed as a configuration page. The set of cachable role combos would be stored with variable_set. The admin could manually choose valid role combos, or could run the query above to automatically set things up. It would be up to the admin to update the config if they added roles or added valid combos.

The config solution is also nice if you have some role combos that you don't want to enable caching for in your use case.

GROUP_CONCAT isn't database neutral. You'll have to use GROUP BY

yeah, i realized that :)

i don't know of a method to get a unique value per role-combo using GROUP BY alone. GROUP_CONCAT allows us to generate a unique string per role-combo.

AFAIK, the other aggregate functions (SUM, etc.) all have possible collisions for distinct role combos (eg, one use has roles 3 and 5, another has 2 and 6, they look the same to SUM)

but i could be missing something, or perhaps there's another SQL-only method?

aha! of course! just like we're doing now with advcache_array2int:

SELECT DISTINCT SUM(POW(rid, 2)) FROM {users_roles} GROUP BY uid;

Close!
SELECT DISTINCT(SUM(POW(2, rid))) as sum, uid FROM users_roles GROUP BY uid;

it's 2^^rid

also, you'll want to see if you need the -2 in there as well to accurately align with the real cache ids.

@Reg: "The GROUP_CONCAT works great and most people will be using MySQL " <--- doesn't cut it. It has to be ANSI SQL compatible or it doesn't get in.

Reg, could you elaborate with example data how your suggestion works? It would keep the discussion going at a moment when I don't have time to actually implement and test your suggestions. Thanks.

The tables the come the advcache installation are empty.

One more issue is in the advcache_nodeapi().
$maxrole = pow(2, (int)dbresult(dbquery(“SELECT MAX(rid) FROM {role}”)) - 1);
for ($i = 1; $i < $maxrole; $i++) {
cache_clear_all($node->nid. ‘::’. $i, ‘cachenode’);
}

My system has $maxrole = pow(2, 27). It means cacheclearall will execute pow(2, 28) times. And it will result ti a PHP time-out.

And, by the way, I’m also running memcache. Is there any conflict with it?

I hope I’m making sense here.

No conflict with memcache.

$int = advcache_array2int($user->roles);

This line of code is found on advcache_nodeapi().

But then, I always wonder why $int is never used.

I used this replacing $maxrole with $int.

for ($i = 1; $i < $int; $i++) {
cache_clear_all($node->nid. ‘::’. $i, ‘cachenode’);
}

And it works fine.

But then my problem still exists. The tables that come with the advcache installation are empty even after I applied the patches. Why is it that these tables are not used? What are these tables for?

raising priority and category level..

guys this is not about "oh what kind of site has over 31 roles"? this bug just killed my prod site when I added role #5.. because its not about how many roles there are (which is a purely unacceptable excuse in my professional mind) it is about where the rid sequence is at. this took down my site because that sequence suddenly was 31. why is absolutely irrelevant.

there needs to be a clear explanation in the code for what the heck 2^max(roles.rid) has to do whatsoever with clearing cached nodes. i defy the coder to provide an explanation to this thinking.

and i really do think this code needs to be downgraded from production-stable to beta. this is most definitely not production stable and there is a serious risk of bringing a site down with such a lurking bug as this.

$int = advcache_array2int($user->roles);

$int is defined but neve used in this module.
can anyone explain?

The thinking is that every combination of roles is unique and the cache key wanted to represent this uniqueness. There's probably a better implementation, though. Try something like this:

function array2int($roles) {
  return md5(sort($roles));
}

As this module is only good for users having only one (1) role, I made a query to check if the user has only one role.

$roles = db_result(db_query("SELECT COUNT(*) FROM {users_roles} where uid = $user->uid"));

Having the value of $roles, we can then check it before we proceed to the clearing of cache. So, I replaced

if (!in_array($node->type, variable_get('advcache_node_exclude_types', array('poll')))) {
with
if (!in_array($node->type, variable_get('advcache_node_exclude_types', array('poll'))) && $roles == 1) { ...

Another point here:

if (!in_array($node->type, variable_get('advcache_node_exclude_types', array('poll')))) {
$maxrole = pow(2, (int)db_result(db_query("SELECT MAX(rid) FROM {role}")) - 1);
for ($i = 1; $i < $maxrole; $i++) {
cache_clear_all($node->nid. '::'. $i, 'cache_node');
}
}
This line of code is good if the maximum rid is low. But in my case, where MAX(rid) is 28, cache_clear_all() would execute 2^27 times. And it will take so much time that it will result a php-time-out.

Studying in depth, I learned that cache_clear_all api is all about deleting the entries stored in the database.
Evaluating the code above, it is just like executing the database command:
$maxrole = pow(2, (int)db_result(db_query("SELECT MAX(rid) FROM {role}")) - 1);
for ($i = 1; $i < $maxrole; $i++) {
db_query("DELETE FROM cache_node WHERE cid = '$node->nid'.::.'$i'");
}
To avoid the loop, I made a query that would practically do the same. But this one, the trick is on the sql and not on php.
I replaced
$maxrole = pow(2, (int)db_result(db_query("SELECT MAX(rid) FROM {role}")) - 1);
for ($i = 1; $i < $maxrole; $i++) {
cache_clear_all($node->nid. '::'. $i, 'cache_node');
}
with
db_query("DELETE FROM {cache_node} WHERE cid LIKE '$node->nid::%'");

So, this is my final code for the nodeapi() of advcache module:

function advcache_nodeapi($node, $op) {
switch ($op) {
case 'update':
case 'insert':
case 'delete':
global $user;
$int = advcache_array2int($user->roles);
$roles = db_result(db_query("SELECT COUNT(*) FROM {users_roles} where uid = $user->uid"));
if (!in_array($node->type, variable_get('advcache_node_exclude_types', array('poll'))) && $roles == 1) {
db_query("DELETE FROM {cache_node} WHERE cid LIKE '$node->nid::%'");
}
// It is unfortunate that we have to use the wildcard here, but it
// comes from the fact that the signature to taxonomy_node_get_terms
// has a $key parameter which goes into building the cache key, which
// we can't reliably reconstruct here.
cache_clear_all('node::'. $node->nid, 'cache_taxonomy', TRUE);
if ($node->type == 'forum') {
cache_clear_all('*', 'cache_forum', TRUE);
}
break;
}
}

This worked out fine on my system. I hope the authors could comment on this.

@jeuelc: did you try my replacement role2int function in #26?

did you mean this:

$int = array2int($role)

no, i haven't tried this. i don't know where in the module is $int used.

Can people please test the approach in this patch and report back? Thanks.

i really think this is moot. see dupe ticket http://drupal.org/node/188085

there's no reason to cache nodes on a role-combo basis, since all access involving node viewing and node lists happens before the node cache is accessed.

function array2int($roles) {
  asort($roles);
  return md5($roles);
}

This is my proposed solution.

this solution is a good replacement for array2int, however it doesn't resolve the issue that, for sites with a high MAX(rid), we end up sending many many cache_clear_all when we invalidate a node.

i propose two additional changes:

first, we can use wildcards on the cache_clear as long as memcache is also in use. this should be the default behavior.

second, on install we should compute all current role combos and store it in a variable.
then, with advcache_user, we should check to make sure a new combo has not been introduced and update the variable if it has.
finally, if memcache is enabled, we can use the variable to send a minimal number of cache_clears.

ahh, except that i don't know an easy way to compute the hash of the role array from the database when we determine the list of valid role combos.

previous best SQL effort was

SELECT DISTINCT(SUM(POW(2, rid))) as sum, uid FROM users_roles GROUP BY uid;

i'm willing to keep the previous implementation of array2int in order to allow us to cache_clear on the smallest number of keys...