r80861 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r80860‎ | r80861 | r80862 >
Date:15:30, 24 January 2011
Author:reedy
Status:resolved (Comments)
Tags:
Comment:
Fixup usage of raw sql in Title

Followup r80856, add missing __METHOD__
Modified paths:
  • /trunk/phase3/includes/Title.php (modified) (history)
  • /trunk/phase3/includes/search/SearchMySQL.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/search/SearchMySQL.php
——@@ -284,7 +284,9 @@
285285 $match,
286286 $this->queryRedirect(),
287287 $this->queryNamespaces()
288 - ) );
 288+ ),
 289+ __METHOD__
 290+ );
289291 }
290292
291293 /**
Index: trunk/phase3/includes/Title.php
——@@ -3519,15 +3519,15 @@
35203520
35213521 $titlekey = $this->getArticleId();
35223522 $dbr = wfGetDB( DB_SLAVE );
3523 - $categorylinks = $dbr->tableName( 'categorylinks' );
35243523
3525 - # NEW SQL
3526 - $sql = "SELECT * FROM $categorylinks"
3527 - . " WHERE cl_from='$titlekey'"
3528 - . " AND cl_from <> '0'"
3529 - . " ORDER BY cl_sortkey";
3530 -
3531 - $res = $dbr->query( $sql );
 3524+ $res = $dbr->select( 'categorylinks', '*',
 3525+ array(
 3526+ 'cl_from' => $titleKey,
 3527+ "cl_from <> '0'",
 3528+ ),
 3529+ __METHOD__,
 3530+ array( 'ORDER BY' => 'cl_sortkey' )
 3531+ );
35323532 $data = array();
35333533
35343534 if ( $dbr->numRows( $res ) > 0 ) {

Follow-up revisions

RevisionCommit summaryAuthorDate
r81049Followup fixme on r80861,...reedy20:03, 26 January 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r80856Start another attack on raw sql queriesreedy13:59, 24 January 2011

Comments

#Comment by Brion VIBBER (talk | contribs)   20:23, 25 January 2011

The query in Title::getParentCategories() looks pretty...... bad. It's been about the same since r4913, where it replaced an older query that also joined to the then 'cur' table. :)

As currently written it comes out to about: 

mysql> explain select * from categorylinks where cl_from=1 and cl_from <> '0' order by cl_sortkey;
+----+-------------+---------------+------+---------------+---------+---------+-------+------+-----------------------------+
| id | select_type | table         | type | possible_keys | key     | key_len | ref   | rows | Extra                       |
+----+-------------+---------------+------+---------------+---------+---------+-------+------+-----------------------------+
|  1 | SIMPLE      | categorylinks | ref  | cl_from       | cl_from | 4       | const |    1 | Using where; Using filesort |
+----+-------------+---------------+------+---------------+---------+---------+-------+------+-----------------------------+

So big problems that should be cleaned up:

  1. cl_from <> '0' clause should be removed and replaced with a check that the article id is non-zero before doing a query.
  2. order by cl_sortkey is very wrong: it doesn't produce useful sorting results in this query, and it triggers filesort which can be potentially slow
#Comment by Reedy (talk | contribs)   20:27, 25 January 2011

Yay for shitty old code.

#Comment by Reedy (talk | contribs)   20:27, 25 January 2011

Can I assign the FIXME to hashar? =D

Status & tagging log