Page MenuHomePhabricator

Wikibase Travis CI jobs failing with the SQL syntax error
Closed, ResolvedPublic

Description

Since around afternoon (Europe time) October, 3rd, Wikibase's Travis CI builds have been erroring.
First seen in: https://travis-ci.org/wikimedia/mediawiki-extensions-Wikibase/builds/592940774. All build since then erroring on the same set of jobs. Interestingly, all jobs that error are sqlite ones.

Example error from the most recent failure (https://travis-ci.org/wikimedia/mediawiki-extensions-Wikibase/jobs/594462818):

Could not insert main page: A database query error has occurred. Did you forget to run your application's database schema updater after upgrading? Query: INSERT INTO revision_actor_temp (revactor_rev,revactor_actor,revactor_timestamp,revactor_page) VALUES ('1','2','20191007080245','1') ON CONFLICT DO UPDATE SET revactor_actor = '2',revactor_timestamp = '20191007080245',revactor_page = '1' Function: ActorMigration::getInsertValuesWithTempTable Error: 1 near "UPDATE": syntax error

.......

Could not insert main page: A database query error has occurred. Did you forget to run your application's database schema updater after upgrading? Query: INSERT INTO revision_actor_temp (revactor_rev,revactor_actor,revactor_timestamp,revactor_page) VALUES ('1','2','20191007080245','1') ON CONFLICT DO UPDATE SET revactor_actor = '2',revactor_timestamp = '20191007080245',revactor_page = '1' Function: ActorMigration::getInsertValuesWithTempTable Error: 1 near "UPDATE": syntax error

[66ce91bcb7ec6da45625faba] [no req]   Wikimedia\Rdbms\DBTransactionError from line 1436 of /home/travis/build/wikimedia/phase3/includes/libs/rdbms/database/Database.php: Explicit transaction still active. A caller may have caught an error. Open transactions: MediaWiki\Storage\PageUpdater::doCreate

Backtrace:
#0 /home/travis/build/wikimedia/phase3/includes/libs/rdbms/loadbalancer/LoadBalancer.php(1602): Wikimedia\Rdbms\Database->assertNoOpenTransactions()
#1 /home/travis/build/wikimedia/phase3/includes/libs/rdbms/loadbalancer/LoadBalancer.php(2109): Wikimedia\Rdbms\LoadBalancer->Wikimedia\Rdbms\{closure}(Wikimedia\Rdbms\DatabaseSqlite)
#2 /home/travis/build/wikimedia/phase3/includes/libs/rdbms/loadbalancer/LoadBalancer.php(1621): Wikimedia\Rdbms\LoadBalancer->forEachOpenMasterConnection(Closure)
#3 /home/travis/build/wikimedia/phase3/includes/libs/rdbms/lbfactory/LBFactory.php(208): Wikimedia\Rdbms\LoadBalancer->approveMasterChanges(array, string, integer)
#4 /home/travis/build/wikimedia/phase3/includes/libs/rdbms/lbfactory/LBFactorySingle.php(96): Wikimedia\Rdbms\LBFactory->Wikimedia\Rdbms\{closure}(Wikimedia\Rdbms\LoadBalancerSingle, string, array)
#5 /home/travis/build/wikimedia/phase3/includes/libs/rdbms/lbfactory/LBFactory.php(210): Wikimedia\Rdbms\LBFactorySingle->forEachLB(Closure, array)
#6 /home/travis/build/wikimedia/phase3/includes/libs/rdbms/lbfactory/LBFactory.php(269): Wikimedia\Rdbms\LBFactory->forEachLBCallMethod(string, array)
#7 /home/travis/build/wikimedia/phase3/maintenance/doMaintenance.php(124): Wikimedia\Rdbms\LBFactory->commitMasterChanges(string)
#8 /home/travis/build/wikimedia/phase3/maintenance/install.php(195): require_once(string)
#9 {main}
The command "bash ./build/travis/install.sh" failed and exited with 255 during .

Other failures stop at the same point, with the same error.

Current random suspect (guess, not git bisected yet) is the following change in mediawiki: https://gerrit.wikimedia.org/r/c/mediawiki/core/+/535337

Event Timeline

WMDE-leszek triaged this task as High priority.Oct 7 2019, 8:39 AM
Ladsgroup added subscribers: aaron, Krinkle.

@Anomie, you know ActorMigration very well, do you know what's going on here?

Anomie added a comment.Oct 7 2019, 8:34 PM

Doesn't seem to be a problem with ActorMigration.

I wonder what version of sqlite is being used there. rMW4bd1b4b45571: rdbms: optimize insert(), replace(), and upsert() for sqlite when possible added use of this new syntax that was added in sqlite 3.24.0, but it also included a version check to use a fallback implementation for earlier versions.

Thanks for the note, upgrading to bionic didn't fix it though: https://travis-ci.org/wikimedia/mediawiki-extensions-Wikibase/jobs/594808130 I will look into this more.

Krinkle added a comment.EditedOct 7 2019, 10:24 PM

@Ladsgroup Can you find out which version of sqlite it uses? https://packages.ubuntu.com/bionic/sqlite3 says it comes with 3.22.0, but not sure if it actually uses that plainly on Travis.

@aaron Can you check if this is using the old or new code? If using the new, might be that the version check doesn't work correctly. If using the old, maybe the fallback has an issue?

In any event, if non-trivial to figure out, I suggest we revert for the time being as the optimisation isn't a must in the short-term afaik.

Travis (Bionic used explicitly) reports for sqlite3 --version

3.22.0 2018-01-22 18:45:57 0c55d179733b46d8d0ba4d88e01a25e10677046ee3da1d5b1581e86726f2alt1

So it should be following the fallback code path

I have a hypotheses that I haven't tested yet but in bionic if you type "sqlite", you get sqlite2 and if you type "sqlite3", you get v3 (similar to python). Our setting is set to sqlite but I don't if travis take it as 2 or 3.

The last release of sqlite 2.x is from 2005, and don't think Travis even provides a way to use such ancient services.

sqlite --version
sqlite: command not found

I hacked up DatabaseSqlite to var dump the result of getServerVersion in upsert method calls (https://github.com/manicki/mediawiki/tree/travis-sqlite-debug), and, interestingly, it says 3.24.0
See: https://travis-ci.org/manicki/mediawiki-extensions-Wikibase/builds/595017133#L513

Anomie added a comment.Oct 8 2019, 4:14 PM

If it's using 3.22 but reporting 3.24, that would be what's causing the problem here.

Or it could just be that the PHP sqlite extension is linked with a different version from the command line client.

I hacked up DatabaseSqlite to var dump the result of getServerVersion in upsert method calls (https://github.com/manicki/mediawiki/tree/travis-sqlite-debug), and, interestingly, it says 3.24.0
See: https://travis-ci.org/manicki/mediawiki-extensions-Wikibase/builds/595017133#L513

[…] Or it could just be that the PHP sqlite extension is linked with a different version from the command line client.

Seems to be the case indeed.

Travis CI log
"3.24.0"

"3.22.0 2018-01-22 …"

Question then becomes, how common is this in practice? If not common, can we consider this GIGO or should we support this?

Anomie added a comment.Oct 8 2019, 4:31 PM

But that doesn't change the fact that whatever PHP is using, it reports 3.24.0 but doesn't have the upsert syntax added in 3.24.0.

@Anomie Ah do you mean php-sqlite3 is not just a driver/client (like for mysql) but actual contains an embedded copy of the lib? If so then yeah, it should indeed behave better. I thought maybe they're separate in which case it's mildly more forgivable to return a newer version if it is indeed the newer driver but dynamically bound to an older shared lib.

Anomie added a comment.EditedOct 8 2019, 4:50 PM

Bah, I figured it out. I should have read https://www.sqlite.org/lang_UPSERT.html more closely. The diagram at the top shows part of the syntax is optional, but the text adds restrictions:

The syntax that occurs in between the "ON CONFLICT" and "DO" keywords is called the "conflict target". The conflict target specifies a specific uniqueness constraint that will trigger the upsert. The conflict target is required for DO UPDATE upserts, but is optional for DO NOTHING. When the conflict target is omitted, the upsert behavior is triggered by a violation of any uniqueness constraint on the table of the INSERT.

So it's not anything to do with sqlite being 3.22 instead of 3.24, it's that rMW4bd1b4b45571: rdbms: optimize insert(), replace(), and upsert() for sqlite when possible missed that required part of the syntax.

I also note that IDatabase::upsert() allows multiple "indexes" to be specified via $uniqueIndexes, but the SQLite syntax allows only one. So it would presumably have to fall back to the parent implementation if multiple indexes are supplied too.

Change 541571 had a related patch set uploaded (by Anomie; owner: Anomie):
[mediawiki/core@master] Revert "rdbms: optimize insert(), replace(), and upsert() for sqlite when possible"

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

Change 541571 merged by jenkins-bot:
[mediawiki/core@master] Revert "rdbms: optimize insert(), replace(), and upsert() for sqlite when possible"

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

Anomie closed this task as Resolved.Oct 8 2019, 5:07 PM
Anomie claimed this task.

Many thanks @Anomie for spotting the problem!