Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature Request: Add a filter/configuration to disable cache on queries #139

Open
engahmeds3ed opened this issue Jun 23, 2022 · 3 comments

Comments

@engahmeds3ed
Copy link

I know that using cache for queries is very important and makes the process faster but we have a case that we need to disable it for one query.

The case is that we have a code that inserts multiple fake posts on the same request and with pre_post_update hook we have our callback to make a query to make sure that we don't have a record with some criteria to do action with it so when the cache is enabled, it inserts the first one and with the second one the query gets the data from the cache.

We temporary can solve that by using the following hook
https://github.com/berlindb/core/blob/master/src/Database/Query.php#L871

then use the WP function wp_cache_flush but this will flush the whole cache :(

I know that you try to eliminate having short circuit filter into the plugin so we may have an argument to disable the cache with default value is false.

If u agree I can start PR for this one.

@JJJ
Copy link
Collaborator

JJJ commented Jun 23, 2022

Makes sense to me. Good idea 👍

With this implemented, the get_results() method could finally become a wrapper – right now it duplicates a lot of existing stuff (like Queries\Compare, etc...)


In the meanwhile, look into using wp_suspend_cache_addition(). You could suspend cache addition before you start, which would prevent subsequent queries from finding results in the cache (and also writing to it) until you unsuspend it when you are finished.

// Prevent cache additions
$old = wp_suspend_cache_addition( true );

(...do your things...)

// Restore cache additions to whatever the state used to be
wp_suspend_cache_addition( $old );

Relatedly, this reminds me that Berlin uses wp_suspend_cache_invalidation() on adds and sets, but WP_Object_Cache only uses it for calls to add() and not set() or replace(), and it's worth making doubly sure Berlin is using the correct calls in the right places.

@JJJ
Copy link
Collaborator

JJJ commented Jun 24, 2022

I went outside and did some exercise, and am coming back with more thoughts.

You can tell me truthfully if they are not good ones. 🤣


There are areas of improvement in WordPress itself with how the wp_suspend_cache_addition() and wp_suspend_cache_invalidation() functions (and the $_wp_suspend_cache_invalidation global) are implemented:

  • The two functions work differently, are used differently, and are not alike internally. This makes it easy to use them wrong, confuse them for each other, and make incorrect assumption about their intended usages.
  • $_wp_suspend_cache_invalidation is a global (introduced in WordPress 2.7) that is only used inside of the functional/application layer (the clean_*_cache() functions) of Sites, Networks, Posts, and Terms; it is not used for Comments, Users, Options, or Meta. Using this global requires touching it directly because the helper function –wp_suspend_cache_invalidation() – has it's own problems (a quick search through the WordPress codebase reveals it doesn't even use it 😅)
  • That global is used in functions, largely due to historical reasons, and only to have a single global way to prevent the WordPress Importer from melting Memcache(d) and Redis backends.
  • The wp_suspend_cache_addition() function (introduced in WordPress 3.3.0) works how I think most developers would expect it to – calling it toggles an internal static variable on/off; passing true|false toggles it on/off; passing null returns the current static variable value. It's only problems are that it's used at too low of a level API – inside the WP_Object_Cache class – which means every drop-in needs to implement it separately and independently, and I know they haven't 💀
  • The other problem with wp_suspend_cache_addition() is it's only used for add, and not set or replace, so cache writes are not totally off; only new entries. (It is also worth noting that replace didn't get a _multiple() variant, and I've made a core ticket to add it.)

All of this is to say, that – I think – if we do this in Berlin that we should add query variables to help influence/control/toggle all of the cache interactions, and not just object/Query reads or writes, and probably not using WordPress core's approaches, because they aren't really all that awesome currently.

Up to and including adding our own Cache.php file with its own Class and variables to help us work with and abstract everything out in some logical way.

Thoughts?

@engahmeds3ed
Copy link
Author

WoW that's a great explanation John, many thanks.

I totally agree having a cache class to have all the cache attributes and methods that can be used in all other classes and we can start with the Query one and see how it goes from there.

do u think that this cache class should have static methods like Utility class to be easily embedded into other classes or a normal class that somehow can be injected into their constructors?

I will invest this weekend trying to think about this new class and the data should be there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants