attached patch stores agrcache's file list in a separate table.

under heavy load, this reduces the impact of css/js rebuilds causing multiple variable-cache rebuilds, which can have nasty effects on a busy site.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

Status: Needs review » Needs work

The custom table is missing indexes - at least needs an index on file type and could use a primary key as well.

Would it be worth caching the files on get - would save a couple of database hits each page when using the alternative cache storage.

Anonymous’s picture

Issue summary: View changes
FileSize
7.79 KB

new patch with some indexes and caching.

Anonymous’s picture

Status: Needs work » Needs review
Anonymous’s picture

Assigned: » Unassigned
erikwebb’s picture

The cache_clear_all() call was missing an explicit cache bin.

Otherwise this works well for me.

erikwebb’s picture

Status: Needs review » Reviewed & tested by the community
erikwebb’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
7.76 KB

Actually discovered an issue where the file list was updating for the 'footer' scope, then being fully overwritten by the 'header' scope later within the same request. This prevented 'footer' from every being preserved in the database. I've changed the logic so that

erikwebb’s picture

I believe the merge operation here actually creates a possibility for a race condition. Since a cache_get() followed by the array_merge(), any changes to the agrcache array between these operations could result in lost files. This implementation will reduce that, but without locking it's still possible.

erikwebb’s picture

I refactored this slightly so that, rather than relying on an existing array structure in the database, the module can be more resilient if the database table is empty. This also means we can remove the custom INSERTs at install.

erikwebb’s picture

Accidentally added the d.o .info snippet

catch’s picture

Category: Feature request » Bug report
Priority: Normal » Major
FileSize
6.96 KB

Until variable_set() stampedes are fixed/mitigated in core, treating this as an agrcache bug.

The update should not use the schema API but specify the table definition directly, re-rolled for that and a conflict.

catch’s picture

FileSize
6.95 KB

Another update - the watchdog warning happens straight after updating the module, changed to a notice.

catch’s picture

FileSize
6.95 KB

And db_update() instead of db_query('UPDATE...

  • catch committed 19014af on 7.x-1.x
    Issue #2069473 by erikwebb, catch, beejeebus: store agrcache's file data...
catch’s picture

Status: Needs review » Fixed

With that, this is testing OK. Committed/pushed to 7.x-1.x, thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.