Prefix wildcard clears have been supported in Drupal for a very long time, but they're closely tied to using a relational database for caching (i.e. DELETE FROM foo WHERE cid LIKE 'prefix%').
While backends like MongoDB or Redis can handle this, it's very difficult to support in key value stores (as far as I know the only implementation that exists is in http://drupal.org/project/memcache - and getting this right took several iterations over the course of around a year from various different people working on it, and is currently very complex).
If we add cache tags: #636454: Cache tag support, we should be able to use that instead of prefix clears, so this issue would be a case of going through all places that use this feature, converting them to use tags instead, then removing the feature when that's done.
This would mean that cache backends only have to implement one way of clearing cache items that's not tied to their actual cid instead of two, making them much simpler to maintain for what is essentially the same feature set.
Comment | File | Size | Author |
---|---|---|---|
#23 | cache-delete-prefix-23.patch | 26.53 KB | fubhy |
#21 | cache-delete-prefix-20.patch | 24.06 KB | fubhy |
#13 | cache-delete-prefix-5.patch | 24.04 KB | c960657 |
#10 | cache-delete-prefix-4.patch | 24.09 KB | c960657 |
#8 | cache-delete-prefix-3.patch | 26.06 KB | c960657 |
Comments
Comment #1
c960657 CreditAttribution: c960657 commentedComment #3
c960657 CreditAttribution: c960657 commented#1: cache-delete-prefix-1.patch queued for re-testing.
Comment #5
c960657 CreditAttribution: c960657 commentedI could only reproduce one of the test failures (the one in Drupal\system\Tests\Theme\ThemeTest) - and for some reason only when running tests using run_tests.php, not when using the UI. This has been fixed now. Let's try the other one (in Drupal\locale\Tests\LocaleTranslationTest) one more time.
Comment #7
Lars Toomre CreditAttribution: Lars Toomre commentedSome thoughts from a review of the #5 patch if and when it is re-rolled.
Missing '@var array'
Ibid.
Can we add type hinting to this docblock since you are adding another variable. Also this variable need '(optional) to start description as well as what happens in the default case.
This needs a docblock describing what the @param variables being passed to the constructor are.
Comment #8
c960657 CreditAttribution: c960657 commentedThe test failure in LocaleTranslationTest seems to be caused by #1791920: Create new translations:// directory for tests, though I don't understand why that problem only causes a failure with the patch for this issue and not on a clean git checkout (anyone?). But in order to make the test bot happy, this patch includes the patch for #1791920.
@Lars Toomre: This patch addresses all your comments.
Comment #9
tstoecklerSeems this is slipped in accidentally...
Otherwise look really great!
Comment #10
c960657 CreditAttribution: c960657 commentedThose lines were a fix for #1791920: Create new translations:// directory for tests. These were included intentionally in order to the patch pass on the test bot (see comment #8). The fix for #1791920 was committed a few minutes ago, so here is a clean patch.
As mentioned over in #1791920, the test failure only occurred when running the two tests "Translation export" and "String translate, search and validate" in one batch, not when they were run independently. One possible reason for this (only an unverified theory) is that the static class variable DatabaseBackend::$tagCache is not cleared between each test run. A fix for this problem is included in the patch for another cache API improvement, #1774332: Better handling of invalid/expired cache entries.
Anyway, this patch should pass the test bot now.
Comment #12
tstoeckler@c960657: Sorry I hadn't read that, stupid me. Code-wise this is RTBC, but needs to be re-rolled obviously...
Comment #13
c960657 CreditAttribution: c960657 commentedThanks for the reviews and the RTBC. Here is a reroll.
Comment #14
c960657 CreditAttribution: c960657 commentedComment #15
catchLooks RTBC to me too, marking as such.
Comment #16
c960657 CreditAttribution: c960657 commentedIt it allowed to update existing change records, or should we create a new one for this? It seems that this change is relevant to add in New, cleaner cache API.
Comment #17
catchYes updating existing change records is allowed and encouraged :)
Comment #18
fubhy CreditAttribution: fubhy commentedQuickly updated the old changelog over at [#1272696]. Not sure if I nailed it. (Was my first ever change notice. /me hides) :P
Comment #19
catchSorry, this no longer applies. Quick re-roll?
Comment #20
fubhy CreditAttribution: fubhy commentedOn it.
Comment #21
fubhy CreditAttribution: fubhy commentedComment #22
fubhy CreditAttribution: fubhy commentedThis doesn't cover field_info yet (/core/modules/field/lib/Drupal/field/FieldInfo.php)
Comment #23
fubhy CreditAttribution: fubhy commentedOkay that should cover field_info now too. Dunno if that was missed in the first place or added in the mean time. Probably the latter.
Comment #24
c960657 CreditAttribution: c960657 commentedThe latest additions look fine.
Comment #25
fubhy CreditAttribution: fubhy commentedCool. Let's get this commited asap. We are currently pushing forward quite fast in the whole Cache API world and could use this to unblock other issues.
Comment #26
catchAlright. Re-reviewed the patch and this still looks fine. Committed/pushed to 8.x.
This will need a change notice.
Comment #27
webchickLooks like this was done http://drupal.org/node/1272696/revisions/view/1832888/2378858 Thanks, fubhy!
Comment #28
webchickComment #29
sunCouple of questions:
Shouldn't we have added a tag for
'language' => $langcode
here and elsewhere?Why didn't we tag these with 'theme' => $theme->name?
+ more questions identical to these two, but for other cache item key name ($cid) parts in this patch.
Comment #30
fubhy CreditAttribution: fubhy commented@29: Are we clearing anywhere using those, more granular tags? If not, then that is correct. We can add that of course if we ever need to clear on that more granular scale.
Comment #31
catchDon't think so. Entity info is cached per language, but it doesn't need to be invalidated per-language. Tags and cid parts are completely different and don't need to match at all (cids are for uniqueness, tags are for freshness).
Comment #32
sunHm.
1) Is there a significant overhead per (additional) tag in terms of performance?
2) I'd think and assume that the better "categorized" cache items of core are, the more opportunities might arise for us in core (and/or contrib) to improve the cache invalidation? In other words, if I was able to say "Everything that relates to language X is (guaranteed to be) tagged with
'language' => 'X'
, then I can perform a very granular but at the same time very effective tag invalidation for language X only across all cache bins. (Real world example for that would be to add or edit a string translation for language X.) [Whereas language is just one of many possible examples.]3) Separate issue: Thinking further, I almost wonder whether the effective $cid couldn't be auto-generated, in order to significantly simplify the function parameters for get() and set(); e.g.:
Comment #33
c960657 CreditAttribution: c960657 commentedThat depends on the specific cache backend. For the database backend I think the overhead is small (the invalidate value for each tag is fetched only once per request).
Instead of “relates to” I suggest “depends on”. If a cache item depends on some specific piece of data, we should invalidate the cache item when the underlying data changes.
A theoretical example: The cache keys could be table names. If we calculate a value by doing a "SELECT * FROM a JOIN b ON ..." we tag the cache item with "a" and "b". Whenever we write to a table (INSERT/UPDATE/DELETE), we delete all cache items tagged with the table name (this could even be done by the database layer).
This would additionally allow us to make an API for declaring cache dependencies, i.e. that invalidating one tag recursively invalidates other tags. Basically the page cache for node/123 depends on node 123. This node may have a declared dependency on its author, user 456, so calling ->invalidateTags(array('user' => 456)) would trigger a call to ->invalidateTags(array('node' => 123)) tag and thus the page cache item for that page will be invalidated.
Your suggestion requires that you need to know all tags in order to call ->get(). This is not always the case, e.g. in the example provided in the documentation for ->set().
Comment #35
Lars Toomre CreditAttribution: Lars Toomre commentedThis is important issue that I, as a modest observer, would prefer not lag from a lack of core developer attention. How can I help move this issue forward?
Comment #36
fubhy CreditAttribution: fubhy commented@Lars: Which issue are you speaking off? The patch in this issue was commited already. :)