Moving a class between files can cause SQL errors on registry rebuild

Subscribing. No time to look at this right now, but I will try to soon.

While registry rebuild is a great example of where we want transactions, it's not per-file. It's per-rebuild.

The problem here is:

Before:
foo.php: class A {}
bar.php: nothing

After:
foo.php nothing
bar.php: class A {}

If the registry rebuild hits foo.php first, then there is a brief period where class A exists nowhere, which is OK. If it hits bar.php first, then it tries to insert class A in bar.php and chokes on a duplicate key error.

Possible solutions include:

1) Wrap the entire rebuild in a transaction: This wouldn't protect us against the above breaking stuff, but the breakage wouldn't be as bad because we're not left with a half-built registry table. So net win, but only part of the solution.

2) Wrap each file in a transaction. This doesn't help us here unless one file contains the name name twice, which would break PHP anyway. What it would do if we parsed bar.php first is rollback bar.php's changes entirely, then process foo.php (unless we told it to stop) and clear out class A, and then on the next rebuild it would find the new location for A. But between those rebuilds (which is two user-triggered answers) Drupal won't know where A is, and that's bad.

3) Use a merge query instead of a multi-insert. This is actually what we did originally when we were still tracking functions. It's a bit slower, but is not going to fatal out on us. The side effect is that if a class really is defined in more than one location, whichever one parses second will win. That makes "define it twice and include just one of 'em" break, but that's OK since that's a poor pattern anyway, especially for classes. That's not supported in the first place. We will still need to delete all entries in a file before parsing so that we clear out removals.

So we want to do #1 and #3, I'd say.

Now, I should go look at the patch... :-)

I'm pretty sure the test bot runs myisam for performance reasons.

hmm, I double checked and according to http://qa.drupal.org/performance-tuning-tips-for-D7 it is indeed innoDB.

Just wanted to note that I can't reproduce the test fail either on my local system.

#18: registry.transaction.patch queued for re-testing.

The test passes in my environment.

#18: registry.transaction.patch queued for re-testing.

if it gets committed like this... wont all patches start failing the bot testing?

Last patch now passes the test bot.

It looks like all this really does is wrap the existing code in a transaction, which seems reasonable. This won't help those on MyISAM, of course, but at least we un-screw part of our userbase. :P It'd be nice to have test coverage for this, but I can't really envision a way for that to happen.

I'm a bit nervous about whether that test passage was a fluke or whether the previous fails were flukes. I guess if testbot starts crapping out after this, we'll have a good place to start on where to point the finger of BLAAAAAME. ;)

Committed to HEAD!

There is a merge query hunk at the bottom, but I can't parse the earlier patches well enough to tell if it's equivalent.

This patch led to a massive increase in the amount of time it takes to install Drupal (from ~8 seconds to ~22 seconds on my machine). Granted, installations only happen once, so their performance is not the most important thing in the world, but I recall a fair amount of work a few months ago in the D7 dev cycle to shave some seconds off the installer so people didn't have to stare at that progress bar forever, and now we just doubled or even tripled the entire installation time :(

@pwolanin's earlier patch in this issue does not have a noticeable effect on the time it takes to install Drupal.

Also, the flood test failure seems to be happening out in the wild. It happened to a patch of mine earlier today even though that patch touched nothing but JavaScript (#819724: Improve tabledrag accessibility for screen-reader users with WAI-ARIA).

This massive increase of time has two other very serious effects:

1. It uses far more resources in our testing environment. Remember that we do hundreds or thousands of installs in each D7 test suite. I note that we've doubled the time it takes to run the test suite. Not so good.

2. The important work in #686196: Meta issue: Install failures on various hosting environments aims to make Drupal install before timeouts on run-of-the-mill shared hosting. This would be a step back for us.

I'm surprised that just adding a transaction would take so much time.

Didn't profile, but ran the installer via drush as 'time drush -v si --db-url:d7 -y' with HEAD and with this patch reverted:

HEAD:

real	1m19.336s
user	0m47.867s
sys	0m15.121s

Patch reverted:

real	0m44.215s
user	0m26.682s
sys	0m10.165s

About what rfay reports.

That almost certainly means a regression on #686196: Meta issue: Install failures on various hosting environments then.

My gut intuition is that it's not the transaction that slows it down as much as the changes to the DB queries that result, although I've not tested to confirm that. But normally transactions should speed things up if anything since the DB doesn't need to commit to disk as many times.

Before patch real 0m12.535s after patch real 0m4.164s

Extra extra critical, patch looks fine.

Oh, dear lord! :\ Thanks for the sleuthing, folks.

Committed to HEAD!

Yay! PIFR test time on client #32 is down to 15 minutes again from 40 minutes.

Hehe, awesome bugfix :) Does this fix the sporadic flood API test failure also? Well, if not, I guess we'll know about it soon.