Problem/Motivation
There are many cases where running the database updates can be problematic with warm caches. #3052971: Notice: Undefined index: #include_fallback in Drupal\Core\Render\Element\StatusMessages::generatePlaceholder() is just a very recent example.
Proposed resolution
Decorate the cache_factory with a special update cache factory that uses a NullBackend based backend. The special update backend passes on deletes to the real backend so cache flushes occur as expected so once updates are not being done the caches are empty as expected.
Maybe a better fix would be to somehow swap back to the unmodified container - ie. one without UpdateServiceProvider prior to calling drupal_flush_all_caches(). The problemswith this are:
- How to determine this in UpdateKernel - use a query parameter?
- drupal_flush_all_caches() does a container rebuild so we’re going to end up with multiples of these.
Remaining tasks
Discuss/review.
User interface changes
Nope.
API changes
TBD.
Data model changes
Nope.
Release notes snippet
Caches are no longer used during Drupal's update process. They are only cleared.
Comment | File | Size | Author |
---|---|---|---|
#44 | 3055443-2-44.patch | 7.9 KB | alexpott |
#44 | 37-44-interdiff.txt | 1.15 KB | alexpott |
#37 | 3055443-2-37.patch | 7.88 KB | alexpott |
#37 | 36-37-interdiff.txt | 5.75 KB | alexpott |
#36 | 3055443-2-36.patch | 13.63 KB | alexpott |
Comments
Comment #2
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThis should do it.
Comment #3
catchSo if we do this I think we should only clear out the cache bins (this logic copied from drupal_flush_all_caches() rather than doing a full rebuild to minimise the code path.
Comment #5
alexpottAnother thought is that we should not be write or reading anything from caches until update.php is done with.
Comment #6
alexpottAlso putting it in the batch is too late - #3052971: Notice: Undefined index: #include_fallback in Drupal\Core\Render\Element\StatusMessages::generatePlaceholder() occurs when the user visits update.php
Comment #7
alexpottI think whatever we do should potentially be in \Drupal\Core\Update\UpdateServiceProvider
Comment #8
BerdirThere's an issue that aims to use memory cache backends for the installer, could we do the same for update? However, just like in the installer is that at the end, we do want to clear the real caches.
Comment #9
alexpott@Berdir +1 to the memory cache idea. At least means we don't spend unnecessary resources working things out twice.
Comment #10
alexpottComment #11
alexpottLet's see what breaks - gonna add some proper testing to ensure the cache is cleared by an update.
Comment #13
alexpottComment #14
alexpottComment #15
alexpottComment #16
alexpottThe change to
core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBase.php
is no longer necessary now we're passing deletes on to the real cache backend.Comment #17
alexpott@catch asked for some comments about why we have the changes to the cache classes.
Comment #18
oknate1) s/serialised/serialized
Content style guide specifies American spelling, and it's used with American spelling on core.
2) same for s/minimise/minimize
3) same for s/whilst/while
Comment #19
alexpottIndeed - thanks @oknate!
Comment #20
alexpottResolved the @todo and added a change record.
Comment #21
alexpottRerolled now that #2863986: Allow updating modules with new service dependencies has landed.
Comment #22
catchJust nits. Looks really good to me now otherwise.
using
This should probably be either actual English or the fully qualified class name.
Comment #23
alexpott@catch thanks for the review. I re-read all the things and found some missing properties and copy pasta. I decided to drop the "real" from the names of things because it's not helpful and replaced with "regular runtime" which I think is more indicative of what I meant by "real".
Comment #24
chr.fritschThis looks good to me. We are already using this patch for our Thunder 2 -> 3 update path and it works very well.
Comment #25
catchCommitted c22202e and pushed to 8.8.x. Thanks!
Comment #27
alexpottI think we should consider backporting to this to 8.7.x.
Also whilst this change is big
Comment #28
alexpottUnfortunately we need to revert this as it is causing random fails for SQLite. Not sure why yet. See https://www.drupal.org/pift-ci-job/1353542 for an example fail.
Comment #30
alexpottSo this patch fixes the random fail. I've not worked out why it's happening yet.
Comment #31
alexpottOkay worked this out :( it's because system_update_8013() does not account for the container rebuild :( :( :(
This happens for all db types it just happens that SQLite does updates fast enough sometimes for system_update_8013() and system_update_8200() to run in the same batch.
Whilst the patch attached fixes the problem I'm not 100% sure it is the correct solution. The reason nothing goes wrong in HEAD is that none of the protected properties on the stale config factory are re-used during the save after the container rebuild. I think the correct solution might be do use a static property in
Comment #32
alexpottSo here's the fix I think we should do. We need to make the UpdateBackend caches survive a container rebuild so they behave more like a consistent cache like DatabaseBackend. Therefore we need to use statics.
All the interdiffs are back to #23 - I include a simple fail patch that fails with a very small addition to #23 to show the problem. This fails regardless of DB backend.
Comment #33
alexpottI wonder if I should put the static stuff into it's own backend because I suspect it would safer to use in #2488350: Switch to a memory cache backend during installation too. Install has plenty of container rebuilds going on.
Comment #35
catchThis seems like a problem with the contact test to me - i.e. the test runner is using a stale version of the container.
Same problem as in #2066993: Use \Drupal consistently in tests. See diff for a suggestion.
Comment #36
alexpottDiscuss a bit more with @catch. We ruled out #32 because statics have their own level of problems and while it does it the UpdateBackend more like the DatabaseBackend @catch favoured another solution. We can extend the NullBackend instead and not actually cache anything!
I include a test patch which adds test code to
system_udpate_8013()
to prove we have it fixed.The "real" patch here fixes
system_udpate_8013()
so that core sets a good example for contrib and custom to follow.Comment #37
alexpottNow we're extending from NullBackend there's no need to fix the MemoryBackend or MemoryCache classes.
Comment #38
alexpottThe improvements to
MemoryBackend
andMemoryCache
can be a part of #2488350: Switch to a memory cache backend during installation - which is where they came from originally.Comment #39
chr.fritschWe need a title and an IS update here since we are no longer switching to the memory backend.
Comment #40
alexpott@chr.fritsch good point.
Comment #41
chr.fritschLooks good. We are already using this patch in Thunder for a while.
Comment #42
catch*during Drupal database updates
(fixable on commit).
Comment #43
Wim LeersWoah! Super interesting 🤩
🤓 Nit: "to use when updating Drupal" would be more accurate I think?
MemoryBackend
, but this is now using a subclassedNullBackend
.Comment #44
alexpottHere's fixes for #43.2 and #42 - going to update CR now..
Comment #46
catchCommitted 80a307d and pushed to 8.8.x. Thanks!
Comment #48
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThis patch also fixed a nasty regression from 8.7.x: #3075675: Route cache can become poisoned with invalid data during database updates, resulting in 404s
I'm not sure if we need to backport the patch to that branch, but I thought it would be good to mention it at least.
Comment #49
liquidcms CreditAttribution: liquidcms commentedI came across this: #2981353: The "media" entity type does not exist which led to this core issue. This seems to state that this fix was committed to 8.8.x 4 months ago yet I am running 8.8.1 which is only 1 month old.. and i am still getting this error when i try to do a db update.
Comment #50
liquidcms CreditAttribution: liquidcms commentedi found another post suggesting to enable the (core) Media module - and this solved the issue.
Comment #51
quietone CreditAttribution: quietone at PreviousNext commentedPublished the CR