r88929 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r88928‎ | r88929 | r88930 >
Date:20:34, 26 May 2011
Author:mah
Status:reverted (Comments)
Tags:
Comment:
Commiting here, because other problems exist on trunk. This needs to
be forward-ported to trunk.

* Make Pg installation work for lesser privileged role in Postgres (i.e. not super user, but can create users and databases) for Bug #28845.
* Switch to Pg's new “role” tables to replace the old “user” nes
* Give DatabaseInstaller::openConnection() the ability to select a db since there isn't any working selectDB method for Pg yet.
* If the installing role is the same as the one that the wiki will use, make sure it can see the tables in the MW schema
* Remove addition of user_hidden field to the mwtable in Pg installation since it isn't referred to anywhere and breaks the installation.
* Remove the word “below” from the config-connection-error message since sometimes the message is displayed where there is no login information shown at the same time.
Modified paths:
  • /branches/REL1_17/phase3/includes/installer/DatabaseInstaller.php (modified) (history)
  • /branches/REL1_17/phase3/includes/installer/Installer.i18n.php (modified) (history)
  • /branches/REL1_17/phase3/includes/installer/MysqlInstaller.php (modified) (history)
  • /branches/REL1_17/phase3/includes/installer/OracleInstaller.php (modified) (history)
  • /branches/REL1_17/phase3/includes/installer/PostgresInstaller.php (modified) (history)
  • /branches/REL1_17/phase3/includes/installer/PostgresUpdater.php (modified) (history)
  • /branches/REL1_17/phase3/includes/installer/SqliteInstaller.php (modified) (history)

Diff [purge]

Index: branches/REL1_17/phase3/includes/installer/PostgresUpdater.php
——@@ -79,7 +79,6 @@
8080 array( 'addPgField', 'logging', 'log_id', "INTEGER NOT NULL PRIMARY KEY DEFAULT nextval('logging_log_id_seq')" ),
8181 array( 'addPgField', 'logging', 'log_params', 'TEXT' ),
8282 array( 'addPgField', 'mwuser', 'user_editcount', 'INTEGER' ),
83 - array( 'addPgField', 'mwuser', 'user_hidden', 'SMALLINT NOT NULL DEFAULT 0' ),
8483 array( 'addPgField', 'mwuser', 'user_newpass_time', 'TIMESTAMPTZ' ),
8584 array( 'addPgField', 'oldimage', 'oi_deleted', 'SMALLINT NOT NULL DEFAULT 0' ),
8685 array( 'addPgField', 'oldimage', 'oi_major_mime', "TEXT NOT NULL DEFAULT 'unknown'" ),
Index: branches/REL1_17/phase3/includes/installer/Installer.i18n.php
——@@ -252,7 +252,7 @@
253253 Use only ASCII letters (a-z, A-Z), numbers (0-9), underscores (_) and hyphens (-).',
254254 'config-connection-error' => '$1.
255255
256 -Check the host, username and password below and try again.',
 256+Check the host, username and password and try again.',
