Page MenuHomePhabricator

Clean up retry logic in SqlIdGenerator and UpsertSqlIdGenerator
Open, Needs TriagePublic

Description

SqlIdGenerator and UpsertSqlIdGenerator have some code that’s supposed to retry acquiring an ID if a database query failed:

$success = $database->newUpdateQueryBuilder()
	->update( 'wb_id_counters' )
	->set( [ 'id_value' => $id ] )
	->where( [ 'id_type' => $type ] )
	->caller( __METHOD__ )->execute();
// ...or...
$success = $database->insert(
	'wb_id_counters',
	[
		'id_value' => $id,
		'id_type' => $type,
	],
	__METHOD__
);

// Retry once, since a race condition on initial insert can cause one to fail.
// Race condition is possible due to occurrence of phantom reads is possible
// at non serializable transaction isolation level.
if ( !$success && $retry ) {
	$id = $this->generateNewId( $database, $type, false );
	$success = true;
}

However, IDatabase::update() (or UpdateQueryBuilder::execute()), IDatabase::insert() and IDatabase::upsert() no longer return a success boolean: they either return true or throw an exception, and that’s been the case for years now (at least since rdbms: clean up return values of IDatabase write methods, though it’s not completely clear to me if that introduced the change or not).

We should either make the retry code work correctly again (by catching DB errors that previously would’ve returned false? but that’s discouraged…), or just remove it (on the basis that, apparently, nobody has had an issue with it being broken for years).

Event Timeline

Note: There is a same named class SqlIdGenerator in EntitySchema with the same code.

Note: There is a same named class SqlIdGenerator in EntitySchema with the same code.

which is forked from an older version of SqlIdGenerator in Wikibase - see T213727: Assign incrementing unique IDs to new schemas and (later declined) T215405: Extract Wikibase IdGenerator(s) into a library and use it in WikibaseSchema. Wikibase later did T213817: Use separate DB connection for (Sql/UpsertSql)IdGenerator in Wikibase (because of T194299: Lock wait timeout exceeded in SqlIdGenerator::generateNewId) but EntitySchema does not (since there are never mass creation of EntitySchemas)

The UpsertSqlIdGenerator retry code was removed in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Wikibase/+/1003054; SqlIdGenerator still has some retry code left (though it has no effect since the related change linked previously). So this task remains open, pending a decision to either fully remove and clean up the retry code, or to restore it in a functional form.