Page MenuHomePhabricator
Create Task
Maniphest T249598

Wikibase schema updaters must not modify database directly
Closed, ResolvedPublic
Actions

Description

Wikibase uses the LoadExtensionSchemaUpdates hook for database schema updates. Handlers of this hook must not modify the database directly, but must instead register updates for later execution. Wikibase does apply modifications directly in:

  • /workspace/src/extensions/Wikibase/repo/includes/Store/Sql/ChangesSubscriptionSchemaUpdater.php:48
  • /workspace/src/extensions/Wikibase/repo/includes/Store/Sql/DatabaseSchemaUpdater.php:78

This can lead to database corruption in production, see T157651.

There is a structure test up for review to prevent this kind of mistake: https://gerrit.wikimedia.org/r/c/mediawiki/core/+/475065. However, it cannot be merged until Wikibase is fixed.

Details

SubjectRepoBranchLines +/-
Register schema update callbacks rather than running them on DatabaseUpdater constructionmediawiki/extensions/Wikibasemaster+11 -18
Remove the last use of dropTable in schema updatermediawiki/extensions/Wikibasemaster+1 -1
Use dropExtensionTable instead of dropTable in the updatermediawiki/extensions/Wikibasemaster+6 -6
Customize query in gerrit

Related Objects
Search...

Event Timeline

daniel triaged this task as High priority.Apr 7 2020, 9:47 AM
daniel created this task.
daniel mentioned this in T157651: sql.php must not run LoadExtensionSchemaUpdates.
gerritbot added a comment.Apr 7 2020, 10:17 AM

Change 587223 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] DatabaseUpdater: don't call hook in constructor

https://gerrit.wikimedia.org/r/587223

gerritbot added a project: Patch-For-Review.Apr 7 2020, 10:17 AM
daniel removed a project: Patch-For-Review.Apr 7 2020, 10:20 AM

No patch yet, one one above was tagged with this ticket erronously.

Addshore mentioned this in T249565: Wikidata's wb_items_per_site table has suddenly disappeared, creating DBQueryErrors on page views.Apr 7 2020, 10:22 AM
gerritbot added a comment.Apr 7 2020, 10:41 AM

Change 587223 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] DatabaseUpdater: don't call hook in constructor

https://gerrit.wikimedia.org/r/587223

gerritbot added a project: Patch-For-Review.Apr 7 2020, 10:41 AM
daniel mentioned this in T249603: DatabaseUpdater: protect methods for direct database modification .Apr 7 2020, 11:47 AM
daniel added a parent task: T249603: DatabaseUpdater: protect methods for direct database modification .
akosiaris updated the task description. (Show Details)Apr 7 2020, 12:27 PM
gerritbot added a comment.Apr 7 2020, 12:45 PM

Change 587248 had a related patch set uploaded (by Ladsgroup; owner: Ladsgroup):
[mediawiki/extensions/Wikibase@master] Use dropExtensionTable instead of dropTable in the updater

https://gerrit.wikimedia.org/r/587248

gerritbot added a comment.Apr 7 2020, 1:26 PM

Change 587248 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Use dropExtensionTable instead of dropTable in the updater

https://gerrit.wikimedia.org/r/587248

ReleaseTaggerBot added a project: MW-1.35-notes (1.35.0-wmf.27; 2020-04-07).Apr 7 2020, 2:00 PM
Jdforrester-WMF subscribed.Apr 7 2020, 3:52 PM
gerritbot added a comment.Apr 8 2020, 6:11 AM

Change 587402 had a related patch set uploaded (by Ladsgroup; owner: Ladsgroup):
[mediawiki/extensions/Wikibase@master] Remove the last big of dropTable in schema updater

https://gerrit.wikimedia.org/r/587402

gerritbot added a comment.Apr 8 2020, 8:40 AM

Change 587402 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Remove the last use of dropTable in schema updater

https://gerrit.wikimedia.org/r/587402

ReleaseTaggerBot edited projects, added MW-1.35-notes (1.35.0-wmf.28; 2020-04-14); removed MW-1.35-notes (1.35.0-wmf.27; 2020-04-07).Apr 8 2020, 10:00 AM
daniel added a comment.Apr 20 2020, 9:23 PM

Looks like this is done, can the ticket be closed?

daniel added a comment.Apr 20 2020, 9:28 PM

Oh wait, this patch also belongs here, right?
https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Wikibase/+/507898

gerritbot added a comment.Apr 20 2020, 9:29 PM

Change 507898 had a related patch set uploaded (by Daniel Kinzler; owner: Aaron Schulz):
[mediawiki/extensions/Wikibase@master] Register schema update callbacks rather than running them on DatabaseUpdater construction

https://gerrit.wikimedia.org/r/507898

daniel added a comment.Apr 21 2020, 9:05 AM
In T249598#6073487, @daniel wrote:

