Page MenuHomePhabricator
Create Task
Maniphest T175439

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

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)

Details

Related Changes in Gerrit:
SubjectRepoBranchLines +/-
Quote $default in PostgresUpdater::setDefaultmediawiki/coreREL1_27+2 -1
Quote $default in PostgresUpdater::setDefaultmediawiki/coreREL1_28+2 -1
Quote $default in PostgresUpdater::setDefaultmediawiki/coreREL1_29+2 -1
Quote $default in PostgresUpdater::setDefaultmediawiki/coremaster+2 -1
Customize query in gerrit

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 subscribed.Sep 9 2017, 1:23 AM
Reedy added a subscriber: Anomie.Sep 9 2017, 2:26 AM
Reedy subscribed.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?

Paladox added a comment.Sep 9 2017, 3:46 PM

@Reedy it's not running from a file. Doing a GitHub search results in php is executing it based on what function is used.

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

which leads to https://github.com/wikimedia/mediawiki/blob/11cf01dd9a8512ad4d9bded43cf22ebd38af8818/includes/installer/PostgresUpdater.php#L782

saper subscribed.Sep 9 2017, 3:49 PM

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

Paladox added a comment.Sep 9 2017, 3:54 PM

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
In T175439#3593835, @Paladox wrote:

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

In T175439#3593827, @saper wrote:

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 );
		}
	}
gerritbot subscribed.Sep 9 2017, 4:12 PM

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

gerritbot added a project: Patch-For-Review.Sep 9 2017, 4:12 PM
saper added a comment.Sep 9 2017, 4:15 PM

Thanks @Reedy!

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

gerritbot added a comment.Sep 12 2017, 11:46 PM

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

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

gerritbot added a comment.Sep 12 2017, 11:46 PM

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

gerritbot added a comment.Sep 12 2017, 11:46 PM

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

gerritbot added a comment.Sep 12 2017, 11:47 PM

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

gerritbot added a comment.Sep 13 2017, 4:57 PM

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

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

ReleaseTaggerBot added a project: MW-1.29-release-notes.Sep 13 2017, 5:00 PM
gerritbot added a comment.Sep 13 2017, 5:01 PM

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

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

gerritbot added a comment.Sep 13 2017, 5:05 PM

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

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

ReleaseTaggerBot added projects: MW-1.27-release-notes, MW-1.28-release-notes.Sep 13 2017, 6:01 PM
Reedy closed this task as Resolved.Sep 13 2017, 7:59 PM
Reedy claimed this task.
Reedy triaged this task as Medium priority.
Aklapper removed a subscriber: Anomie.Oct 16 2020, 5:42 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits