Race condition in the update process causes random test bot failures

Here is what I found.

update.php 385 : drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL);
bootstrap.inc 2056 drupal_bootstrap_full
common.inc 4918 drupal_path_initialize
path.inc 18 drupal_get_normal_path
path.inc 264 drupal_lookup_path
path.inc 70 drupal_path_alias_whitelist_rebuild
path.inc 381 db_query that uses the missing postgres substring_index function

seems to me the problem is the use of a mysql specific function in the path.inc drupal_path_alias_whitelist_rebuild function.

Here is a patch that worked for me. It removes the mysql function and replaces it with an php explode with limit of 1.

This is an update to previous. explode limit is weird, so had to up index to 2 to split the string.

This must not be running soon enough...

/**
 * Add the substr_index() function to PostgreSQL.
 *
 * Note: this should go into the driver itself, but we have no support
 * for driver-specific updates yet.
 */
function system_update_7024() {
  if (db_driver() == 'pgsql') {
    db_query('CREATE OR REPLACE FUNCTION "substring_index"(text, text, integer) RETURNS text AS
      \'SELECT array_to_string((string_to_array($1, $2)) [1:$3], $2);\'
      LANGUAGE \'sql\''
    );
  }
}

The attached patch moves system_update_7024() to update_fix_d7_requirements().

It is untested since I do not have pgsql installed at the moment. system_update_7024() is from June 2009, so we wouldn't break the upgrade path for anybody who has installed one of the D7 release candidates.

Likely should be critical, since it blocks the upgrade path for postgresql users, but since I cannot confirm I'll leave at major.

See also #106559: drupal_lookup_path() optimization - skip looking up certain paths in drupal_lookup_path().

(Didn't mean to actually change the title, sorry).

I think that adding a function to postgresql is wrong. If Drupal is to be database agnostic, removing the substring_index from the query in path.inc is the correct approach.

Reworked parented my patch.

How about running the DatabaseTasks from the update workflow?

@vwX: There are heavy performance indications for what you suggest in #12, so I will have to advocate against it. See post #181, #184, etc of #106559: drupal_lookup_path() optimization - skip looking up certain paths in drupal_lookup_path() which I referenced previously.

Read through includes/database/pgsql/install.inc to see that this definition of substring_index is just one of many.

@Stevel: This sounds like a reasonable idea to me, however I don't have time to diagnose the failures coming from the patch in #13 today.

An undefined variable caused by copy/paste from install.core.inc, let's try it again.

#16 looks like the way forward for me.

#16: 1006478-run-db-tasks-on-first-upgrade-without-undefined-variable.patch queued for re-testing.

The failing test works good locally, both on mysql and postgresql. Not sure what is going on here...

The second issue should be resolved by applying #951116: db_change_field() fails to convert int to varchar on PostgreSQL, which is also needed for the D6->D7 upgrade to work properly on postgresql. Not sure about the first one.

There is nothing wrong or to fix with the "MySQL-centric" stuff. We needed this function, it was only available on MySQL so we took the signature and implemented it on PostgreSQL and SQLite (and even on SQL Server).

Locale upgrade path fails this time on the test bot :( First time it was the Node body Upgrade Path. Perhaps third time it'll pass completely?

#16: 1006478-run-db-tasks-on-first-upgrade-without-undefined-variable.patch queued for re-testing.

PostgreSQL upgrade issues are not critical. http://drupal.org/node/951116#comment-3864622

If PostgreSQL was properly supported in Drupal 6, the upgrade path between Drupal 6 and Drupal 7 could be critical. But it wasn't, and nobody ever cared about PostgreSQL during the Drupal 6 era. Downgrading.

The patch in #16 makes sure that database maintenance tasks (from install.inc) are executed when preparing an update. That's a really good idea, and partially unbreaks the upgrade process on PostgreSQL. Let's get this in.

mradcliffe, webchick stated where I linked that these are not criticals. A D7->D8 upgrade will be critical. The reason this is not that many people needed to do unholy things to their postgresql databases to run with D6 and it's very hard to support all those.

Then include me as community unfriendly too. .. not being critical wont stop it from getting in.

WTF at this issue status ping-pong?

We are not holding up a release of software for a million+ people over a broken upgrade path that affects ~100 people, at best. Period.

I have no idea how you go from that fact to thinking that this somehow means that no PostgreSQL bug reports will get addressed or some silly such thing. I really resent the assertion of "having to wait weeks/months/years for HEAD commits ". I've been committing PostgreSQL fixes as fast as they've come in. There's no reason to bloat the criticals list needlessly with them.

Anyway. Committed #16 to HEAD. I'm going to go commit #951116: db_change_field() fails to convert int to varchar on PostgreSQL as well. Another test of the upgrade path after that would be appreciated, as I'd like to roll a new RC in the next day or so.

By running the database tasks when updating, a race condition was introduced when the database tasks from two drupal instances are updated at the same time: there is a "create table, insert/update/delete data, drop table" sequence for an unprefixed table, thus an attempt to create the same table is done by the second instance before the first one has deleted this table. This explains recent random test bot failures for the upgrade tests.

The test replaces the unprefixed tables by the prefixed ones.

Updating title and component to reflect the follow-up issue.

Very nice find and very ugly! (and yes the prefix system is in place by the time these are fired)

Committed to HEAD. Let's see if that makes testbot any happier.