During the installation process, we do a lot of clearing and rebuilding of the schema cache. This is not by itself a problem, but it also triggers a lot of cache_set() from SchemaCache::__destruct. And as a consequence, a lot of lock/unlock and a lot of truncate query on the cache tables.

We should not try to save a DrupalCacheArray we already know is invalid.

In addition, we don't have a way to *only* clear the schema, without rebuilding it. As a consequence, saving fields is actually very costly because we keep clearing and rebuilding the schema cache, even if we don't have an immediate use for it.

CommentFileSizeAuthor
#13 1470670-drupalcachearray-destruct-invalid-13.patch5.53 KBamateescu
#8 1470670-drupalcachearray-destruct-invalid-8.patch4.99 KBamateescu
#6 1470670-drupalcachearray-destruct-invalid-6.patch2.56 KBamateescu
#4 1470670-drupalcachearray-destruct-invalid.patch2.57 KBDamien Tournoud
#1 1470670-drupalcachearray-destruct-invalid.patch2.57 KBDamien Tournoud
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Status: Active » Needs review
FileSize
1470670-drupalcachearray-destruct-invalid.patch2.57 KB

Proposed patch:

  • we introduce a DrupalCacheArray::invalidate() method that we call at the proper time
  • we introduce a drupal_get_schema_reset() that can be called to only clear the schema, not immediately rebuild it
Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Issue tags: +Performance
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs review » Needs work

+ cache_clear_all('schema:', 'cache', TRUE);
This is throwing an exception because you want cache()->deletePrefix(), hence the 0 passes. Apart from that looks good I think.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Status: Needs work » Needs review
FileSize
1470670-drupalcachearray-destruct-invalid.patch2.57 KB

Rerolled for current 8.x.

Status: Needs review » Needs work

The last submitted patch, 1470670-drupalcachearray-destruct-invalid.patch, failed testing.

amateescu’s picture

amateescu CreditAttribution: amateescu commented
Status: Needs work » Needs review
Issue tags: +Needs backport to D7
FileSize
1470670-drupalcachearray-destruct-invalid-6.patch2.56 KB

#4 was not a reroll but the same patch as #1. Let's try this one, with #3 fixed.

Status: Needs review » Needs work

The last submitted patch, 1470670-drupalcachearray-destruct-invalid-6.patch, failed testing.

amateescu’s picture

amateescu CreditAttribution: amateescu commented
Status: Needs work » Needs review
FileSize
1470670-drupalcachearray-destruct-invalid-8.patch4.99 KB

Ok, now we're getting somewhere.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

The are some more calls to drupal_get_schema(NULL, TRUE) throughout core, most notably one in drupal_flush_all_caches()...
http://api.drupal.org/api/drupal/core!includes!schema.inc/function/calls...

Were they intentionally omitted?

amateescu’s picture

amateescu CreditAttribution: amateescu commented

Yep, because there we want the schema to be rebuilt, not just cleared.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Reviewed & tested by the community

Makes sense, thanks!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Needs review

Why do we want the schema to be rebuilt in drupal_flush_all_caches()? Seems fine for the next request that needs it to do that cache building to me.

amateescu’s picture

amateescu CreditAttribution: amateescu commented
FileSize
1470670-drupalcachearray-destruct-invalid-13.patch5.53 KB

Why do we want the schema to be rebuilt in drupal_flush_all_caches()?

I haven't investigated in depth, just looked at the function doc: "Flushes all persistent caches, resets all variables, and rebuilds all data structures.". If that's not true after all, let's try :)

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs review » Reviewed & tested by the community

In the case of the schema cache, it's a data structure, but it's also a straight cache item, so yeah this looks good to me.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/includes/common.inc
@@ -7291,7 +7291,7 @@ function drupal_flush_all_caches() {
   // before any modules are loaded, so if there is no cached schema,
   // drupal_get_schema() will try to generate one, but with no loaded modules,
   // it will return nothing.
-  drupal_get_schema(NULL, TRUE);

There are six lines of inline comment in front of this schema rebuild, which try to explain why this explicit rebuild is needed:
http://api.drupal.org/api/drupal/core!includes!common.inc/function/drupal_flush_all_caches/8

If it is not required anymore, then I at least want to know why or how that is so (and wonder why that isn't covered by tests).

And of course, also removing/replacing the lengthy comment.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

OK so what we want to do then is prevent saving the schema when there hasn't been a full bootstrap then. That's similar to #592008: Don't save theme registry before modules are included.

I was going to open a follow-up for this, but then realised if we're replacing explicit rebuilds with cache clears all over the place anyway, then there's a much higher potential for that race condition (i.e. a hook_boot() implementation calls drupal_get_schema()), so better to do it here. The ThemeRegistry class already has code that could be borrowed.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented

// Rebuild the schema and cache a fully-built schema based on new module data.
// This is necessary for any invocation of index.php, because setting cache
// table entries requires schema information and that occurs during bootstrap
// before any modules are loaded, so if there is no cached schema,
// drupal_get_schema() will try to generate one, but with no loaded modules,
// it will return nothing.

^ At one point during Drupal 7 cycle, running a InsertQuery / UpdateQuery or MergeQuery on PostgreSQL required a loaded schema information. Since then, we are querying the database directly for this information, so this has not been a problem for a long time.

Of course, as catch mentioned, we should not save the schema if ever it has been built before full bootstrap is reached.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx commented
Issue summary: View changes
Parent issue: » #534594: [meta] system info cache writing and registry rebuilding is broken without a full bootstrap

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Needs work » Closed (duplicate)

This is no longer relevant after #2451395: drupal_get_schema()/drupal_get_complete_schema() no longer work as expected; remove them.