Oh wait, this patch also belongs here, right?
https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Wikibase/+/507898

Actually, I think that patch is wrong.

So, can we close this?

gerritbot added a comment.Apr 21 2020, 7:05 PM

Change 507898 abandoned by Aaron Schulz:
Register schema update callbacks rather than running them on DatabaseUpdater construction

Reason:
obsolete

https://gerrit.wikimedia.org/r/507898

Addshore moved this task from Incoming to Prioritized Wikidata Tech Backlog (prioritised from top to bottom) on the Wikidata-Campsite board.May 5 2020, 11:56 AM
Addshore moved this task from Prioritized Wikidata Tech Backlog (prioritised from top to bottom) to Wikidata-Campsite-Iteration-∞ (On Hold) on the Wikidata-Campsite board.May 5 2020, 12:04 PM
Addshore edited projects, added Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)); removed Wikidata-Campsite.
Addshore moved this task from To Do (prioritised from top to bottom) to Test (Verification) on the Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)) board.
Maintenance_bot moved this task from incoming to in progress on the Wikidata board.May 5 2020, 12:15 PM
Michael subscribed.Jun 11 2020, 1:24 PM
In T249598#6074491, @daniel wrote:

So, can we close this?

The original core test still throws errors that seem related to Wikibase, but, OTOH, it is currently 1555 commits behind master. Could you verify whether those errors are still an issue that needs resolved in Wikibase or whether they are just an artifact from the patch needing to be rebased?

See https://integration.wikimedia.org/ci/job/wmf-quibble-core-vendor-mysql-php72-docker/28710/console

