Page MenuHomePhabricator

Enable MYSQLI_CLIENT_FOUND_ROWS option for consistency with other RDBMS backends
Closed, ResolvedPublic

Description

Enabling the MYSQLI_CLIENT_FOUND_ROWS option will make affectedRows() after UPDATE queries reflect the number of matching rows, regardless of whether any column values had to be changed. This means if the row was already in the state the query wanted to ensure, it will still be counted.

This is, I think, what callers want to know and is consistent with how Postgres, SQLite, and MSSQL behave already.

Event Timeline

Krinkle created this task.Jul 31 2019, 5:53 PM
Restricted Application added a project: Platform Engineering. · View Herald TranscriptJul 31 2019, 5:53 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Change 524099 had a related patch set uploaded (by Krinkle; owner: Aaron Schulz):
[mediawiki/core@master] rdbms: set MYSQLI_CLIENT_FOUND_ROWS in DatabaseMysqli

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

Change 526577 had a related patch set uploaded (by Krinkle; owner: Aaron Schulz):
[mediawiki/core@master] Clean up use of IDatabase::affectedRows() in WikiPage::updateRedirectOn()

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

Change 526587 had a related patch set uploaded (by Krinkle; owner: Aaron Schulz):
[mediawiki/extensions/FlaggedRevs@master] Clean up the use of IDatabase::affectedRows()

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

Change 526595 had a related patch set uploaded (by Krinkle; owner: Aaron Schulz):
[mediawiki/extensions/Newsletter@master] Cleanup use of IDatabase::affectedRows()

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

Change 526594 had a related patch set uploaded (by Krinkle; owner: Aaron Schulz):
[mediawiki/extensions/PageTriage@master] Cleanup setTriageStatus() and IDatabase::affectedRows() usage

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

Change 526578 had a related patch set uploaded (by Krinkle; owner: Aaron Schulz):
[mediawiki/core@master] Cleanup JobQueueDB::recycleAndDeleteStaleJobs() use of IDatabase::affectedRows()

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

Change 526587 merged by jenkins-bot:
[mediawiki/extensions/FlaggedRevs@master] Clean up the use of IDatabase::affectedRows()

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

Change 526595 merged by jenkins-bot:
[mediawiki/extensions/Newsletter@master] Cleanup use of IDatabase::affectedRows()

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

Change 526576 had a related patch set uploaded (by Krinkle; owner: Aaron Schulz):
[mediawiki/core@master] Cleanup UserGroupMembership::insert() and make it more atomic

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

Change 526576 merged by jenkins-bot:
[mediawiki/core@master] Cleanup UserGroupMembership::insert() and make it more atomic

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

Krinkle triaged this task as Medium priority.Aug 6 2019, 5:56 PM
Krinkle moved this task from Inbox to Radar on the Performance-Team board.
Krinkle edited projects, added Performance-Team (Radar); removed Performance-Team.
Krinkle moved this task from Limbo to Watching on the Performance-Team (Radar) board.

Change 526577 merged by jenkins-bot:
[mediawiki/core@master] Clean up use of IDatabase::affectedRows() in WikiPage::updateRedirectOn()

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

Change 526578 merged by jenkins-bot:
[mediawiki/core@master] Cleanup JobQueueDB::recycleAndDeleteStaleJobs() use of IDatabase::affectedRows()

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

Change 524099 merged by jenkins-bot:
[mediawiki/core@master] rdbms: set MYSQLI_CLIENT_FOUND_ROWS in DatabaseMysqli

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

Tgr added a subscriber: Tgr.Aug 16 2019, 12:04 PM

This is significantly less useful than the old behavior. Affected_rows is typically used to either skip expensive cache purges when nothing actually changed, or signal to the user whether they actually managed to change something. (Why would a caller care about rows matched but not changed?)

I guess consistency trumps utility here... but if there was a way to change the other engines in the other direction instead, that would be much preferable.

Change 530564 had a related patch set uploaded (by Gergő Tisza; owner: Gergő Tisza):
[mediawiki/core@master] Fix IDatabase::affectedRows() documentation

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

Tgr added a comment.EditedAug 16 2019, 12:20 PM

Working around the new behavior is not hard but ugly:

// old code
$db->update( 't',
    [ 'c1' => $v1, 'c2' => $v2, 'c3' => $v3 ],
    [ 'id' => 1 ],
    __METHOD__
);

// new code
$db->update( 't',
    [ 'c1' => $v1, 'c2' => $v2, 'c3' => $v3 ],
    [
        'id' => 1,
        $db->makeList( [
            'c1 != ' . $db->addQuotes( $v1 ),
            'c2 != ' . $db->addQuotes( $v2 ),
            'c3 != ' . $db->addQuotes( $v3 ),
        ], IDatabase::LIST_OR ),
    ],
    __METHOD__
);

and then a lot more complicated than that if the values can be null or array. I wonder if it would make sense to have a helper function / flag for automatically adding that kind of check...

Change 530564 merged by jenkins-bot:
[mediawiki/core@master] Fix IDatabase::affectedRows() documentation

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

aaron closed this task as Resolved.Aug 21 2019, 7:25 PM

This is significantly less useful than the old behavior. Affected_rows is typically used to either skip expensive cache purges when nothing actually changed, or signal to the user whether they actually managed to change something. (Why would a caller care about rows matched but not changed?)

SqlBagOStuff::changeTTL(), for example, only cares if there was a row. Callers have more control by being able to use the WHERE clause to decide when to care about column values changing or not.

The code example above could probably be simplified:

$set = [ 'c1' => $v1, 'c2' => $v2, 'c3' => $v3 ];
$db->update( 
	't',
	$set,
	[ 'id' => 1, 'NOT (' . $db->makeList( $set, IDatabase::LIST_AND ) . ')' ],
	__METHOD__
);

Change 526594 merged by jenkins-bot:
[mediawiki/extensions/PageTriage@master] Cleanup setTriageStatus() and IDatabase::affectedRows() usage

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