Page MenuHomePhabricator

MediaWiki updater error: revision_comment_temp
Open, Needs TriagePublic

Description

I got this when running update.php on a vagrant box that I messed with a fair bit, so it's entirely possible that I just got it into some inconsistent state. The error seems fairly straightforward though: the updater is executed on a wiki which does not have revision_comment_temp yet, it invokes SpecialVersion::getSoftwareInformation(), that invokes the message cache, which uses RevisionStore::getQueryInfo() for joins, which includes revision_comment_temp so things blow up.

$ foreachwiki update.php --quick
-----------------------------------------------------------------
wiki
-----------------------------------------------------------------
wiki:  #!/usr/bin/env php
wiki:  MediaWiki 1.34.0-alpha Updater
wiki:  
wiki:  Wikimedia\Rdbms\DBQueryError from line 1589 of /vagrant/mediawiki/includes/libs/rdbms/database/Database.php: A database query error has occurred. Did you forget to run your application's database schema updater after upgrading? 
wiki:  Query: SELECT  rev_id,rev_page,rev_timestamp,rev_minor_edit,rev_deleted,rev_len,rev_parent_id,rev_sha1,comment_rev_comment.comment_text AS `rev_comment_text`,comment_rev_comment.comment_data AS `rev_comment_data`,comment_rev_comment.comment_id AS `rev_comment_cid`,actor_rev_user.actor_user AS `rev_user`,actor_rev_user.actor_name AS `rev_user_text`,temp_rev_user.revactor_actor AS `rev_actor`,page_namespace,page_title,page_id,page_latest,page_is_redirect,page_len,user_name  FROM `revision` JOIN `revision_comment_temp` `temp_rev_comment` ON ((temp_rev_comment.revcomment_rev = rev_id)) JOIN `comment` `comment_rev_comment` ON ((comment_rev_comment.comment_id = temp_rev_comment.revcomment_comment_id)) JOIN `revision_actor_temp` `temp_rev_user` ON ((temp_rev_user.revactor_rev = rev_id)) JOIN `actor` `actor_rev_user` ON ((actor_rev_user.actor_id = temp_rev_user.revactor_actor)) JOIN `page` ON ((page_id = rev_page)) LEFT JOIN `user` ON ((actor_rev_user.actor_user != 0) AND (user_id = actor_rev_user.actor_user))   WHERE page_is_redirect = '0' AND page_namespace = '8' AND (page_title NOT LIKE '%/%' ESCAPE '`' ) AND (page_len <= 10000) AND (page_latest = rev_id)  
wiki:  Function: MessageCache::loadFromDB(en)-small
wiki:  Error: 1146 Table 'wiki.revision_comment_temp' doesn't exist (127.0.0.1)
wiki:  
wiki:  #0 /vagrant/mediawiki/includes/libs/rdbms/database/Database.php(1557): Wikimedia\Rdbms\Database->getQueryExceptionAndLog('Table 'wiki.rev...', 1146, 'SELECT  rev_id,...', 'MessageCache::l...')
wiki:  #1 /vagrant/mediawiki/includes/libs/rdbms/database/Database.php(1146): Wikimedia\Rdbms\Database->reportQueryError('Table 'wiki.rev...', 1146, 'SELECT  rev_id,...', 'MessageCache::l...', false)
wiki:  #2 /vagrant/mediawiki/includes/libs/rdbms/database/Database.php(1785): Wikimedia\Rdbms\Database->query('SELECT  rev_id,...', 'MessageCache::l...')
wiki:  #3 /vagrant/mediawiki/includes/cache/MessageCache.php(549): Wikimedia\Rdbms\Database->select(Array, Array, Array, 'MessageCache::l...', Array, Array)
wiki:  #4 /vagrant/mediawiki/includes/cache/MessageCache.php(441): MessageCache->loadFromDB('en', NULL)
wiki:  #5 /vagrant/mediawiki/includes/cache/MessageCache.php(364): MessageCache->loadFromDBWithLock('en', Array, NULL)
wiki:  #6 /vagrant/mediawiki/includes/cache/MessageCache.php(1050): MessageCache->load('en')
wiki:  #7 /vagrant/mediawiki/includes/cache/MessageCache.php(977): MessageCache->getMsgFromNamespace('July', 'en')
wiki:  #8 /vagrant/mediawiki/includes/cache/MessageCache.php(947): MessageCache->getMessageForLang(Object(LanguageEn), 'july', true, Array)
wiki:  #9 /vagrant/mediawiki/includes/cache/MessageCache.php(889): MessageCache->getMessageFromFallbackChain(Object(LanguageEn), 'july', true)
wiki:  #10 /vagrant/mediawiki/includes/language/Message.php(1310): MessageCache->get('july', true, Object(LanguageEn))
wiki:  #11 /vagrant/mediawiki/includes/language/Message.php(865): Message->fetchMessage()
wiki:  #12 /vagrant/mediawiki/includes/language/Message.php(957): Message->toString('text')
wiki:  #13 /vagrant/mediawiki/languages/Language.php(954): Message->text()
wiki:  #14 /vagrant/mediawiki/languages/Language.php(972): Language->getMessageFromDB('july')
wiki:  #15 /vagrant/mediawiki/languages/Language.php(1276): Language->getMonthName('07')
wiki:  #16 /vagrant/mediawiki/languages/Language.php(2348): Language->sprintfDate('H:i, j F Y', '20190708124917')
wiki:  #17 /vagrant/mediawiki/includes/specials/SpecialVersion.php(368): Language->timeanddate('20190708124917', true)
wiki:  #18 /vagrant/mediawiki/includes/specials/SpecialVersion.php(320): SpecialVersion::getVersionLinkedGit()
wiki:  #19 /vagrant/mediawiki/includes/specials/SpecialVersion.php(233): SpecialVersion::getVersionLinked()
wiki:  #20 /vagrant/mediawiki/maintenance/update.php(126): SpecialVersion::getSoftwareInformation()
wiki:  #21 /vagrant/mediawiki/maintenance/doMaintenance.php(99): UpdateMediaWiki->execute()
wiki:  #22 /vagrant/mediawiki/maintenance/update.php(270): require_once('/vagrant/mediaw...')
wiki:  #23 /var/www/w/MWScript.php(98): require_once('/vagrant/mediaw...')
wiki:  #24 {main}
1 wikis failed.