12:50:45 There were 3 errors:
12:50:45 
12:50:45 1) LoadExtensionSchemaUpdatesTest::testLoadExtensionSchemaUpdates with data set #0 ('mysql', 'MysqlUpdater')
12:50:45 Wikimedia\Rdbms\DBUnexpectedError: DB connection was already closed
12:50:45 
12:50:45 /workspace/src/includes/libs/rdbms/database/Database.php:967
12:50:45 /workspace/src/includes/libs/rdbms/database/Database.php:1242
12:50:45 /workspace/src/includes/libs/rdbms/database/Database.php:1211
12:50:45 /workspace/src/includes/libs/rdbms/database/DatabaseMysqlBase.php:556
12:50:45 /workspace/src/extensions/Wikibase/repo/includes/Store/Sql/DatabaseSchemaUpdater.php:77
12:50:45 /workspace/src/extensions/Wikibase/repo/includes/Store/Sql/DatabaseSchemaUpdater.php:61
12:50:45 /workspace/src/includes/HookContainer/HookContainer.php:319
12:50:45 /workspace/src/includes/HookContainer/HookContainer.php:131
12:50:45 /workspace/src/includes/Hooks.php:136
12:50:45 /workspace/src/tests/phpunit/structure/LoadExtensionSchemaUpdatesTest.php:94
12:50:45 /workspace/src/tests/phpunit/MediaWikiIntegrationTestCase.php:437
12:50:45 /workspace/src/maintenance/doMaintenance.php:105
12:50:45 === Logs generated by test case
12:50:45 [objectcache] [debug] MainWANObjectCache using store {class} {"class":"EmptyBagOStuff"}
12:50:45 [localisation] [debug] LocalisationCache using store LCStoreNull []
12:50:45 [Wikibase] [debug] {method}: setting {settingName} was given as a closure, resolve it to {logValue} {"method":"Wikibase\\Lib\\SettingsArray::getSetting","settingName":"conceptBaseUri","logValue":"'http:\/\/127.0.0.1:9412\/entity\/'"}
12:50:45 [objectcache] [debug] MainWANObjectCache using store {class} {"class":"EmptyBagOStuff"}
12:50:45 ===
12:50:45 
12:50:45 2) LoadExtensionSchemaUpdatesTest::testLoadExtensionSchemaUpdates with data set #1 ('postgres', 'PostgresUpdater')
12:50:45 Wikimedia\Rdbms\DBUnexpectedError: DB connection was already closed
12:50:45 
12:50:45 /workspace/src/includes/libs/rdbms/database/Database.php:967
12:50:45 /workspace/src/includes/libs/rdbms/database/Database.php:1242
12:50:45 /workspace/src/includes/libs/rdbms/database/Database.php:1211
12:50:45 /workspace/src/includes/libs/rdbms/database/DatabaseMysqlBase.php:556
12:50:45 /workspace/src/includes/installer/DatabaseUpdater.php:394
12:50:45 /workspace/src/extensions/Wikibase/repo/includes/Store/Sql/ChangesSubscriptionSchemaUpdater.php:48
12:50:45 /workspace/src/extensions/Wikibase/repo/includes/Store/Sql/ChangesSubscriptionSchemaUpdater.php:37
12:50:45 /workspace/src/includes/HookContainer/HookContainer.php:319
12:50:45 /workspace/src/includes/HookContainer/HookContainer.php:131
12:50:45 /workspace/src/includes/Hooks.php:136
12:50:45 /workspace/src/tests/phpunit/structure/LoadExtensionSchemaUpdatesTest.php:94
12:50:45 /workspace/src/tests/phpunit/MediaWikiIntegrationTestCase.php:437
12:50:45 /workspace/src/maintenance/doMaintenance.php:105
12:50:45 === Logs generated by test case
12:50:45 [objectcache] [debug] MainWANObjectCache using store {class} {"class":"EmptyBagOStuff"}
12:50:45 [localisation] [debug] LocalisationCache using store LCStoreNull []
12:50:45 [Wikibase] [debug] {method}: setting {settingName} was given as a closure, resolve it to {logValue} {"method":"Wikibase\\Lib\\SettingsArray::getSetting","settingName":"conceptBaseUri","logValue":"'http:\/\/127.0.0.1:9412\/entity\/'"}
12:50:45 [objectcache] [debug] MainWANObjectCache using store {class} {"class":"EmptyBagOStuff"}
12:50:45 [warning] [info] Database type 'postgres' is not supported by the Wikibase repository. [Called from Wikibase\Repo\Store\Sql\DatabaseSchemaUpdater::doSchemaUpdate in /workspace/src/extensions/Wikibase/repo/includes/Store/Sql/DatabaseSchemaUpdater.php at line 71] {"private":false}
12:50:45 ===
12:50:45 
12:50:45 3) LoadExtensionSchemaUpdatesTest::testLoadExtensionSchemaUpdates with data set #2 ('sqlite', 'SqliteUpdater')
12:50:45 Wikimedia\Rdbms\DBUnexpectedError: DB connection was already closed
12:50:45 
12:50:45 /workspace/src/includes/libs/rdbms/database/Database.php:967
12:50:45 /workspace/src/includes/libs/rdbms/database/Database.php:1242
12:50:45 /workspace/src/includes/libs/rdbms/database/Database.php:1211
12:50:45 /workspace/src/includes/libs/rdbms/database/DatabaseMysqlBase.php:556
12:50:45 /workspace/src/extensions/Wikibase/repo/includes/Store/Sql/DatabaseSchemaUpdater.php:77
12:50:45 /workspace/src/extensions/Wikibase/repo/includes/Store/Sql/DatabaseSchemaUpdater.php:61
12:50:45 /workspace/src/includes/HookContainer/HookContainer.php:319
12:50:45 /workspace/src/includes/HookContainer/HookContainer.php:131
12:50:45 /workspace/src/includes/Hooks.php:136
12:50:45 /workspace/src/tests/phpunit/structure/LoadExtensionSchemaUpdatesTest.php:94
12:50:45 /workspace/src/tests/phpunit/MediaWikiIntegrationTestCase.php:437
12:50:45 /workspace/src/maintenance/doMaintenance.php:105
12:50:45 === Logs generated by test case
12:50:45 [objectcache] [debug] MainWANObjectCache using store {class} {"class":"EmptyBagOStuff"}
12:50:45 [localisation] [debug] LocalisationCache using store LCStoreNull []
12:50:45 [Wikibase] [debug] {method}: setting {settingName} was given as a closure, resolve it to {logValue} {"method":"Wikibase\\Lib\\SettingsArray::getSetting","settingName":"conceptBaseUri","logValue":"'http:\/\/127.0.0.1:9412\/entity\/'"}
12:50:45 [objectcache] [debug] MainWANObjectCache using store {class} {"class":"EmptyBagOStuff"}
12:50:45 ===
12:50:45 
12:50:45 ERRORS!
12:50:45 Tests: 4236, Assertions: 14220, Errors: 3, Skipped: 65.
daniel added a comment.Jun 12 2020, 11:18 AM

The original core test still throws errors that seem related to Wikibase

You mean this one? https://gerrit.wikimedia.org/r/c/mediawiki/core/+/475065

As I noted there:

The "dangerous" methods on DatabaseUpdater are no longer public, see Ie73e6143bb5cbce0dae705a7451fb49cf7e3abcd.
The structure test would need to be changed to prevent access to dangerous methods on the Database object.

I'm not sure how exactly the "DB connection was already closed" issue is triggered, but the structure test as written no longer makes sense.

Addshore closed this task as Resolved.Jul 6 2020, 9:06 PM
Addshore claimed this task.
Addshore moved this task from Test (Verification) to Done on the Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)) board.
Restricted Application added a project: User-Addshore. · View Herald TranscriptJul 6 2020, 9:06 PM
Aklapper removed a subscriber: Anomie.Oct 16 2020, 5:40 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits