Page MenuHomePhabricator

Bug in Database::getTempTableWrites is causing a flood of warnings from phpunit when using Postgres
Closed, ResolvedPublic

Description

PostgresUpdater is hitting a bug in Database::getTempTableWrites, causing a flood of warnings to be emitted (and presumably incorrect behavior) in unit tests:

Warning: Invalid argument supplied for foreach() in /var/www/html/includes/libs/rdbms/database/Database.php on line 1137
Call Stack:

  0.0044     403408   1. {main}() /var/www/html/tests/phpunit/phpunit.php:0
  0.0154     688288   2. require('/var/www/html/maintenance/doMaintenance.php') /var/www/html/tests/phpunit/phpunit.php:130
  0.2809   11303824   3. PHPUnitMaintClass->execute() /var/www/html/maintenance/doMaintenance.php:105
  0.2999   12226376   4. MediaWikiPHPUnitCommand->run() /var/www/html/tests/phpunit/phpunit.php:71
 81.8041  322215352   5. PHPUnit\TextUI\TestRunner->doRun() /var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:204
239.6191  326764192   6. PHPUnit\Framework\TestSuite->run() /var/www/html/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:621
257.5556  326768448   7. PHPUnit\Framework\TestSuite->run() /var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:597
262.3867  326830928   8. PHPUnit\Framework\TestSuite->run() /var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:597
286.5867  333170816   9. PHPUnit\Framework\DataProviderTestSuite->run() /var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:597
292.1301  334050704  10. ActorMigrationTest->run() /var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:597
292.2962  334124728  11. ActorMigrationTest->resetDB() /var/www/html/tests/phpunit/MediaWikiIntegrationTestCase.php:429
292.3170  334136488  12. ActorMigrationTest->truncateTables() /var/www/html/tests/phpunit/MediaWikiIntegrationTestCase.php:1810
292.3170  334136488  13. Wikimedia\Rdbms\DatabasePostgres->truncate() /var/www/html/tests/phpunit/MediaWikiIntegrationTestCase.php:1836
292.3180  334136864  14. Wikimedia\Rdbms\DatabasePostgres->doTruncate() /var/www/html/includes/libs/rdbms/database/Database.php:5096
292.3191  334137512  15. Wikimedia\Rdbms\DatabasePostgres->query() /var/www/html/includes/libs/rdbms/database/DatabasePostgres.php:814
292.3193  334137512  16. Wikimedia\Rdbms\DatabasePostgres->executeQuery() /var/www/html/includes/libs/rdbms/database/Database.php:1212
292.3195  334137512  17. Wikimedia\Rdbms\DatabasePostgres->getTempTableWrites() /var/www/html/includes/libs/rdbms/database/Database.php:1255

The reason is this loop in Database::getTempTableWrites:

foreach ( ( $m[3] ?? [] ) as $quotedTable ) {
	$queryTables[] = trim( $quotedTable, "\"'`" );
}

Here, $m[3] is a capture group from preg_match, which is either null or a string. When it is a string, the warning is triggered. The intent here is that $m[3] should be an array from an expression that matches multiple times, namely (?:\s*,\s*(\w+|\w+|'\w+'|"\w+"))*. PCRE does not support this kind of multiple matching, the capture group will just record the last match (compare https://stackoverflow.com/questions/41582889/repeated-capturing-group-pcre).

The solution would be to capture the entire list as a single string using something like ((?:\s*,\s*(?:\w+|\w+|'\w+'|"\w+"))*)?, and split it up on the "," later.

Event Timeline

daniel renamed this task from Bug in Database::getTempTableWrites is causing a flood of warnings from PostgresUpdater to Bug in Database::getTempTableWrites is causing a flood of warnings from phpunit when using PostGres.May 8 2020, 8:30 AM
daniel added a project: PostgreSQL.
daniel triaged this task as Medium priority.May 8 2020, 9:45 AM
daniel removed a project: Platform Engineering.

See also T246358 and T214552 which would have likelt caught this in CI.

Krinkle renamed this task from Bug in Database::getTempTableWrites is causing a flood of warnings from phpunit when using PostGres to Bug in Database::getTempTableWrites is causing a flood of warnings from phpunit when using Postgres.May 21 2020, 1:35 AM
Krinkle moved this task from Limbo to Watching on the Performance-Team (Radar) board.

Change 616856 had a related patch set uploaded (by Umherirrender; owner: Umherirrender):
[mediawiki/core@master] Fix Database::getTempTableWrites for multi table DDLs

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

Change 616856 merged by jenkins-bot:
[mediawiki/core@master] Fix Database::getTempTableWrites for multi table DDLs

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

Change 637791 had a related patch set uploaded (by Edward Chernenko; owner: Umherirrender):
[mediawiki/core@REL1_35] Fix Database::getTempTableWrites for multi table DDLs

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

Change 637791 merged by jenkins-bot:
[mediawiki/core@REL1_35] Fix Database::getTempTableWrites for multi table DDLs

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