Optimize bootstrap by making a new "max_validate" handler only called during bootstrap_max

Hello!

Drush core-status appears to have been designed to function in a variety of different scenarios; for instance:

1. If you run it from a directory that isn't inside a Drupal installation, it'll give you the drush version and the location of php.ini.
2. If you run it from a directory that just has Drupal source code, but no settings.php file (or a settings.php without a db_url), it'll give you the version number for the Drupal source code, in addition to the drush version and php.ini information.
3. If you run it from a directory that has Drupal source code and a settings.php file that points at a valid, but as-yet-uninstalled database, you get ugly error messages.
4. If you run it from a directory that has a working, fully-installed Drupal instance, it'll give you the Drupal version, the database connection parameters, and everything else it listed in scenarios 1 and 2.

From these examples, it looks like the basic idea behind drush core-status is to provide the user with as much Drupal-related status information about the current working directory as it can find. The only scenario under which it fails to do so is scenario 3, which is what I ran into this morning. Personally, I think if you're going to support scenarios 2 and 4, it only makes sense to support 3 as well.

One other way to look at it: the scenario I've described is the point at which a Drupal site install meets the minimum requirements for successfully running install.php. No site is going to stay in that condition for long, but you could say that about scenario 2 as well, and that's supported just fine.

Just my two cents; I really could use this feature, so I figured I'd do my best to argue for it :)

Thanks!
Adam

Hello!

OK, I'm happy to help however I can. Trick is, I'm not sure I completely trust my understanding of how the bootstrap process works. If someone can help me put the pieces together I'd be happy to submit a patch. Here's a brain dump that covers what I think I understand, and ends with a possible solution I'd be happy to implement if it sounds about right:

When drush is invoked, it compiles a list of available commands from all available hook_drush_command() implementations. When it finds the command definition that matches the command with which it was invoked (say, "core-status"), it checks to see if that command definition requires a certain bootstrap level (specified by the 'bootstrap' parameter). If it finds one, it runs drush_bootstrap_to_phase($command['bootstrap']), ensuring that the Drupal instance is bootstrapped high enough for the command to function. Once that's done, it dispatches the command.

In the case of "core-status", the bootstrap level required is currently DRUSH_BOOTSTRAP_MAX. This is a special-case bootstrap level, which drush_bootstrap_to_phase() handles by passing control over to drush_bootstrap_max(). The latter is designed to "bootstrap to the highest level possible, without triggering any errors". For each possible bootstrap phase, it does two things: (1) validate that this bootstrap phase can be reached by running drush_bootstrap_validate(), and (2) actually bootstrap to that phase by running drush_bootstrap().

So …given the conditions necessary to produce this bug (settings.php is present and contains a valid $db_url, but the database is empty), here's what happens. Our error is occurring during drush_bootstrap_max(), specifically during the call to drush_bootstrap(DRUSH_BOOTSTRAP_DRUPAL_FULL). Both validation and bootstrap succeed completely for phases 1-4 (_ROOT, _SITE, _CONFIGURATION, and _DATABASE) …it's just that last phase that fails. The problem is that it actually passes validation …that is to say, even though there isn't a database, drush_bootstrap_validate() thinks it's still safe to bootstrap to phase _FULL.

So then, it seems like the best fix for this problem would be to add some validation logic for the _FULL phase. It does not appear that there is currently any validation logic for that phase (there is no _drush_bootstrap_drupal_full_validate() function). I'd be happy to write such a validation function, but the question is, what should it contain? In order to fix this particular bug, the only test we'd absolutely HAVE to implement is "does the 'access' table exist in the database?". However, that doesn't really seem like it guarantees a positive answer to the real question, "is the site installed?"

What do you think?

Thanks!
Adam

Hello!

The attached patch is a decent starting point. Couple of notes:

* I don't fully understand why, but it seems that _drush_sql_query() utilizes drush_op_system() to execute the query, which results in the query output being thrown away. The patch changes this to drush_shell_exec(), allowing the output to be retrieved later by drush_shell_exec_output().
* I have yet to benchmark the performance difference; I figured I'd get the patch out there first and then come back with some timings later as I have opportunity.

Please review; this does seem to meet my use case, although since it touches such a core aspect of drush's functionality I definitely understand if it needs additional review.

Thanks!
Adam

Hello!

OK, and now for some quick benchmark results. My methodology here is simple: inside drupal_bootstrap_max(), I inserted code that calls microtime() once before the call to drupal_bootstrap_validate() and once after, both only if the $phase_index is DRUSH_BOOTSTRAP_DRUPAL_FULL. The difference is then output directly to the console.

Without my modifications, the results of five consecutive runs were as follows:

