r50478 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r50477‎ | r50478 | r50479 >
Date:12:38, 11 May 2009
Author:freakolowsky
Status:resolved (Comments)
Tags:
Comment:
Preparations for Oracle database abstraction update.
Replaced a few hardcoded LIMIT clauses with database function.
Wrapped UNION statement in SpecialRecentchangeslinked for Oracle compatibility.
Modified paths:
  • /trunk/phase3/includes/Article.php (modified) (history)
  • /trunk/phase3/includes/specials/SpecialNewimages.php (modified) (history)
  • /trunk/phase3/includes/specials/SpecialRecentchangeslinked.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/Article.php
——@@ -707,8 +707,8 @@
708708 GROUP BY rev_user, rev_user_text, user_real_name
709709 ORDER BY timestamp DESC";
710710
711 - if($limit > 0) { $sql .= ' LIMIT '.$limit; }
712 - if($offset > 0) { $sql .= ' OFFSET '.$offset; }
 711+ if($limit > 0)
 712+ $sql = $dbr->limitResult($sql, $limit, $offset);
713713
714714 $sql .= ' '. $this->getSelectOptions();
715715
Index: trunk/phase3/includes/specials/SpecialRecentchangeslinked.php
——@@ -155,7 +155,9 @@
156156 $sql = $subsql[0];
157157 else {
158158 // need to resort and relimit after union
159 - $sql = "(" . implode( ") UNION (", $subsql ) . ") ORDER BY rc_timestamp DESC LIMIT {$limit}";
 159+ // unwrapped UNION block will not work in Oracle. Wrapper aded.
 160+ $sql = "SELECT * FROM ((" . implode( ") UNION (", $subsql ) . ")) ORDER BY rc_timestamp DESC";
 161+ $sql = $dbr->limitResult($sql, $limit, false);
160162 }
161163
162164 $res = $dbr->query( $sql, __METHOD__ );
Index: trunk/phase3/includes/specials/SpecialNewimages.php
——@@ -40,7 +40,8 @@
4141 if ($hidebotsql) {
4242 $sql .= "$hidebotsql WHERE ug_group IS NULL";
4343 }
44 - $sql .= ' ORDER BY img_timestamp DESC LIMIT 1';
 44+ $sql .= ' ORDER BY img_timestamp DESC';
 45+ $sql = $dbr->limitResult($sql, 1, false);
4546 $res = $dbr->query( $sql, __FUNCTION__ );
4647 $row = $dbr->fetchRow( $res );
4748 if( $row !== false ) {
——@@ -93,7 +94,7 @@
9495 $sql .= ' WHERE ' . $dbr->makeList( $where, LIST_AND );
9596 }
9697 $sql.=' ORDER BY img_timestamp '. ( $invertSort ? '' : ' DESC' );
97 - $sql.=' LIMIT ' . ( $limit + 1 );
 98+ $sql = $dbr->limitResult($sql, ( $limit + 1 ), false);
9899 $res = $dbr->query( $sql, __FUNCTION__ );
99100
100101 /**

Follow-up revisions

RevisionCommit summaryAuthorDate
r50483Partial revert of r50478 "Preparations for Oracle database abstraction update."...simetrical14:05, 11 May 2009

Comments

#Comment by Simetrical (talk | contribs)   14:06, 11 May 2009

When doing this kind of change, make sure you don't break MySQL! Causing syntax errors in MySQL 4.0 or later is going to get people annoyed, especially if they make it to Wikipedia. This causes a syntax error in all versions of MySQL: 

-			$sql = "(" . implode( ") UNION (", $subsql ) . ") ORDER BY rc_timestamp DESC LIMIT {$limit}";
+			// unwrapped UNION block will not work in Oracle. Wrapper aded.
+			$sql = "SELECT * FROM ((" . implode( ") UNION (", $subsql ) . ")) ORDER BY rc_timestamp DESC";
+			$sql = $dbr->limitResult($sql, $limit, false);

On my desktop I get "1248: Every derived table must have its own alias (localhost)". More importantly, subqueries don't work at all in MySQL 4.0 (which Wikipedia uses) ― you have to simply avoid using them at all in core code. If UNION works differently in Oracle, a wrapper function may be needed. I've reverted this in r50483.

Everything else looks okay at first glance, but note that the third parameter to limitResult() is optional, and it would be nicer to simply omit it rather than passing an explicit "false".

#Comment by Tim Starling (talk | contribs)   10:59, 4 June 2009

Assuming resolved as of r50483.

Status & tagging log