Working around it is easy, just get rid of the SpecialVersion::getSoftwareInformation() call or make it return an empty array.

Maybe it's time to get rid of that ridiculous special page dependency in the updater script.

Event Timeline

Tgr created this task.Jul 8 2019, 2:49 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 8 2019, 2:49 PM
Reedy added a subscriber: Reedy.Jul 10 2019, 9:50 AM

Maybe it's time to get rid of that ridiculous special page dependency in the updater script.

Well, it's relatively new and I wanted to avoid re-inventing the wheel/code duplicating (while giving extra debugging info into the update.php output that people do copy paste into bug reports). More so, I didn't didn't really want to add another hook to do the same thing, and with extensions being able to set message keys etc...

Tgr added a comment.Jul 11 2019, 1:52 PM

Invoking message localization in the updater seems like a pretty bad idea, it's not safe to assume you can read from the (unmigrated) database. The format returned by that method doesn't seem super useful for maintenance scripts anyway, it would be best to have a helper that returns structured data and can be used by both.

Anyway, the minimum effort fix is preventing the $wgLang->timeanddate(...) call in SpecialVersion::getVersionLinkedGit() from doing message lookups. Maybe there should be a "safe mode" global flag for messages (used by the updater, installer, and the admin panel once we have one) which overrides Message::$useDatabase?

@daniel @Anomie any thoughts on how to deal with this? In a sense this is a limitation of the migration flag approach, that during updater runs the flag might not accurately reflect the database state. Although you could get into a similar situation without flags too, when you execute DB handling code on a database schema that's much older than what the code was written for.

@daniel @Anomie any thoughts on how to deal with this? In a sense this is a limitation of the migration flag approach, that during updater runs the flag might not accurately reflect the database state.

Yes, we have run into this with the MCR migration as well. Basically, migration code must not rely on the migration flags, but must look at what's actually in the database.

However, this is problematic for code that is not explicitly about migration, but is run inadvertently "on the side". During a migration, the migration flags indicate the target state (this is the only way the wiki owner can specify the desired target state to the updater). However, during the migration, that target stat has not yet been reached.

In a perfect world, we'd have detection code for every migration that could set the flag to the "current" state automatically, while remembering the "target" state from the config. That way, the updater could perform the desired migration safely.

Perhaps the idea of "migration stage" variables should be known to the updater code, so we could provide generic infrastructure for this.

Maybe there should be a "safe mode" global flag for messages (used by the updater, installer, and the admin panel once we have one) which overrides Message::$useDatabase?

For now, that sounds like the best short term solution. Especially since i18n is likely to be used during migration.

@daniel @Anomie any thoughts on how to deal with this?

What you said earlier: Don't try to load i18n (or much of anything else) from the database during the script that's specifically intended for updating the database. I have no real opinion on the details of making that happen.