Make cache backends responsible for their own storage

This issue is (in part) a duplicate of #496248: Refactor cache API. If we expose cache bins in a hook, we can use that hook to flush caches (and replace hook_flush_caches()) and we might use it to create and drop bins dynamically, depending on the backend used for each particular bin.

Wouldn't adding a hook here make a late-bootstrap dependency for the cache system, which is by nature needed very early in bootstrap? What's the behavior when you try to read/write a cache bin that doesn't exist?

I don't like the createBin() and dropBin() functions. What's would be really good is #1 suggestion, forcing modules to set their bins in a declarative manner, so the core would be able to create/drop/clear them with or without the intervention of god, and without the implementation of hook_flush_caches() and such (which need to exists for side tasks like clearing some other stuff like cache itself).

The need to have a database table under the hood for each bin, whatever is the cache backend using it is really important because it allows a runtime fallback on database if needed.

createBin() and dropBin() makes sense, but as API protected functions, because maybe some backends would need some kind of initialization, while core doesn't care about specific implementations therefore shouldn't mess with it: core doesn't have to run the create order, the backend specific implementation must complete its init by itself if needed, probably in some kind of lazzy algorithm.

@#2, because core has a hook for bins doesn't mean that it needs to be run at early bootstrap. Actual system already needs the module to create their cache tables at install time, which means that, whatever we proceed to this declarative system or not, core will end up creating the tables itself instead of letting the module doing it. The result in case of hitting a non existing bin would be exactly the same: an ugly warning/error in watchdog.
Another solution could even have an exception based error handling that throws some kind of NonExistingBinException when trying to, but this going way further than the initial post IMHO.

catch: No, I mean if we're using a hook to define cache bins, how do we initialize the basic bins (cache, cache_bootstrap, etc.) initially? Especially if the installer runs with its own custom no-op cache system in the first place?

(I'm overall fine with this cleanup in general, just trying to deal with the implications.)

For core related, they are basically set up at system module install time (site installation) it could stay there, set up them doesn't mean use them at install time. The "registry of bins" could have those per default (as hardcoded values) as start before finding out a more elegant solution for early usages?

I totally vote for the declarative hook! It would avoid so much typo errors (and we could log non existing bin usage attempts somewhere) while definitely get rid of SQL errors when trying on a non existing table! I thinks this is the really first step to improve the overall cache handling.

Any other improvement is welcome I think, and I really don't care if the error handling is exception based or not, basically it just have to be really performant because it a essential/critical and early bootstrap related code.

Bin creation was not part of my original patch.

In my patch I think I not only considered DB tables as bins, but compressed CSS and JS files as well. It would be cool to have one uniform way of handling cached data.

@#9 Compressed CSS and JS files are not cache, they are resources (as called in most frameworks out there). Assets are public static files, once they have been generated they are meant to be spread over CDN's and stuff, and therefore shouldn't be treated as volatile cache entries.

Even if CSS and JS aggregated files have a shorter lifetime than a real asset (such as user uploaded file), if a particular frontend starts to cache full pages (Varnish, Akamai, ESIGate, etc..) the static outdated files should remain alive too, in order that a client that reach an outdated cache entry never ever see his site without CSS applied.

Cache bin are mostly serialized binary data only used at runtime in order to achieve the greater good, which is compute the final rendered page, while compressed CSS and JS are 100% part of the page itself therefore are not volatile anymore (on the opposite to cache, once cache is destroyed, the underalying API will lazzy build it again, and the user won't see nothing) while CSS and JS files, once destroyed, cannot be restored because they mostly are being called from an external source (and not while building the page).

I disagree with the fact those two really different problems should be mutualised.

The try part of a try-catch block is super cheap. Creating and throwing an exception is expensive, because the exception has to create a backtrace. The question is what we define as the API for cache_set().

1) "If the requested bin doesn't exist, an CacheBinNotFoundException is thrown." - That means you have to have your own try-catch to avoid Drupal dying.

2) "If the requested bin doesn't exist, a watchdog entry will be logged and the cache will respond as if it was simply a normal cache miss." - This means Drupal will "almost silently fail".

Either way the DB cache would likely be faster with a try-catch block internally than with an explicit table_exists() check, since 99% of the time it will succeed.

Mostly agree with #11 on this. Exceptions probably boot performances if you have to unstack 100 calls, but it make you loose time if it's only two (this needs to be verified, but if the JIT/VM/? is properly implemented, it should probably use a setjmp()/longjmp() alike system, which is ultra fast exception like error handling in C).

That's why I proposed the isHealthy() check, that every implementation would give (database based cache would just tell TRUE anyway, because a missing table here is not something that should happen, you can run your site on a downgraded env. without memcache, you cannot do the same without the database).

