Avoid variable_set() and variable_del() stampedes

What about performance?
I see that lock_acquire() using some db queries (db_insert()).
What is the reason of caching something in memory, if it use database anyway.

Pressflow already added a lock in variable_initialize(). I think it makes sense.

I've ported the pressflow version from http://bazaar.launchpad.net/~pressflow/pressflow/6/annotate/head:/includ... to a core patch.

Main changes vs. HEAD are:

1. When you call variable_set() or variable_del(), it sets a global, which is then checked in drupal_page_footer() to trigger a cache clear - that way if you submit a long system settings form you don't clear the cache for each one at a time.

2. Adds a lock to variable_initialize() on cache misses, using exactly the same pattern as #802446: Cache stampede protection: drupal_render() and page cache.

I didn't do any testing of this beyond checking HEAD would still bootstrap, but I know that nnewton will need to in the next couple of days in an environment where this issue is getting hit.

Also according to twitter, this is the last core patch from Drupal 6 pressflow that hasn't made it into D7, would be great if Drupal 7 pressflow can start from a clean slate.

It might be possible to add a write through cache to this, but I think if we want to add such a thing, we should do it for the page cache first in #762978: Prevent cache stamped for page cache table.

Ok for the lock in variable_initialize(), but the delayed clearing is a really bad idea from a consistency point of view.

These are really two separate patches anyway. Removing the delayed clearing for now. Let's see if that helps the tests at all too.

And whitespace.

