Page MenuHomePhabricator

SQL error with postgres during 1.30 update.php run
Closed, ResolvedPublic

Description

Seems to be a syntax error in there.

[3dfa8c153031fbe511c84a48] [no req]   Wikimedia\Rdbms\DBQueryError from line 1149 of /home/aaron/OSS/core/includes/libs/rdbms/database/Database.php: A database query error has occurred. Did you forget to run your application's database schema updater after upgrading? 
Query: ALTER TABLE revision ALTER rev_comment SET DEFAULT 
Function: Wikimedia\Rdbms\Database::query
Error: 42601 ERROR:  syntax error at end of input
LINE 1: ...spire-S5... */ TABLE revision ALTER rev_comment SET DEFAULT 
                                                                       ^


Backtrace:
#0 /home/aaron/OSS/core/includes/libs/rdbms/database/DatabasePostgres.php(262): Wikimedia\Rdbms\Database->reportQueryError(string, string, string, string, boolean)
#1 /home/aaron/OSS/core/includes/libs/rdbms/database/Database.php(979): Wikimedia\Rdbms\DatabasePostgres->reportQueryError(string, string, string, string, boolean)
#2 /home/aaron/OSS/core/includes/installer/PostgresUpdater.php(786): Wikimedia\Rdbms\Database->query(string)
#3 /home/aaron/OSS/core/includes/installer/DatabaseUpdater.php(472): PostgresUpdater->setDefault(string, string, string)
#4 /home/aaron/OSS/core/includes/installer/DatabaseUpdater.php(436): DatabaseUpdater->runUpdates(array, boolean)
#5 /home/aaron/OSS/core/maintenance/update.php(174): DatabaseUpdater->doUpdates(array)
#6 /home/aaron/OSS/core/maintenance/doMaintenance.php(92): UpdateMediaWiki->execute()
#7 /home/aaron/OSS/core/maintenance/update.php(219): require_once(string)

Event Timeline

aaron created this task.Sep 9 2017, 12:29 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 9 2017, 12:29 AM
Paladox added a subscriber: Paladox.Sep 9 2017, 1:23 AM
Reedy added a subscriber: Anomie.Sep 9 2017, 2:26 AM
Reedy added a subscriber: Reedy.Sep 9 2017, 3:40 PM

Do you know what file it's running? Or it thinks it's running...

I see the following in mysql's patch-comment-table.sql

ALTER TABLE /*_*/revision
  ALTER COLUMN rev_comment SET DEFAULT '';

https://github.com/wikimedia/mediawiki/blob/master/maintenance/archives/patch-comment-table.sql#L28-L29

vs

https://github.com/wikimedia/mediawiki/blob/master/maintenance/postgres/archives/patch-comment-table.sql

Why is it running mysql's patch-comment-table.sql when Postgres has it's own?

saper added a subscriber: saper.Sep 9 2017, 3:49 PM

SQL injection? We do not quote $default at all?

i think '' could be turning into false. maybe if we did $default = '', by default that may fix the problem.

Reedy added a comment.Sep 9 2017, 4:11 PM

i think '' could be turning into false. maybe if we did $default = '', by default that may fix the problem.

No it won't.

php > var_dump( "Foo " . '' );
php shell code:1:
string(4) "Foo "

Of course, setDefault isn't new... https://github.com/wikimedia/mediawiki/commit/f752cf80423615b380cf5612a3f1f68a6b9d0173

SQL injection? We do not quote $default at all?

Apparently not

	protected function setDefault( $table, $field, $default ) {
		$info = $this->db->fieldInfo( $table, $field );
		if ( $info->defaultValue() !== $default ) {
			$this->output( "Changing '$table.$field' default value\n" );
			$this->db->query( "ALTER TABLE $table ALTER $field SET DEFAULT " . $default );
		}
	}

Change 376907 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/core@master] Quote $default in PostgresUpdater::setDefault

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

Reedy added a comment.Sep 9 2017, 4:15 PM

Add quotes should handle things like null fine too...

> var_dump( "Foo " . $db->addQuotes( '' ) );
/var/www/wiki/mediawiki/core/maintenance/eval.php(78) : eval()'d code:1:
string(6) "Foo ''"

> var_dump( "Foo " . $db->addQuotes( null ) );
/var/www/wiki/mediawiki/core/maintenance/eval.php(78) : eval()'d code:1:
string(8) "Foo NULL"
Reedy added a comment.Sep 9 2017, 4:18 PM

I also note.. There's a lot of dodgy other wheel reinventing for handling this in other PostgresUpdater code...

https://github.com/wikimedia/mediawiki/blob/11cf01dd9a8512ad4d9bded43cf22ebd38af8818/includes/installer/PostgresUpdater.php#L742-L748

				if ( preg_match( '/DEFAULT (.+)/', $default, $res ) ) {
					$sqldef = "ALTER TABLE $table ALTER $field SET DEFAULT $res[1]";
					$this->db->query( $sqldef );
					$default = preg_replace( '/\s*DEFAULT .+/', '', $default );
				}
				$sql .= " USING $default";

changeField and changeFieldPurgeTable use this code...

Reedy added a comment.Sep 9 2017, 4:22 PM

We should probably be quoting $table and $field. But with where it should be generally called from... It's less of an issue, ish

Reedy added a comment.Sep 9 2017, 4:31 PM

I guess that lot should be filed in a separate ticket

Change 376907 merged by jenkins-bot:
[mediawiki/core@master] Quote $default in PostgresUpdater::setDefault

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

Change 377670 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/core@REL1_27] Quote $default in PostgresUpdater::setDefault

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

Change 377671 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/core@REL1_28] Quote $default in PostgresUpdater::setDefault

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

Change 377672 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/core@REL1_29] Quote $default in PostgresUpdater::setDefault

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

Change 377672 merged by jenkins-bot:
[mediawiki/core@REL1_29] Quote $default in PostgresUpdater::setDefault

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

Change 377671 merged by jenkins-bot:
[mediawiki/core@REL1_28] Quote $default in PostgresUpdater::setDefault

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

Change 377670 merged by jenkins-bot:
[mediawiki/core@REL1_27] Quote $default in PostgresUpdater::setDefault

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

Reedy closed this task as Resolved.Sep 13 2017, 7:59 PM
Reedy triaged this task as Normal priority.
Reedy claimed this task.