EDIT: This is why I came here: http://drupal.org/node/1164484

Subscribe

catch: Your thoughts on #11, vis, the API of a "bin miss"? I'm inclined toward option B, "silently fail with a log entry", as forcing everyone to wrap try-catch blocks around every cache get is going to be a losing battle.

Missing table would be a module install problem, not a cache problem. These kind of database errors should explode at the developer's face, not downgrade silentely an innocent man's site. Database cache is not depending on any external dependency, therefore a missing table is a real error that should not happen.

A db_table_exists() in the catch is fine, as that's already the edge case (presumably).

So you're expecting the cache system to be self-healing in this case? Something like:

DrupalDatabaseCache {
  function getMultiple(&$cids) {
    try {
      // ...
      $result = db_query('SELECT cid, data, created, expire, serialized FROM {' . db_escape_table($this->bin) . '} WHERE cid IN (:cids)', array(':cids' => $cids));
      // ...
      return $cache;
    }
    catch (Exception $e) {
      if (!db_table_exists($this->bin)) {
        db_create_table(...);
        watchdog(...);
        return $this->getMultiple($cids);
      }
    }
  }
}

I am not against it in concept, but I worry about cleanup when uninstalling modules. (And actually, the bin check may make more sense to do in the constructor rather than on get, but that's an implementation detail.)

Yes, at least it keeps the specific error handling into the specific implementation!

If the cache object is self-healing, why would we need a hook? Just start throwing stuff into cache_foo, and it creates it as needed. The only issue then is module uninstall where we need to clean up such tables. Perhaps we can get away with just a ->deleteBin() method (and corresponding createBin() method for internal use), and modules are expected to clean up after themselves?

This issue actually is about two different, but related things:
- How do modules expose/create their cache bins?
- How do cache backends handle cache bins?

Modules exposing cache bins is not mutually exclusive to cache backends being able to dynamically create or delete bins of their type. If a bin is set to use backend X, but X does not have that bin yet, it needs a fallback. This is common to all backends, since DB caching is just the default backend and should not be used as a fallback.

//Edit: double post

Modules exposing cache bins is not mutually exclusive to cache backends being able to dynamically create or delete bins of their type. If a bin is set to use backend X, but X does not have that bin yet, it needs a fallback. This is common to all backends, since DB caching is just the default backend and should not be used as a fallback.

More precisely, the database bins are the only one for which the logic is not handled by the cache specific implementation itself. The clean way to proceed is to let the "bin" abstraction into core but remove all database code (from hook_schema() and other places) from core and modules and let only the database backend do whatever it has to do with it (creating and removing tables).

The only thing that can/should sweat from the core is "module X that handles the Y bin has been removed, please all cache backends remove it!".

The correct issue title should not be "Allow cache backends to create and drop bins" but "Make each specific cache backends responsible of their bins transparently".

Yes agree for the flush and garbage collection hooks.

If we want to allow bins to handle their own GC, then don't we need a global list of known bins?

Each bin may be on a different driver, and each driver may not know for certain all of the bins assigned to it. So somewhere we need a master list of "bins that exist", and then tell each one "bin, clear thyself!" (And also, possibly, "bin, kill thyself!")

I suggest hook_cache_bin_info(). It complies with the standards of keeping names singular and adding the "_info" suffix to info hooks.

+1 to #28, #30

I'm still not a huge fan of Yet-Another-Hook (henceforth YAH), but a proposal:

function hook_cache_bin_info() {
  // Deliberately over-verbose for later extension.
  $bins['page'] = array(
    'bin' => 'page',
  );
  return $bins;
}

interface CacheInterface {

  // A cache object knows its bin.
  public function __constructor($bin, ...);

  // Create and initialize the bin.  Usually only called internally but a legit public operation.
  public function createBin();

  // Remove a bin from the system.  This is probably a no-op on many non-SQL implementations.
  // Return TRUE if the bin was removed, FALSE if it wasn't there to begin with.
  // (Removing a non-existent bin is not an error condition; that's consistent with the DB schema API.)
  public function removeBin();

  // If the bin doesn't exist, call $this->create() and make a non-error watchdog note.
  public function getMultiple();

  // If the bin doesn't exist, call $this->create and make a non-error watchdog note.
  public function set();

  // Remove a single cache key, or subset.
  public function clear($key);

  // Clear the entire bin.
  public function clearAll();
}

- On module install... do nothing. All bins lazy-instantiate. The schema for the DB cache implementation is not even in schema hook, just in the SQL cache implementation.

- On module uninstall, check for all bins declared by that module. Load whatever cache object is set for that bin, and call remove().

- If a module created a bin on one backend and then it was reassigned to another backend, the admin is responsible for cleaning up the stale backend, not us.

- If you try to write to a cache bin that isn't declared, throw an exception. I'd rather that to watchdog as it is a sign of a significant bug in your code to begin with, probably a dumb typo. If you're using a dynamic bin name, then you're abusing the cache system and need to stop doing that.

Thoughts?

Seems rather good to me, and I like the fact you dissociated clear($key) and clearAll() (maybe clear all could be called empty() by the way).

Appart from this totally off-topic remark, yes, seems a good way, only one thing is bothering me, is that the fact the core is responsible for giving the remove() order, but not the create() order, I think the layer that is responsible for it (either the backend itself, or the core) should be responsible for calling both of these functions. It would make a lot of sens that core triggers the create(). Usage of a non existing bin would then raise an exception (not lazy creation) and alert the developer he is doing it wrong (it definitely makes the error evident).

Oups cross post removed the tags, and I can put them back, sorry.

catch: Fair point on the circular dependency in the installer. My concern, though, is that it effectively makes the hook optional. Nothing *actually* breaks if you don't declare your cache bin, so it can never be properly garbage collected. That said, if we decide that's the lesser of two evils (like all those modules that forgot to clean up their variables in hook_uninstall()) then I can live with that.

I would love if we can go as far as even removing watchdog() function calls from cache objects. Any time we can push as far as an object that has zero external function calls, that is the gold-standard to shoot for. (Not always achieveable, yet, but that's where we want to end up for modularity and testability reasons.)

For the structure of the hook, we could but I prefer to leave ourselves an opening for later. We did the same for hook_hook_info() in expectation that sometime in the future we could add hook weights there, and it would be a backward-compatible change. I could see us adding things like default expiry ages to a given bin in this hook, in which case I'd rather not have to change that API even more times.

I may have mentioned it already, and it may be obvious if not but a follow-up goal would be to do a similar thing for queue, lock and session.

I don't believe you did, but that makes a lot of sense to me.

- Lazy bin instantiation might be good, so bins can easily switch backends, but we will have to document that well to prevent WTFs.
- Because bins can change backends at any moment, we should call removeBin($bin_name) for every backend when a module is uninstalled, to make sure no unused bins remain.
- As far as I can see the hook_cache_bin_info() proposal in #32 requires developers to set the bin name twice: both as key in the parent array and as a value in the bin definition. This is duplicate data and when used or altered incorrectly or inconsistently, this means trouble.

This issue has a pretty good shape, and I think it's time to integrate this one: http://drupal.org/node/1164484#comment-4728356 as well. Having an isHealthy() check over cache backends allows to implement the lazy table creation in it, and return FALSE if creation didn't succeed. What do you think?

Aside from the initial and minimal review that follows, I don't like this change proposal.

It mixes high-level (hook/module system) logic into low-level, pluggable cache system logic. I can't provide factual evidence for where and when this will blow up, but my gut feeling on these changes tells me that it will blow up.

It's not only the interdependency between application layers, but also the idea of moving storage container/facility logic within and into data/CRUD controller logic -- these are fundamentally different tasks happening on entirely different times and levels of the application stack. A data/CRUD controller is dumb. It must be able to rely on the fact that the storage it attempts to read from or write to already exists. The approach taken here looks way too "dynamic" and not reliable nor maintainable to me.

+++ b/includes/cache.inc
@@ -464,60 +476,89 @@ class DrupalDatabaseCache implements DrupalCacheInterface {
+    foreach ($this->schema() as $table => $definition) {
...
+    foreach ($this->schema() as $table => $definition) {

$this->schema() doesn't return a multi-dimensional array

+++ b/includes/cache.inc
@@ -464,60 +476,89 @@ class DrupalDatabaseCache implements DrupalCacheInterface {
+      if (!db_table_exists($bin)) {

$bin is undefined,

-12 days to next Drupal core point release.

I think it's time to wake up this issue.

hook_cache_bin_info() is a good idea.

On the fly table creation doesn't sound a good idea: the cache table should be created per default during the module install (and removed during the module uninstall).

The cache bin info must live in .module file, it will be a very lightweight hook and can be run each request without being itself stored in a persistent cache (static cache will be enough).

It will fix this http://drupal.org/node/636454#comment-6128574

Having a registration based info hook for bins allows us to get rid of most hook_flush_caches() implementations (in favor of hook_cache_bin_info()) calls.

It also enable possibility to create full UI for cache analysis/handling etc.

Non existing bin should raise logic exceptions when requested: more robust code, early errors detection.

@pounard: See my patch in #496248: Refactor cache API for most of what you suggest. We shouldn't forget a hook_cache_bin_info_alter() so websites can easily switch a particular bin from DB cache to memcache, for instance.

Also, if we go forward with this issue I'm willing to pick up where I left off.

@Xano sounds good, why wouldn't we migrate your patches here then? @catch's opinion as some value here too, he was the one at the initiative of a series of issues about cache including this one.

I briefly overlooked at your patch, maybe we should start without the UI additions and only with the bin registration mecanism, then open a new issue for the UI?

Picked this up. Will start from #44.

Here's a patch for testing.

Summary:

• Cache bin tables are registered now in module .install file using hook_cache_bin_info().
• When a module is enabled it automatically installs the cache bin table(s). On uninstall will drop the tables.
• Cache bin tables are consistent as naming convention (pattern: 'cache_bin_' . $bin).
• Main 'cache' table was renamed to 'drupal' as bin (table name: 'cache_bin_drupal') for consistency reasons and API cleanup.

A recent Twitter conversation leads me to this interesting question...

We originally added separate cache bins to the cache API back in 4.7 or so, to avoid cache clears wiping out too much at once. Vis, we didn't want a page cache flush to also wipe a filter cache or block cache, or vice versa. And we moved each bin to separate tables in MySQL. Or rather, we moved caching to multiple tables and have since started calling those separate tables "bins" as we try to pretend that the cache API started as an abstraction rather than just an SQL implementation. :-) At the time it was a good move, as it allowed Drupal sites to get bigger without suffering from cache stampedes as badly, and it meant that the SQL query cache for one cache bin/table was not affected when we blast away a different table.

However, that was then, this is now. These days, sites of any respectable size, those large enough to care about optimizing the SQL query cache, are not using SQL caching anyway. They're running memcache, or Redis, or something better performing than SQL. And those systems don't have, generally, as much manual setup around cache tables that the SQL implementation does.

So why do we still carry around the complexity of such SQL optimization?

Rather, if we changed the SQL implementation back to being single-table, with a "bin" column, then we completely eliminate the need to manage/coordinate creating/deleting tables at different times. The "bin" is implicit, and we don't need to create anything new. That simplifies the implementation and allows us to remove a fair bit of race conditions. (Can I use this table before it's been created?) The downside is that the SQL cache implementation becomes a bit less performant, but... if you care about cache performance that much you're not using the SQL cache implementation anyway. Times have changed and what made sense to optimize 6 years ago no longer makes sense to optimize. We should treat the SQL cache implementation as a fallback, much like the in-PHP HTTP caching we're inheriting from Symfony (http://drupal.org/node/1597696), on the assumption that sites that really care will have a real Varnish in front of them rather than a simulated Varnish.

Would that sort of approach not greatly simplify (or even eliminate?) this issue entirely?

I think that the bin registration is still something we want, because we want a unified single list of all registered bins so we can flush on cache clear, provide listings in admin UI, etc...

Regarding high-volume-performance-wise sites using external cache store, I don't agree, I'm myself working on sites that are quite big enough but still using database cache due to technical restrictions on the environment it's running in, so I'd like database cache to remain performant as it is, separate tables is the right way to keep this.

What we could do is get rid of createBin() and dropBin() from the cache interface, and keep the SQL tables creation in the system module, system module then acting as the provider of the database cache backend, preparing the environment as it should in an encapsuled fashion, reacting to hooks.

Regarding the patch, we shouldn't do the try/catch workarround, because try/catch as some kind of cost on runtime, and it is totally unnecessary since the first thing you do after each upgrade is hitting update.php, and not the site itself, site should never be hit directly after files upgrade without hitting the update.php first.

I'm not fond of renaming all 'cache_*' to 'cache_bin_*' because 'cache_' is explicit enough and a well known convention in Drupal.

I mean, having one table per cache bin doesn't sound that bad too me: the only downside is that modules must take care of creating and removing their own table. This sounds inconsistent to me, because one module just declare a bin for the cache system to provide a new namespace. Since backend is an implementation detail and the module just need to use an interface, the module should not care about the table itself, it's only a SQL implementation implementation detail. Having the system module managing the tables instead of custom module doing it just ensure this encapsulation. If we consider that the system module provides the SQL backend, and because the backend internals is an implementation detail that should be encapsulated, then it's normal to say that the system module *must* ensures itself that those tables exists when a new bin is requested. Core itself doesn't really care about SQL, core brings the interfaces for modules to use. The fact that the SQL implementation is inside the core namespace and not inside the component namespace confirms that the SQL implementation is not part of the publicly exposed API but is an specific encapsuled implementation: as such, any implementation detail must be hidden from modules, tables included. I don't care then who needs to create/suppress them, but I would naturally say that it's the system module responsability, because it's also the only module that creates and manage system tables.

pounard: With a bin column (which we didn't have in 4.6), we can still clear each bin independently. All we loose then is the query cache, which is not that big of a deal, and at some point we need to cut our losses and focus on an enabling API, not micro-optimizing a degenerate case. And it means the create/drop table management question goes away completely; the whole "whose responsibility is it?" question ceases to exist.

I like @Crell's proposal.

Ideally, we'd leverage k/v for it and simply turn 'bin' into 'collection'.

Second, if we manage to still bear in mind the use-case of a DatabaseStorage, then we perform the necessary plumbing on the application layer (not cache layer) in a way that notifies the system about bins that need to be added or removed.

In turn, people on environments that don't support alternate stores/backends, but yet have a decent amount of data to cache, can swap out the cache implementation with an alternative one that uses separate tables per cache bin (provided by contrib, not core).

That would keep core simple, but still allows for alternate implementations.

I'm kinda against relying on contrib. And I don't see the k/v fiting for this purpose since it doesn't carry half the business of cache backends.

@#60

micro-optimizing a degenerate case

And all the people that uses Drupal on shared hosting too.

And it means the create/drop table management question goes away completely; the whole "whose responsibility is it?" question ceases to exist.

This is certainly not true, because when you uninstall a module that declares a bin, you still have to clean up after it.

I understand this idea, but let's face it, the cache system is now working very well this way, and if this is only the matter of creating and dropping tables, I'm not concerned about keeping that.

function system_modules_enable($modules) {
  foreach ($modules as $module) {
    if (module_implements($module, 'cache_bin_info')) {
      foreach (module_invoke($module) as $bin => $info) {
        db_create_table('cache_' . $bin, some_function_that_gives_me_the_schem());
      }
    }
  }
}

function system_modules_disable() {
  // Same as upper with "drop" instead.
}

Doesn't sound too hard for me.

That assumes you're using the SQL backend. If not, then you have system.module creating and dropping unnecessary tables. It's also harder to unit test.

The cleanup process with a single table is easy: Meh, don't worry. The next time you run a flush-all-caches everything gets wiped anyway, regardless of bin, so that will clear anything left over from the previous module. Which will likely happen on module-disable anyway, so it's not even a delay.

I think I agree with the code snippet of #63, and I'd address #64 through this:

-        db_create_table('cache_' . $bin, some_function_that_gives_me_the_schem());
+        cache($bin)->create();

In words: The built-in DatabaseBackend would probably be a no-op, an alternate SeparateBinDatabaseBackend would create a {$bin} table.

More than 40% of my Drupal sites are still using standard table caching. And if I need to quickly build a new website having some specific functionality but not being under stress, I would not implement Memcache because I'm happy with table cache while there is no problem with the performance.

Going to "one-table-cache". Relying on SQL cache doesn't mean that performance is not an issue at all. If I remember well the reason for multiple cache tables was that queries against small tables are faster. I'm for keeping multiple cache tables. What I would review is if we want to keep update and form temporary storage in cache (see comment bellow).

Meanwhile I'm coming with a new patch. What is different:

• Removed methods createBin(), dropBin() from interface while we are using the only for standard SQL cache tables.
• Even I'm for renaming tables to cache_bin_* pattern (because cache bin tables need to be named different from others like cache_tags), I had to quit doing this. Renaming those tables it's a nightmare when trying to test. This is because of how it's build the temporary storage of update.module. That particular one is implementing its own API, directly to the DB backend cache, accessing tables directly with SQL. In my opinion update cache is not really a cache! It's something that we might want to name "temporary storage" and (like form cache) it should be moved out from cache. Cache is something that can be wiped out in any moment without affecting functionality but only performance. Update tests are crashing there because the code is using cache_bin_update but the tested SQL snapshot is still having cache_update. It's too much to workaround for a such a small gain. So, back to cache & cache_* patterns.

@#65 I would not add those methods on the backend interface, because I often switch from one implementation to another during site life, and if I do this, the hooks won't be run a second time, and I start with a memcache backend, then change per configuration to the SQL backend, the table will never be created. I'd prefer to see those empty tables even if unused, because they can always serve in case of system downgrade to SQL because something else went wrong.

cache($bin)->createBin($info['description']);

Is wrong, because the backend might be another one per configuration than SQL, and once the module enabled we cannot re-trigger the table creation upon configuration change. I think the table schema is not an instance method, but a class method (static) of the database backend. It is a utility function which is used only @enable/install time and has no sense to live as an instance method.

Aside of that, I like this patch.

I don't see why should we complexify the runtime code by adding try/catch blocks, install procedures and such whereas having 10 empty tables in the tablespace won't hurt a bit and actually works very well.

EDIT: The only cons I can see to this is that maybe some other backends will need init too, but if they follow the system module principle, it will work gracefully.

Fair enough, but when a site has 200+ tables (real use case too) where cache tables are only 5% of them, I don't think that cache bins are the right vector to attack to try to fix the problem.

I do understand your point here, and I agree, but I want the solution to be graceful when changing the backends on a live site, and I want it performant enough for sites that don't use anything else than the database.

On a commerce site I'm actually working on, with separate tables everything is fine, but if I put everything in the same table, I will expect to quickly reach more than 10 million rows (fields, blocks, pages, menus, leaving some other outside): in that case, if my bootstrap cache for example ends up in the same table as the others, loosing some bits of query cache and force all lookups on a 10 million rows table is not something the site will support, at all.

On the other side, the same site uses some apc backend cache (bootstrap and cache only), if apc fails (sometime we have to deactivate it for debugging, I bet it could happen later on the live site if we need to put in downgrade mode) I need to be able to switch the backends without writing any code, nor running anything that might alter the site configuration or data outside of the settings.php file.

Do you think there is a compromise between all that was proposed in this issue? The try/catch runtime check would work in the end, but is this performance wise? Wouldn't it be too aggressive and catch unrelated exceptions and make the site behave wrongly? If you feel it's OK, I'm would be OK to switch to this method and test it.

Off-topic note: In the end, I don't want to bet on the "contrib will fix that" speach, it's a very wrong speach, saying this means "I don't care about what we do, we're not responsible for the code we provide" and that's something I'm generally frustrated about in Drupal, people don't care about other real use cases than their own.

Ok, indeed I know have a better understanding of your problem. Let's try the try/catch solution then, after all that was the kind of things I proposed a long time ago regarding broken backend detection, let's do that.

The try/catch approach is wrong IMHO -- that was the main offender in the original patch already.

The for loop in #69 looks acceptable to me. (Heck, we could even provide a UI for switching backends. In the end, the implementation to use needs to be saved in the config/container anyway.)

Also, to clarify: I'm especially eager on the single-table implementation, since that would eliminate a lot of needless DDL for tests.

The single table implementation will drop the query cache and cause varchar indexes problems, for both read and write operations, in tables that will grow potentially exponentially depending on all the user, page, field and entity count. Single table is a no go because even on small sites it will be a huge performance regression, especially using MySQL.

I'm not fond of the try/catch approach, but right now it's the easiest solution that take care of both performance and tablespace bloat problems exposed by me and catch.

I'd prefer the pre create table by the system module, but it will cause catch huge problems because of his tablespace bloat problems, using a single table will kill the site I'm working on (and potentially *a lot* of others). Do you have any path we didn't explore?

EDIT: Considering providing a UI, I'm not fond of this idea neither, even if it's in the container, the container is not a miracle box: cache backends are early instanciated beasts and will need some love throught a static configuration such as the actual $settings.php, just like the database.

sun: You need to explain why try-catch inside the DB cache implementation is wrong, because it solves the coupling problem, performance concern, and "use the cache before the table is there, boom!" problem.

I can live with try-catch or with single-table, because in both cases it pushes the cache schema management exclusively into the DB cache backend driver, which is where it belongs.

This whole issue is currently tied to the DatabaseBackend. However we actually need a way to register cache bins for all backend types anyways. So IMO this issue has to be solved at a more generic level. We need to know which bins we currently have in our system (even non-DatabaseBackend bins) to run ->invalidateTags(), ->garbageCollection() (dunno when tho :P), etc. properly.

I agree, the bin info list is necessary for us, the database table creation problem is actually more like a parallel issue of this one.

This is also kind-of related to #1772682: Remove the variable_get inline in CacheFactory::getBackends

+++ b/core/modules/system/system.module
@@ -3496,7 +3496,9 @@ function system_cron() {
+  // hook_cache_bin_info() lives in .install files, so load them.
+  module_load_all_includes('install');
+  $cache_bins = module_invoke_all('cache_bin_info');

Is there any reason to put the hook into .install files though?

Unless I'm mistaken, we're loading .module files early enough already?

+++ b/core/modules/views/views.install
@@ -15,16 +15,10 @@ function views_install() {
-function views_schema() {
-  $schema['cache_views_info'] = drupal_get_schema_unprocessed('system', 'cache');
...
+function views_cache_bin_info() {
+  return array('views_info', 'views_results');

Eliminating cache bin schema definitions from hook_schema() has an interesting consequence though: Doing so technically requires us to make update.php drop/destroy all [previously known] cache bins (entirely), and re-create them [the current] with their current schema definitions.

+++ b/core/includes/module.inc
@@ -710,6 +710,13 @@ function module_uninstall($module_list = array(), $uninstall_dependents = TRUE)
+    // Remove any cache bins defined by the module.
+    if ($bins = module_invoke($module, 'cache_bin_info')) {
+      foreach ($bins as $bin) {
+        cache($bin)->removeBin();
+      }
+    }

As discussed in meatspace, I'd rather eliminate the hook and let modules be responsible for removing their own bins on uninstall. There's only a handful of modules that would have their own cache bins, and most of them either are already in core or should be using Http caching anyway. It's not a hard thing to ask of a module that's defining a cache bin.

Although, I don't know how we handle cache-clear-all without it. I don't know if #1764474: Make Cache interface and backends use the DIC would help there or not.

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
@@ -298,4 +299,108 @@ public function isEmpty() {
+    catch (Exception $e) {
+    }

Silently catching an exception is almost never appropriate. If there's a valid reason for it, it should be documented inline because otherwise this is a bug.

sun: How does this require dropping and readding tables?

For foreaching(): If we tag cache backends then we can iterate them in the compiler, but not afterward since the tags are lost in the compiled container. However, we should be able to also store a list of service IDs in the DIC as well, and then grab that and iterate the service IDs that way.

For the empty catch: Gotcha. Yeah, we should at least document it for now; if we can refactor it later, bonus.

Table deletion: Hrm. I don't know that a cache schema would ever change, but with this approach dropping tables on update.php wouldn't be all that catastrophic. There's a possible race condition there, but only if you're running update.php in non-maintenance mode. Which... update.php forces you into maintenance mode temporarily, doesn't it? If so, cycling the bins would be a bit expensive but manageable and wouldn't break anything. So, sure, let's do. We'd probably want to createBin() while still in maintenance mode to avoid a race condition. (So foreach($bins as $bin) { $bin->dropBin(); $bin->createBin(); }

I'm not opposed to manual removal in hook_uninstall()

I think I am totally opposed to that, because I cannot make sense of it :) Let's presume I was the maintainer of admin_menu module which has its own cache table. So the scenario I'm facing is:

1) I need a cache bin.
2) I am not the site operator.
3) Only the site operator decides which cache backend is used for which cache bin.
4) Alrighty, I register my bin.
5) Since I'm not in charge and shouldn't be, registering it is reasonably all I can do. I also cannot maintain it, because I don't know what backend you're actually using. Nor can I uninstall it, because, again, I don't know what backend you're actually using. And maintaining a database schema of a table that isn't used to begin with is pretty much pointless. ;)

Perhaps I'm missing something. :)

What you're missing is that as the module author you know that the bin is called foo. Thus there is a service ID of cache.foo. And you know that you can call $container->get('cache.foo')->deleteBin();

And you do that in hook_uninstall(). That method will either delete a DB table if it's a DB backend, or do nothing if it's memcache, or do whatever it is you do on Redis, etc. You don't care. You are just saying "this cache bin is no longer necessary. Do what must be done."

The current implementation of the cache in DIC issue does the same as the key value, it does not register every bin, it registers the factory, which calls the backend specific factory which returns the actual backend implementation. So there is nothing to iterate on.

Also, note that the backend might have been switched between install and uninstall. install might have created the database table, and when you call dropBin() on it, it might use the memcache bin. So we would have to inform every possible backend that we know of (we don't do that yet) that the bin no longer exists, so that it can remove it if it was at any point in there.

Also, I hope that the need for custom bins will go away for most cases after #1194136: Re-organise core cache bins because then core will provide a handful of bins for most use cases instead of simply adding one to every module that needs the cache.

I like this issue and I will make it happen.

See also #1194136: Re-organise core cache bins, which will probably overlap quite a bit with this.

Definitely not my prettiest code ever but it'll do. We might want to consider moving module_enable/disable/uninstall into a separate file (module.install.inc?) which is not loaded always.

+++ b/core/includes/module.inc
@@ -578,6 +579,38 @@ function module_uninstall($module_list = array(), $uninstall_dependents = TRUE)
+                catch (Exception $e) {
+                  // This is a best effort removal. At least we tried.

We should at least log something in this case so an admin knows to go clean up manually.

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
@@ -188,11 +214,16 @@ public function deleteTags(array $tags) {
     $tag_cache = &drupal_static('Drupal\Core\Cache\CacheBackendInterface::tagCache');

I know these were not introduced by this patch, but allow me to cry for a moment that we're still calling drupal_static() inside a class. I mean, really. These belong in an object property.

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
@@ -243,19 +289,31 @@ public function invalidateTags(array $tags) {
+    catch (\Exception $e) {
+      // If the table does not exist, it surely does not have garbage in it.
+      // If the table exists, the next garbage collection will clean up.
+      // There is nothing to do.
+    }

While logical, if GC fails, wouldn't the next GC fail, too? If GC always fails, shouldn't we know about it?

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
@@ -329,9 +387,147 @@ public function isEmpty() {
+  protected function catchException(\Exception $e, $table_name = NULL) {
+    if ($this->connection->schema()->tableExists($table_name ?: $this->bin)) {
+      throw $e;
+    }
+  }

What happens if the tableExists() call dies? That gets us an Exception-ception situation, which is not a fun place to be for debugging.

If I read this right, set() will lazy-create a table if it's missing (yay!) but setMultiple will not (boo!). Both should be doing so, presumably.

Let me quickly just address setMultiple -- that's a key-value store method and cache has no such thing.

Ah! I was reading too fast misread getMultiple(), which just swallows the exception.

In that case, we should be doing something other than swallowing the exception. At least logging, if not creating the table there. (I can see the argument for not creating it on get, only set, but silent exceptions are not a good idea.)

> silent exceptions are not a good idea.

Why. Is this another of those pretty theories that we use to murder the performance of our subsystems? If so, I am not interested. Here the exception is irrelevant -- the question is whether we have anything in the cache, and we don't. Whether because the cache is inaccessible or because it's empty is irrelevant. The code makes sure that when a relevant exception is raised it gets propagated. I am writing practical code. You can keep your theories if they are part of what made Drupal 8 3-5 times slower than Drupal 7.

As for logging, I have no idea where to log. Someone said that we shouldn't add function calls to our classes so I can't add watchdog() can I?

Hopefully, this fixes the test failures. I simply forgot to update the schema definitions since #99.

I am surprised/amused that the discussion centers around eating exceptions instead of the "arrow shaped" (almost every line opens a new block) code in module_uninstall. Can we do something better? As for logging in there, yes we could watchdog there, that's true. However, what do we want to log? Here is one version.

#111: 1167144_111.patch queued for re-testing.

Moving back to Crell for another (hopefully, final) review.

Two minor nitpicks, after which I think this is RTBC.

+++ b/core/lib/Drupal/Core/Cache/BackendChain.php
@@ -222,4 +222,13 @@ public function isEmpty() {
+  /**
+   * Implements Drupal\Core\Cache\CacheBackendInterface::removeBin().
+   */
+  public function removeBin() {

These "Implements" lines should now be just "{@inheritdoc}". I think we've switched over fully at this point, at least for new patches...

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
@@ -329,9 +387,159 @@ public function isEmpty() {
+    // If another process has already created the cache table, attempting to
+    // recreate it will throw an exception. In this case just catch the
+    // exception and do nothing.
+    catch (\Exception $e) {
+    }

Fair, but I think we should swallow only SchemaObjectExistsException here, which is what's thrown for creating a table that already exists (aka, the race condition in question here). That makes sense to just drop. Any other exception (eg, database going away, syntax error, etc.) should either get handled or allowed to propagate.

Pointless to {@inheritdoc} when everything else is Implements... -- we need to eventually script that anyways.

Changed that exception.

Alas, no interdiff because the old patch no longer applies.

Sigh.

Do or do not, there is no try.

Anyways #117 was said to be RTBC before we got hung up on doxygen. If that's a problem, commit #120 instead.

Here's #117 with {@inheritdoc}

#122 specifically is RTBC.

I really do hate the try/catch statement in every cache backend method, but that's probably just a matter of taste. Anyway it just makes it terrible to read, but at least it should have no performance penalty. One huge problem thought, it doesn't check the error type, so what happens if the database is down? The catchException() will surely rethrow a new exception once again, but its stack trace will tainted and the original exception will be lost.

Re #124, nope, you'd get an exception thrown within the catch and that'd be horrible to debug. I added a comment to that end.

I wonder how it's possible I didn't catch all those bins?

Opsie, that included another patch :) Disregard.

D'oh.

Here.

Ouch. FilterAdminTest is going for the jugular: it runs a query against the cache_filter table. Good that we have APIs. Also, interdiff is behaving so badly, I might need to go for a git workflow :(

New set() code suggested by beejeebus, commented by me.

Looks like catch has this one.

#136: 1167144_136.patch queued for re-testing.

Looks like it is ready to land.

Shameless plug, kinda-sorta related: #1957204: Stop exposing dynamic field data tables in hook_schema() ?

https://drupal.org/node/2012500