* 0.00001499 µs (originally output as 1.4999999999987E-5 µs …correct me if I've converted this wrong)
* 0.00001400 µs
* 0.00001799 µs
* 0.00001499 µs
* 0.00001999 µs
* Average: 0.00001639 µs

After applying my patch, the results of five consecutive runs were as follows:

* 0.014139 µs
* 0.014775 µs
* 0.01417 µs
* 0.013974 µs
* 0.016126 µs
* Average: 0.0146368 µs

The difference is significant if you think of it in terms of percentage increase (almost 900%!), but we're talking a fraction of a microsecond here, so the actual increase is pretty negligible (.00000000001639 seconds total increase).

Personally I think the ability to use drush core-status in this situation is worth the extra fraction of a microsecond it takes to count the tables in the db. However, there's still the question of unintended consequences. Note that my patch does include a change to how drush_sql_query executes its commands; if someone more familiar with the codebase can tell me if that change is a good idea or not, that's really the only remaining question I can see needing answering here.

Thanks!
Adam

Ah, you're right about the unit. I always get that confused when using microtime(); the function name kind of implies that it's returning microseconds …but it's actually just returning seconds with microsecond precision. Sorry about that.

Meanwhile, I'll see what I can do about the sql_query call.

Thanks!
Adam

Hello!

OK, here's a new patch implementing your suggestions from #10. Please let me know what you think!

Thanks,
Adam

Ah, I didn't see that. This new patch should resolve that; it tests the return value of drush_shell_exec for TRUE (success), instead of 0 (which would be the success value for drush_op_system).

Thanks!
Adam

Hello!

Something else just occurred to me: the actual installation process has its own means of determining the installation status. Apparently, it uses a value in the variable table called "install_task" …if it's "done", it doesn't continue with the installation.

Two things, then:

1. Checking for "install_task" = "done" would probably be just as quick as, if not quicker than, the test I implemented in #14. It also wouldn't require use of the drush_sql_query wrapper, since the syntax of the necessary query would be standard SQL.
2. Personally, I think it would also be very useful to output the value of "install_task" in the table produced by drush core-status. Otherwise, core-status's output is identical for both installed and non-installed sites.

I've attached a new patch that implements #1. #2 seems a bit more complicated; doesn't make much sense just to run a query directly in _core_site_status_table(), but I'm not entirely sure I understand the context system used to get all the values currently listed there. Presumably we could add another context value, say, DRUSH_DRUPAL_SITE_INSTALL_TASK, or some such? I'm just not sure how or where to do that, or if it's actually a good idea.

Thanks!
Adam

Hello!

I tried out your patch from #22, and it seems to work perfectly for me.

Thanks!
Adam

Hello!

Whoops, I think I spoke too soon about that install_task thing …it would seem that Drupal 5 doesn't track installation progress that way. Would it be worth re-implementing the original test for when core-status runs against a D5 site?

I do think that when install_task is available, that's a better, more accurate test of the state of the site …but if it's not available in all versions, it may be necessary to have a fallback method (like the 'show tables;' approach from #14). I'd be happy to write a new patch at some point today.

Thanks!
Adam

Hello!

The attached patch is identical to #22 except that it now returns TRUE for Drupal 5 sites, as moshe suggested in #26.

@moshe, I've done some brainstorming on your other question: what bootstrap phase would we need to get to in order for variable_get() to work?

The problem is, by the time we run _full_validate(), that question becomes moot. We know we're already bootstrapped to DRUPAL_DATABASE because otherwise we wouldn't be trying to validate the next phase higher. We also know we can't attempt to bootstrap to DRUPAL_FULL yet, because the point of _full_validate() is to determine if that's even possible. Based on this logic, it kind of seems like we're stuck at DRUPAL_DATABASE …and if DRUPAL_DATABASE isn't enough to run variable_get(), then we're going to have to rely on a direct query; is there another option?

The only problem I can see with using a direct query is if we're operating on a site that somehow doesn't store its variables in the database …i.e., they've got some module installed that overrides the default backend for performance reasons (does memcached do this?). If that's the case, I'm not sure what options there are …but if variable_get() doesn't work, we're kind of stuck, aren't we?

Just some thoughts …again, my familiarity with this code base only goes back to the beginning of this ticket, so if I'm missing something, just let me know.

Thanks!
Adam

Hello!

@catch: the patch I submitted in #27 wouldn't have the problem you described, since it no longer relies on SHOW TABLES; instead, it queries the variables table for a particular value that's always present on installed sites from Drupal 6 onward. As far as the table cache is concerned, it would have the same impact as the alternative query you suggested. It would also avoid the problem @msonnabaum mentioned in #34, as it's no longer using the drush sql query wrapper that outputs query results to stdout. I believe it also circumvents the potential performance problems @moshe brought up in #17, due to the max_validate modifications introduced by @greg in #22.

I agree that the committed patch wasn't the right approach …but #27 takes quite a different tack, and IMO should be considered on its own merits before we close this down completely.

Thanks!
Adam