257257 'config-invalid-schema' => 'Invalid schema for MediaWiki "$1".
258258 Use only ASCII letters (a-z, A-Z), numbers (0-9) and underscores (_).',
259259 'config-db-sys-create-oracle' => 'Installer only supports using a SYSDBA account for creating a new account.',
Index: branches/REL1_17/phase3/includes/installer/SqliteInstaller.php
——@@ -103,7 +103,7 @@
104104 return Status::newGood();
105105 }
106106
107 - public function openConnection() {
 107+ public function openConnection( $dbName = null ) {
108108 global $wgSQLiteDataDir;
109109
110110 $status = Status::newGood();
Index: branches/REL1_17/phase3/includes/installer/DatabaseInstaller.php
——@@ -102,7 +102,7 @@
103103 *
104104 * @return Status
105105 */
106 - public abstract function openConnection();
 106+ public abstract function openConnection( $dbName = null );
107107
108108 /**
109109 * Create the database and return a Status object indicating success or
——@@ -121,11 +121,14 @@
122122 *
123123 * @return Status
124124 */
125 - public function getConnection() {
126 - if ( $this->db ) {
 125+ public function getConnection( $dbName = null ) {
 126+ if ( isset($this->db) && $this->db ) { /* Weirdly get E_STRICT
 127+ * errors without the
 128+ * isset */
127129 return Status::newGood( $this->db );
128130 }
129 - $status = $this->openConnection();
 131+
 132+ $status = $this->openConnection( $dbName );
130133 if ( $status->isOK() ) {
131134 $this->db = $status->value;
132135 // Enable autocommit
Index: branches/REL1_17/phase3/includes/installer/MysqlInstaller.php
——@@ -111,7 +111,7 @@
112112 return $status;
113113 }
114114
115 - public function openConnection() {
 115+ public function openConnection( $dbName = null ) {
116116 $status = Status::newGood();
117117 try {
118118 $db = new DatabaseMysql(
Index: branches/REL1_17/phase3/includes/installer/OracleInstaller.php
——@@ -127,7 +127,7 @@
128128 return $status;
129129 }
130130
131 - public function openConnection() {
 131+ public function openConnection( $dbName = null ) {
132132 $status = Status::newGood();
133133 try {
134134 $db = new DatabaseOracle(
Index: branches/REL1_17/phase3/includes/installer/PostgresInstaller.php
——@@ -101,22 +101,31 @@
102102 return $status;
103103 }
104104
105 - public function openConnection() {
 105+ public function openConnection( $dbName = null ) {
106106 $status = Status::newGood();
107107 try {
108108 if ( $this->useAdmin ) {
 109+ if ( $dbName === null ) $dbName = 'postgres';
 110+
109111 $db = new DatabasePostgres(
110112 $this->getVar( 'wgDBserver' ),
111113 $this->getVar( '_InstallUser' ),
112114 $this->getVar( '_InstallPassword' ),
113 - 'postgres' );
 115+ $dbName );
114116 } else {
 117+ if ( $dbName === null ) $dbName = $this->getVar( 'wgDBname' );
 118+
115119 $db = new DatabasePostgres(
116120 $this->getVar( 'wgDBserver' ),
117121 $this->getVar( 'wgDBuser' ),
118122 $this->getVar( 'wgDBpassword' ),
119 - $this->getVar( 'wgDBname' ) );
 123+ $dbName );
120124 }
 125+
 126+ if( $db === null ) throw new DBConnectionError("Unknown problem while connecting.");
 127+ $safeschema = $db->addIdentifierQuotes( $this->getVar( 'wgDBmwschema' ) );
 128+ if( $db->schemaExists( $this->getVar( 'wgDBmwschema' ) ) ) $db->query( "SET search_path = $safeschema" );
 129+
121130 $status->value = $db;
122131 } catch ( DBConnectionError $e ) {
123132 $status->fatal( 'config-connection-error', $e->getMessage() );
——@@ -134,15 +143,15 @@
135144
136145 $superuser = $this->getVar( '_InstallUser' );
137146
138 - $rights = $conn->selectField( 'pg_catalog.pg_user',
139 - 'CASE WHEN usesuper IS TRUE THEN
140 - CASE WHEN usecreatedb IS TRUE THEN 3 ELSE 1 END
141 - ELSE CASE WHEN usecreatedb IS TRUE THEN 2 ELSE 0 END
142 - END AS rights',
143 - array( 'usename' => $superuser ), __METHOD__
 147+ $rights = $conn->selectField( 'pg_catalog.pg_roles',
 148+ 'CASE WHEN rolsuper then 1
 149+ WHEN rolcreatedb then 2
 150+ ELSE 3
 151+ END as rights',
 152+ array( 'rolname' => $superuser ), __METHOD__
144153 );
145154
146 - if( !$rights || ( $rights != 1 && $rights != 3 ) ) {
 155+ if( !$rights || $rights == 3 ) {
147156 return false;
148157 }
149158
——@@ -226,11 +235,12 @@
227236 $rows = $conn->numRows( $conn->query( $SQL ) );
228237 $safedb = $conn->addIdentifierQuotes( $dbName );
229238 if( !$rows ) {
230 - $conn->query( "CREATE DATABASE $safedb OWNER $safeuser", __METHOD__ );
 239+ $conn->query( "CREATE DATABASE $safedb", __METHOD__ );
 240+ $conn->query( "GRANT ALL ON DATABASE $safedb to $safeuser", __METHOD__ );
231241 } else {
232 - $conn->query( "ALTER DATABASE $safedb OWNER TO $safeuser", __METHOD__ );
 242+ $conn->query( "GRANT ALL ON DATABASE $safedb TO $safeuser", __METHOD__ );
233243 }
234 -
 244+
235245 // Now that we've established the real database exists, connect to it
236246 // Because we do not want the same connection, forcibly expire the existing conn
237247 $this->db = null;
——@@ -287,17 +297,18 @@
288298 $safeschema = $this->db->addIdentifierQuotes( $schema );
289299
290300 $rows = $this->db->numRows(
291 - $this->db->query( "SELECT 1 FROM pg_catalog.pg_shadow WHERE usename = $safeusercheck" )
 301+ $this->db->query( "SELECT 1 FROM pg_catalog.pg_roles WHERE rolname = $safeusercheck" )
292302 );
293303 if ( $rows < 1 ) {
294 - $res = $this->db->query( "CREATE USER $safeuser NOCREATEDB PASSWORD $safepass", __METHOD__ );
 304+ $res = $this->db->query( "CREATE ROLE $safeuser NOCREATEDB LOGIN PASSWORD $safepass", __METHOD__ );
295305 if ( $res !== true && !( $res instanceOf ResultWrapper ) ) {
296306 $status->fatal( 'config-install-user-failed', $this->getVar( 'wgDBuser' ), $res );
297307 }
298308 if( $status->isOK() ) {
299 - $this->db->query("ALTER USER $safeuser SET search_path = $safeschema");
 309+ $this->db->query("ALTER ROLE $safeuser LOGIN");
300310 }
301311 }
 312+ $this->db->query("ALTER ROLE $safeuser SET search_path = $safeschema, public");
302313
303314 return $status;
304315 }
——@@ -337,12 +348,11 @@
338349
339350 $this->db->begin( __METHOD__ );
340351
 352+ // getConnection() should have already selected the schema if it exists
341353 if( !$this->db->schemaExists( $schema ) ) {
342354 $status->error( 'config-install-pg-schema-not-exist' );
343355 return $status;
344356 }
345 - $safeschema = $this->db->addIdentifierQuotes( $schema );
346 - $this->db->query( "SET search_path = $safeschema" );
347357 $error = $this->db->sourceFile( $this->db->getSchema() );
348358 if( $error !== true ) {
349359 $this->db->reportQueryError( $error, 0, '', __METHOD__ );
——@@ -359,12 +369,18 @@
360370 }
361371
362372 public function setupPLpgSQL() {
 373+ $this->db = null;
363374 $this->useAdmin = true;
364 - $status = $this->getConnection();
 375+ $dbName = $this->getVar( 'wgDBname' );
 376+ $status = $this->getConnection( $dbName );
365377 if ( !$status->isOK() ) {
366378 return $status;
367379 }
 380+ $this->db = $status->value;
368381
 382+ /* Admin user has to be connected to the db it just
 383+ created to satisfy ownership requirements for
 384+ "CREATE LANGAUGE" */
369385 $rows = $this->db->numRows(
370386 $this->db->query( "SELECT 1 FROM pg_catalog.pg_language WHERE lanname = 'plpgsql'" )
371387 );
——@@ -373,7 +389,6 @@
374390 $SQL = "SELECT 1 FROM pg_catalog.pg_class c JOIN pg_catalog.pg_namespace n ON (n.oid = c.relnamespace) ".
375391 "WHERE relname = 'pg_pltemplate' AND nspname='pg_catalog'";
376392 $rows = $this->db->numRows( $this->db->query( $SQL ) );
377 - $dbName = $this->getVar( 'wgDBname' );
378393 if ( $rows >= 1 ) {
379394 $result = $this->db->query( 'CREATE LANGUAGE plpgsql' );
380395 if ( !$result ) {

Follow-up revisions

RevisionCommit summaryAuthorDate
r89389forward port r88929mah03:41, 3 June 2011
r89807revert r88929mah22:30, 9 June 2011
r89821PostgreSQL install fixes:...tstarling11:32, 10 June 2011
r89822Revert r89807, i.e. reapply r88929, since it has some useful changes and is t...tstarling11:55, 10 June 2011

Comments

#Comment by Tim Starling (talk | contribs)   03:46, 8 June 2011

In future, please add a RELEASE-NOTES entry when you fix a bug in a release branch.

#Comment by Tim Starling (talk | contribs)   04:24, 8 June 2011

There are some style problems here: single line if statements, comment style, attempted vertical alignment in DatabaseInstaller::getConnection(). 

+			if( $db === null ) throw new DBConnectionError("Unknown problem while connecting.");

How could this possibly happen? You just unconditionally assigned $db to an object. 

-		$rights = $conn->selectField( 'pg_catalog.pg_user',
-			'CASE WHEN usesuper IS TRUE THEN
-				CASE WHEN usecreatedb IS TRUE THEN 3 ELSE 1 END
-				ELSE CASE WHEN usecreatedb IS TRUE THEN 2 ELSE 0 END
-				END AS rights',
-			array( 'usename' => $superuser ), __METHOD__
+		$rights = $conn->selectField( 'pg_catalog.pg_roles',
+			'CASE WHEN rolsuper then 1
+				  WHEN rolcreatedb then 2
+				  ELSE 3
+			 END as rights',
+			array( 'rolname' => $superuser ), __METHOD__
 		);

By criticising this query on bug 28845 I was trying to hint that it would be better to get rid of the CASE and the integer literals altogether, and to put the logic in PHP instead. 

-	public function getConnection() {
+	public function getConnection( $dbName = null ) {

You broke the caching in getConnection(). Now if someone calls getConnection() with $dbName non-null, it will pollute $this->db and cause subsequent calls with $dbName=null to return a connection to the wrong database. It was already broken by r81440 (which was backported), but now it's broken even more. See the way this was dealt with in Oracle in r81084 and r83017.

Also there is the problem that the $dbName parameter is undocumented and is ignored for most of the DBMSes. 

-				$this->db->query("ALTER USER $safeuser SET search_path = $safeschema");
+				$this->db->query("ALTER ROLE $safeuser LOGIN");
 			}
 		}
+		$this->db->query("ALTER ROLE $safeuser SET search_path = $safeschema, public");

By making the "ALTER ROLE SET search_path" unconditional, instead of only being done for new users, this will have the effect of destroying any existing account, breaking any wiki that uses it other than the one that is being installed. It's probably better to let the new wiki be broken rather than break unrelated wikis and apps. Is there a bug report for the issue that this is trying to fix?

Status & tagging log