Drupal cache clear also clearing Redis

@johan-den-hollander I opened an MR with the patch from #2765895 (with little improvements) I have other ideas, but need to find time to implement it.

I made this feature as an option with the following settings :

You can set flush_redis_on_drupal_flush_cache to TRUE to flush Redis cache when clearing all Drupal cache.

$settings['flush_redis_on_drupal_flush_cache'] = TRUE;

You can set flushdb and flushdb_async to TRUE to use the Redis native flushDB when clearing Drupal cache.

$settings['redis_flushdb'] = TRUE;
$settings['redis_flushdb_async'] = TRUE;

You need to set a redis base properly to use this feature. By default, Redis gets cleared getting all keys using cache_prefix. This is not recommended on production as it could be an expensive task to clear all the keys.

The redis client does not necessarily have databases. For instance the redis cluster cleint #2900947: Implement initial RedisCluster client integration does not have databases.

How ever with redis_flushdb=FALSE one actually does not need the DB index anyway - So I would suggest to move the redis index/DB safety checks and the ->select into the flushdb block.

There is a problem with the patch when prefix is an array and results into an error when logging messages. Correct version in the last else statement should be '@prefix' => $p:

else {
  foreach ($prefix as $p) {
    $keys = $redis->keys('*' . $p . '*');
    $redis->del($keys);
    Drupal::logger('redis')->info(t('Redis Keys deleted for prefix @prefix', ['@prefix' => $p]));
    Drupal::messenger()->addMessage(t('Redis Keys deleted for prefix @prefix', ['@prefix' => $p]));
  }
}

As we don't use MR diff or patch files in our builds as they are dangerous, attaching a patch file to have working static patch.

Forgotten patch file :)

We use the patch on multiple sites (in production). It works as expected.

This is a much needed feature and the patched got the job done!

It bit us in the back on a production env. Wondering why we where still seeing stale cache for objects set to never expire and no cache-tags to help them being evicted in other way.

And yes you are right, it's wrong and somewhat foolish to rely on cache clear to solve this problem, yet is worked perfectly till we moved that project to use redis for cache.

So yeah, please merge this and cut a new release :D

As mentioned above, this patch does not work with RedisCluser

PHP Fatal error: Uncaught Error: Call to undefined method RedisCluster::select() in /var/www/chat-dev/web/modules/contrib/redis/redis.module:61
Stack trace:
#0 [internal function]: redis_cache_flush()

Contributions need to be merge requests.

last comment's that there needs to be an MR. but i see both an MR and a patch ...

can anyone clarify where this actually stands, and what specifically needs to happen to move forward?

With redis 1.10.0 the patch from MR25 isn't applicable, as the README.md had multiple changes. I removed the README.md hunks with this updated patch.

Patch #21 also works on redis 8.x-1.10 (currently using in an Drupal 11.2.3 project)

I have no intention of committing anything like the current patches. keys() based approaches do not scale, there could be hundreds of thousands of keys, also not as an opt-in setting. That is simply not code I'm willing to maintain.

We've been doing this for years as part of our deployment steps:

./vendor/bin/drush ev "\Drupal::service('redis.factory')->getClient()->flushAll()"

What I'd be open to is exposing this as a drush command so it's easier to discover and use. That command could also make it clear that this does not respect prefixes.

Agree that ->keys() is not safe it's plastered all over their documentation.

How about a similar approach using ->scan() then ->del() instead?

I think that Drush command makes sense.

I've been hashing out this problem with @nick_schuch and he's had what I think is a great idea:

1. Create a random hash, store it in state and append it to the site's cache prefix so you have SITE_PREFIX:HASH:rest_of_key
2. On cache clear, change the hash and
3. Trigger an async garbage collection of all keys using the old hash using scan and unlink which are both non-blocking

I'm working on implementing that in our site, if you think it would stand a chance of being committed to the module please let me know and I'll create an MR :)

We can't store a hash in state. Redis is on a way lower level than that. Redis stores the container and state cache, that's recursive. We could store it in redis, but that's an extra lookup per request.

The module already tracks a per-bin flag to track deleteAll(), a cache clear does that for all bins. The implementation is absolutely consistent to Drupal. And with improved ttl configuration (per bin) in the latest release, you can also guide Redis in first cleaning up data from less frequently used and larger bins such as page/dynamic_page_cache/render and store them for shorter time than other bins.

All caches with a stable key that don't vary as much will replace existing keys and their stored data.

You can also vary the prefix on a deployment, the default prefix includes the deployment identifier.

I'm still struggling to understand use cases why you'd go through that trouble.

The problem I am facing is that the module invalidates cache entries by comparing them to a timestamp. This results in a huge amount of stale cache entries being queried and transmitted then discarded by the module, rather than a much quicker "not found" response (once you add them all up).
There are definitely areas in the project where improvements can be made but this change will considerably reduce request times for us.

You're right about state, that was my bad, I still hadn't thought through the full extent of the change.

We could do without the garbage collection, want it there so I can see if there's any headroom in our redis instance in various envs.

We also have no control over the cache strategy with our host, so lfu is not an option I'm afraid, much as I wish it was.

And are you sharing that Redis instance between multiple projects or not? If not then I would go with a full flush. (FWIW, I think that sharing it, especially between non-related projects, is problematic from a security perspective as using the prefix i just convention and there are known bugs, such as queue currently not using the prefix).

