So I was trying to test upgrade paths, but suddenly I wasn't able to get anywhere anymore. I don't recall having this issue months ago during beta, but it's been a long time and I can't pinpoint a regression.
I've been able to replicate this several times on either PHP 5.2 or PHP 5.3 on MAMP 1.9 and PostgreSQL 8.4.1 across MAMP/PostgreSQL restarts and system restarts with a very simple process as listed below. I tried with MySQL and found the same process works. I tested with Drupal 6.19, 6.20 paired with Drupal 7 RC1 or RC2. Just tried RC3 with similar results. I'm marking this critical until it gets notched down to major for whatever reason, but I can't upgrade to Drupal 7 from Drupal 6 and there's no error message to indicate WHY (that's why I call this a WSOD as it's practically the same).
- MAMP 1.9: PHP 5.3.2 (or 5.2.13), Apache 2.x, Cache set to none (tried APC as well)
- PostgreSQL 8.4.1 (/Library/PostgreSQL8)
- C2D Macbook running OS 10.6.5
Instructions (cvs or tarball doesn't matter):
- Make a simple Drupal 6 site + db (assume cwd /Applications/MAMP/htdocs and drupal7 pgsql db)
- cvs -z3 co -d drupal7 -r DRUPAL-6-20 drupal
- cp drupal7/sites/default/default.settings.php drupal7/sites/default/settings.php
- Install Drupal 6 w/ pgsql db: http://localhost:8888/drupal7/install.php
- Log in as user 0. Create a node.
- Send the site into Maintenance Mode per instructions. Don't log out per instructions
- Upgrade to Drupal 7
- cvs -z3 update -dcPA -r DRUPAL-7-0-RC-3
- Go to update.php: http://localhost:8888/drupal7/update.php
- Presented with White Screen of Death with text "The website encountered an unexpected error. Please try again later." and no other error message.
I think the important thing here is to figure out why no error message is generated so we don't tear our hair out. For almost every other issue I see with a WSOD, you get a nice PHP exception now.
I'm not sure where to start to trace this as I've never had to delve into Drupal's Bootstrap before either in index.php or update.php.
I've attached a pg_dump of the database with just that one node created.
Comment | File | Size | Author |
---|---|---|---|
#36 | 1006478-followup-concurrency.patch | 2.57 KB | Stevel |
#19 | postgres-upgrade-exceptions.txt | 1.3 KB | mradcliffe |
#16 | 1006478-run-db-tasks-on-first-upgrade-without-undefined-variable.patch | 2.01 KB | Stevel |
#13 | 1006478-run-db-tasks-on-first-upgrade.patch | 2.21 KB | Stevel |
#12 | 1006478.patch | 923 bytes | vwX |
Comments
Comment #1
mradcliffeMeh, knocking it down myself since it's just me until proven otherwise.
Comment #2
mradcliffeOkay, var_dump() actually works here, and I get something useful.
Message
Query
Comment #3
mradcliffeTrying to figure out if update.inc/php goes through DatabaseTasks like install.inc/php...
Comment #4
mradcliffeAs far as I can tell, the runTasks() method is not being run in update.php meaning that every single PostgreSQL D6 -> D7 upgrade should fail.
pgsql is the only driver that has tasks that it runs in its install.inc.
I think this is a critical bug again, but I'll wait for someone else to chime in.
Comment #5
vwX CreditAttribution: vwX commentedHere 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.
Comment #6
vwX CreditAttribution: vwX commentedHere is a patch that worked for me. It removes the mysql function and replaces it with an php explode with limit of 1.
Comment #7
vwX CreditAttribution: vwX commentedThis is an update to previous. explode limit is weird, so had to up index to 2 to split the string.
Comment #8
bfroehle CreditAttribution: bfroehle commentedThis must not be running soon enough...
Comment #9
bfroehle CreditAttribution: bfroehle commentedThe attached patch moves
system_update_7024()
toupdate_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().
Comment #10
bfroehle CreditAttribution: bfroehle commented(Didn't mean to actually change the title, sorry).
Comment #11
vwX CreditAttribution: vwX commentedI 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.
Comment #12
vwX CreditAttribution: vwX commentedReworked parented my patch.
Comment #13
Stevel CreditAttribution: Stevel commentedHow about running the DatabaseTasks from the update workflow?
Comment #15
bfroehle CreditAttribution: bfroehle commented@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.
Comment #16
Stevel CreditAttribution: Stevel commentedAn undefined variable caused by copy/paste from install.core.inc, let's try it again.
Comment #17
Damien Tournoud CreditAttribution: Damien Tournoud commented#16 looks like the way forward for me.
Comment #19
mradcliffeI tried #16, and it gets me to actually testing the update process.
I ran into two errors in the upgrade (see attachment), but I'll test again from the beginning of my process. After these two errors the site is borked with an unknown table.
Upgrading this to critical as it's a complete game stopper & changing the title.
Comment #20
mradcliffeOkay, fresh database, the patch in #16 works (gets passed update.php), and the upgrade process finishes with only one Exception (taxonomy #7009). Drupal 7 does in fact run after that except for that exception. I think that should be a separate issue.
Although I agree with VWx it's probably too late at this stage to fix all of the mysql-centric stuff.
Comment #21
Stevel CreditAttribution: Stevel commented#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.
Comment #22
Damien Tournoud CreditAttribution: Damien Tournoud commentedThere 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).
Comment #24
Stevel CreditAttribution: Stevel commentedLocale 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?
Comment #25
Stevel CreditAttribution: Stevel commented#16: 1006478-run-db-tasks-on-first-upgrade-without-undefined-variable.patch queued for re-testing.
Comment #26
chx CreditAttribution: chx commentedPostgreSQL upgrade issues are not critical. http://drupal.org/node/951116#comment-3864622
Comment #27
mradcliffeThe only way PostgreSQL (or SQLite) can become "supported" is if we start treating critical issues as critical. If we treat them as second-class citizens they'll remain "unsupported" for Drupal 8, 9, 10, etc...
Can you give me a better reason why seeing this going to update.php shouldn't be treated as critical to release? Damien Tournaroud's cyclical argument doesn't make any sense in this context at all. I could see this being a major issue if maybe there was an error message or an official announcement that existing PostgreSQL should continue to use Drupal 6 instead of upgrading. If we did this, then I'll admit it's not a release blocker.
Treating this as critical and getting it done before Jan. 5 at least gives PostgreSQL users a fighting chance at attempting an upgrade instead of potentially having to wait weeks/months/years for HEAD commits and back ports before even being able to attempt an upgrade.
Comment #28
mradcliffeSince the tests pass now I think we need a couple of more manual Q/A, and this is RTBC to solve the initial problem.
The issue about why it's not reporting errors at that stage of bootstrap might be for a follow-up issue.
Comment #29
Damien Tournoud CreditAttribution: Damien Tournoud commentedIf 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.
Comment #30
Damien Tournoud CreditAttribution: Damien Tournoud commentedComment #31
mradcliffeThat's still a cyclical argument. By this logic, PostgreSQL will never be "supported" because it is "unsupported" in the previous version. This does not make one bit of sense, Damien.
Can you offer a valid reason why this shouldn't be critical?
Edit: Contrarily, if you start considering it critical then PostgreSQL may graduate from this "unsupported" status.
Comment #32
chx CreditAttribution: chx commentedmradcliffe, 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.
Comment #33
mradcliffeI hope you're right in a couple of years... It still seems sad and silly for this not to be critical, and that excuse by webchick and Damien holds no water in this specific issue with the screenshot I posted. The argument is not a community-friendly sentiment no matter what.
Comment #34
chx CreditAttribution: chx commentedThen include me as community unfriendly too. .. not being critical wont stop it from getting in.
Comment #35
webchickWTF 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.
Comment #36
Stevel CreditAttribution: Stevel commentedBy 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.
Comment #37
Stevel CreditAttribution: Stevel commentedUpdating title and component to reflect the follow-up issue.
Comment #38
chx CreditAttribution: chx commentedVery nice find and very ugly! (and yes the prefix system is in place by the time these are fired)
Comment #39
webchickCommitted to HEAD. Let's see if that makes testbot any happier.