Page MenuHomePhabricator

sql.php runs LoadExtensionSchemaUpdates
Open, HighPublic

Description

tgr@deployment-tin:~$ mwscript sql.php --wiki=enwiki
...vote_ip in table securepoll_votes already modified by patch /srv/mediawiki-staging/php-master/extensions/SecurePoll/patches/patch-vote_ip-extend.sql.
...echo_subscription doesn't exist.
>

That sounds like sql.php somehow triggered the update process, which really shouldn't be happening.

Event Timeline

Tgr created this task.Feb 9 2017, 3:02 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 9 2017, 3:02 AM
hashar added a subscriber: hashar.Feb 9 2017, 4:06 PM

Maybe because 3a839c5927f7b8b6301e5c5a0698223bf5cd4a4b made sql.php to use the DatabaseUpdater:

+               if ( $index === DB_MASTER ) {
+                       $updater = DatabaseUpdater::newForDB( $db, true, $this );
+                       $db->setSchemaVars( $updater->getSchemaVars() );
+               }
Tgr triaged this task as High priority.Feb 11 2017, 2:42 AM
Tgr added a subscriber: aaron.

Indeed. The updater calls the LoadExtensionSchemaUpdates hook from the constructor (bleh) and Echo and SecurePoll use dropTable/modifyField (which are executed immediately) instead of dropExtensionTable/modifyExtensionField (which would just push an entry to the update list).

This is a very, very ugly accident waiting to happen. @aaron any preference how to prevent it? We could make getSchemaVars static, or split out the extension loading part from the constructor, or add some flag that prevents calling non-extension methods on the updater. Maybe even a unit test which calls the hook with a fake updater which fails the test if any non-extension methods are called on it.

Change 337211 had a related patch set uploaded (by Gergő Tisza):
Fix method name in LoadExtensionSchemaUpdates

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

Change 337212 had a related patch set uploaded (by Gergő Tisza):
Fix method name in LoadExtensionSchemaUpdates

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

Change 337212 merged by jenkins-bot:
Fix method name in LoadExtensionSchemaUpdates

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

Change 337211 merged by jenkins-bot:
Fix method name in LoadExtensionSchemaUpdates

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

Change 340129 had a related patch set uploaded (by Thiemo Mättig (WMDE)):
Fix broken DatabaseUpdater::dropExtensionTable call

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

Change 340129 merged by jenkins-bot:
Fix broken DatabaseUpdater::dropExtensionTable call

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

Tgr renamed this task from Weird behavior of sql.php on beta to sql.php runs LoadExtensionSchemaUpdates.Dec 7 2017, 10:50 PM

Change 396286 had a related patch set uploaded (by Gergő Tisza; owner: Gergő Tisza):
[mediawiki/core@master] Work around sql.php calling DB schema update hooks

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

Change 396286 abandoned by Gergő Tisza:
Work around sql.php calling DB schema update hooks

Reason:
Will do this properly (when I'm not in the middle of a deploy window).

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

Krinkle moved this task from Untriaged to Usage problem on the Wikimedia-Rdbms board.
Krinkle added a subscriber: Krinkle.

@Tgr This task "High priority" is missing a team tag. Could you see if this fits in your team's backlog, and if not, find which team's backlog would be appropiate?

Tgr added a comment.Oct 30 2018, 12:05 AM

Core Platform I guess, since it's a bug in the DB abstraction layer. I can write the patch for it, wouldn't mind some feedback on T157651#3018942 though.

OK. Tagging CPT for the main work (which, with your help, might just be code review).

Also tagging Perf-team for @aaron to advise regarding T157651#3018942 from the rdbms perspective.

Imarlier moved this task from Inbox to Doing on the Performance-Team board.Nov 5 2018, 9:11 PM
Krinkle assigned this task to aaron.Nov 5 2018, 10:12 PM
aaron added a comment.Nov 20 2018, 7:53 PM

Indeed. The updater calls the LoadExtensionSchemaUpdates hook from the constructor (bleh) and Echo and SecurePoll use dropTable/modifyField (which are executed immediately) instead of dropExtensionTable/modifyExtensionField (which would just push an entry to the update list).
This is a very, very ugly accident waiting to happen. @aaron any preference how to prevent it? We could make getSchemaVars static, or split out the extension loading part from the constructor, or add some flag that prevents calling non-extension methods on the updater. Maybe even a unit test which calls the hook with a fake updater which fails the test if any non-extension methods are called on it.

I prefer the fake updater unit test route.

Change 475065 had a related patch set uploaded (by Gergő Tisza; owner: Gergő Tisza):
[mediawiki/core@master] Structure test for wrong calls in LoadExtensionSchemaUpdates

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

Change 475065 had a related patch set uploaded (by Aaron Schulz; owner: Gergő Tisza):
[mediawiki/core@master] Structure test for wrong calls in LoadExtensionSchemaUpdates

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

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

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

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

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

hashar removed a subscriber: hashar.May 21 2019, 9:27 PM
aaron added a comment.Sep 9 2019, 9:42 PM

So, getting this test merged depends on redoing the wikibase schema hook application order for update.php. In CI, there seems to be a problem when it interacts with Flow hooks trying to make pages.

aaron removed aaron as the assignee of this task.Sep 9 2019, 9:42 PM

@aaron are you requesting code review from Core Platform or do you need something else?

aaron added a comment.Sep 30 2019, 8:05 PM

@aaron are you requesting code review from Core Platform or do you need something else?

It would need some debugging and code changes to work with Wikibase+Flow

So, getting this test merged depends on redoing the wikibase schema hook application order for update.php. In CI, there seems to be a problem when it interacts with Flow hooks trying to make pages.

Tagging Wikidata and pinging @Pablo-WMDE and @Ladsgroup for the Wikibase issue.

Anomie added a subscriber: Anomie.

Long term, this sort of thing should wind up being fixed by T191231: RFC: Abstract schemas and schema changes.

Short term, Wikibase changes are unlikely to be in scope for CPT. Looked at the core structure test patch.