The lock_initialize() causes the single test failure (in lock tests, presumably that's setting global $locks to empty and allowing a lock to be acquired, see if that causes problems elsewhere if we just remove it.

lock_initialize() should just take care not to overwrite the global if it's called twice.

Aren't you just missing the lock_release() call?

I am, but that's not why the test is failing. If you add the lock_release(), but also remove the check from lock_initialize() that test still fails.

Please move lock_initialize() earlier in the bootstrap (for example in _drupal_bootstrap_variables()). Having variable_initialize() call lock_initialize() feels slightly messy.

That's a better fix, it's only slightly earlier in the bootstrap process so no real bloating, and that means no more need for the isset() check in lock_initialize().

#17 looks good to me.

I've been load testing #17 and found an issue. In variable_init, when you hit the lock and re-call the function we arn't passing the $conf array in, so all settings in settings.php are lost. The upshot of this for a high-traffic site is that once you hit the lock, you lose memcache.

    if (!lock_acquire($name, 1)) {
      // Another request is building the variable cache.
      // Wait, then re-run this function.
      lock_wait($name);
      return variable_initialize();
    }

I'm continuing testing as under high traffic we are also getting a stampede of the semaphore table, which is problematic and may require a memcache lock.inc over-ride.

Oh, d'oh for the first issue. For high traffic sites, of course we will need a memcache (or similar) lock.inc. Something that doesn't require polling would be nice. The default db-based implementation is not designed to scale.

Here is the version of the patch I'm currently using. Ya, a memcache plugin for the locking framework definitely, but it actually isn't that bad once you fix this.

Well done. This looks RTBC to me.

As for the memcache version lock.inc - might not be needed once the lock.inc improvements from #802446: Cache stampede protection: drupal_render() and page cache are committed

Ouch on $conf :(

memcache-lock.inc has a patch here #596908: memcached implementation of D7 locking, I re-rolled it yesterday, but didn't test it and that issue is quite old, but looks pretty far along to me.

Committed to CVS HEAD. Thanks.

Updating status.

Subscribing.

Applied the patch from #27.
Now I get the same error but for a different table:

Duplicate entry 'variable_init' for key 'PRIMARY' query: INSERT INTO semaphore (name, value, expire)

(Using D6 with Boost)

Mikey,

I did the samething like #31 and I am getting that error as well. Do we have to delete that and see if it works?

Any help is appreciated.

David

Mikey,

What I did is just install number #27. Do I have to do anything prior to Patch #27 then? Should I do the number #17 or #22 as well?

The reason I am interested is because I would get about 10 to 20 errors in the watchdog page like this:

 boost 11/02/2011 - 15:26 There are php errors on this page, ... Anonymous  
 boost 11/02/2011 - 15:25 There are php errors on this page, ... Anonymous  
 boost 11/02/2011 - 15:19 There are php errors on this page, ... Anonymous  
 boost 11/02/2011 - 15:17 There are php errors on this page, ... Anonymous  
 boost 11/02/2011 - 15:14 There are php errors on this page, ... Anonymous  
 boost 11/02/2011 - 15:13 There are php errors on this page, ... Anonymous  
 boost 11/02/2011 - 15:12 There are php errors on this page, ... Anonymous  
 boost 11/02/2011 - 15:09 There are php errors on this page, ... Anonymous  
 boost 11/02/2011 - 15:07 There are php errors on this page, ... Anonymous  
 boost 11/02/2011 - 15:06 There are php errors on this page, ... Anonymous  
 boost 11/02/2011 - 15:04 There are php errors on this page, ... Anonymous  
 boost 11/02/2011 - 15:01 There are Drupal messages on this ... Anonymous 

Thanks for your quick response.

David

Thanks mikeytown2

I Applied patch #37...
It solved the original error like expected, but now I get a slightly different error

Duplicate entry 'variable_cache_regenerate' for key 1
query: INSERT INTO semaphore (name, value, expire) VALUES ('variable_cache_regenerate', '63824228350d1e4bc9b4692.81439270', 1355932890.636)

Is this related, or a whole different issue?

I see it in the /admin/reports/dblog under "boost" type:

There are php errors on this page, preventing boost from caching. ERROR:

Array

      [type] => 512
      [message] => Duplicate entry 'variable_cache_regenerate' for key 1
query: INSERT INTO semaphore (name, value, expire) VALUES ('variable_cache_regenerate', '151849102550d2f24540f987.74939510', 1356001891.2661)
      [file] => /var/www/mysite/includes/database.mysql.inc
      [line] => 135

Is this what you were looking for?
Drupal 6, Boost, NGINX, InnoDB tables (most of them)

EDIT: When boost is set to cache pages even with PHP errors, the error is ignored in the DB log.
Bug in Boost module?

Hi mikeytown2,

thank you for a great patch!
Using it in production on a large site and it works well.

Attaching a patch for Pressflow 6.28.111 (from https://github.com/pressflow/6/pull/37) in case anyone needs it.

Cheers,
Dan

Moving back to "Needs review" for #37 which is the patch in play here for D6, if I read this right.

#11: variable_stampede.patch queued for re-testing.

#27: drupal-561990-D6.patch queued for re-testing.

Reroll.

Patch 47 failed because it was rerolled against Pressflow.

One thing I do notice - although #37 solves the stampede problem, I regularly see

Duplicate entry 'variable_cache_regenerate' for key 'PRIMARY'

In several different contexts, both in core areas and contrib. Anybody else seen this or started to understand it.

Re #50 I get this, a lot, usually when there is cron. Two traces with this error:

in mysql_query called at /home/ttcn/live/www/includes/database.mysql.inc (143)
in _db_query called at /home/ttcn/live/www/includes/database.mysql-common.inc (42)
in db_query called at /home/ttcn/live/www/includes/lock.inc (117)
in lock_acquire called at /home/ttcn/live/www/includes/bootstrap.inc (569)
in variable_init called at ? (?)

in mysql_query called at /home/ttcn/live/www/includes/database.mysql.inc (143)
in _db_query called at /home/ttcn/live/www/includes/database.mysql-common.inc (42)
in db_query called at /home/ttcn/live/www/includes/lock.inc (117)
in lock_acquire called at /home/ttcn/live/www/includes/bootstrap.inc (569)
in variable_init called at /home/ttcn/live/www/includes/bootstrap.inc (1528)
in _drupal_bootstrap called at /home/ttcn/live/www/includes/bootstrap.inc (1443)
in drupal_bootstrap called at /home/ttcn/live/www/index.php (15)

In my guess, it is because of the concurrency between two SELECTs to check for the lock, then two INSERTs in different requests. But it's a separate issue.