Also, if you are mixing this with persistent data (queues) then you'd lose data, both with a scan based approach and a flush.

The fetch data part is a one-time thing per key and then it will be replaced with valid data. Cache tag based invalidations are the most common way to invalidate stuff for render-caching based things (which are by count/size the majority), so this will happen much more likely than after a cache clear. A full cache clear should be a rare occurrence, typically on deployments only. Are you sure that's worth optimizing for?

I'm working with the host to see if it's feasible to split up our environments, I didn't know that about queues, yuck, thank you. If we do succeed in separating instances then obviously a flushDB is the way to go, yes.

Thank you so much for your feedback, it has been very helpful, and I can see that if I do go with some form of cache prefix fix (much less likely now I know about the queue issue) it won't be useful generically, so I'll stick with the project-level implementation if it comes to that.

Drush command sounds good as a resolution to this ticket, might want to build the call into the client proxies though, cluster and standard clients do flushes slightly differently.

Hey @berdir

I wanted to clear this one up from the security perspective.

And are you sharing that Redis instance between multiple projects or not? If not then I would go with a full flush. (FWIW, I think that sharing it, especially between non-related projects, is problematic from a security perspective as using the prefix i just convention and there are known bugs, such as queue currently not using the prefix).

We don't share Redis instances between multiple developer projects eg. Site1, Site2 etc.

Each project get's a production and non-production instance of Valkey (Redis) to provide resource and permission isolation.

Just in case anyone's lurking here and interested - we did end up varying the cache prefix on cache clear by writing it to a file in private storage. This followed by garbage collection via cron allows us to see how quickly the cache refills and provides confidence that it is being used to its full potential. We've been using cache prefixes wiht multiple dbs in non-prod for years, this doesn't change anything about that.

Thanks @nick_schuch and @darvanen for sharing your solutions. They would be ideal candidates for a documentation page or blog post, briefly outlining the set up :)

+1 for a Drush command. I tried the Drush command from comment #23 and that works well, and would be a nice improvement.

Should it be added in a new branch here, or perhaps in a fresh issue?

Following this discussion for a while, there's one thing I don't get: what's the benefit of clearing Redis at all?

We're running Drupal sites on shared Redis instances, so flushAll() on deployment is a no-go. On the other side, scanning all keys and deleting them one by one is a costly operation, as berdir mentioned.

So why do this at all? As far as I understand the code, drush cr uses drupal_flush_all_caches(), which calls the deleteAll() method of all cache backends. The Redis module implements this method, invalidating cache entries using a timestamp instead of deleting them. That's ok, imho, as it makes sure that cache entries will be rebuilt after a drush cr. What's the benefit of additionally emptying the cache bin?

Two reasons for my purposes:

1. If you just rely on the timestamp, for a significant time after the cache clear redis will be transferring all of those cache entries to Drupal, just for Drupal to discard them and rebuild, which is a performance hit our site cannot afford.
2. The cache hit rate metric records false positives and gives the wrong indication of how well the cache is utilised.

@darvanen I'm not sure, I get this right, so

If you just rely on the timestamp, for a significant time after the cache clear redis will be transferring all of those cache entries to Drupal, just for Drupal to discard them and rebuild, which is a performance hit our site cannot afford.

Don't they need to be rebuilt anyway, whether deleted or invalidated?

The cache hit rate metric records false positives and gives the wrong indication of how well the cache is utilised.

Who does the metrics? The PHP extension, or the Redis module? The first one wouldn't know about the invalidation, correct?

Fair questions @drubb :)

Don't they need to be rebuilt anyway, whether deleted or invalidated?

Yes, but they don't need to be transferred, with our (gargantuan) site that's many MBs of traffic slowing down requests, particularly as each fresh user (there are hundreds of editors) logs in. Yes there is some work to be done on our cache contexts etc but it was also important to squeeze every last drop of performance out of this service, for us.

Who does the metrics? The PHP extension, or the Redis module? The first one wouldn't know about the invalidation, correct?

You're right that Drupal itself would be recording cache hits correctly but not the cloud service where we do most of our monitoring.

I think *in general* that the way the module does things is good enough, but for us "good enough" just isn't good enough ;)

FWIW, transfer of large chunks of invalidated cache data is pretty unavoidable with cache tags. If an invalidation of a high-impact cache tag such as node_list (when not having customized that), rendered, route_match or so happens, then there's no way around that.

I assume you're already using the new ttl offset feature, compression and possibly igbinary? With compression, large cache items are reduced up to 5-10% of their original size based on my testing and only uncompressed when still valid. redis 2.x also features a new drush report that will check how much of your cache is currently expired/invalidated: https://project.pages.drupalcode.org/redis/#drush-report. I just realized that doesn't account for deleteAll() of a bin yet, that could be useful but should also happen pretty rarely. I also haven't tested it for >1GB redis memory limits.

Re #34, the main benefit for me is to have a clean slate for performance investigations, specifically when deploying changes that can impact that. But we're using isolated environments with dedicated redis/valkey container for each site and environment (upsun), so flushall works fine for us, would never bother with a manual key-by-key delete.

Oooh thanks for the tip re igbinary, will investigate further 🙏

See https://project.pages.drupalcode.org/redis/#using-igbinary-serialization and #3014514-21: Make igbinary the default serializer, if available.