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

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.

@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

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.

In T234795#5554738, @WMDE-leszek wrote:

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

In T234795#5556219, @Anomie wrote:

[…] 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.

"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?

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.

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

Many thanks @Anomie for spotting the problem!