r83410 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r83409‎ | r83410 | r83411 >
Date:22:03, 6 March 2011
Author:btongminh
Status:ok (Comments)
Tags:
Comment:
Breaking change: remove faminsize/famaxsize because they lead to an unindexed query, which takes a very long time if there are only few rows that match the filter. Introduced in r66168, so luckily only live since 1.17.
Modified paths:
  • /trunk/phase3/includes/api/ApiQueryFilearchive.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/api/ApiQueryFilearchive.php
@@ -95,14 +95,6 @@
9696 if ( isset( $params['prefix'] ) )
9797 $this->addWhere( 'fa_name' . $db->buildLike( $this->titlePartToKey( $params['prefix'] ), $db->anyString() ) );
9898
99 - if ( isset( $params['minsize'] ) ) {
100 - $this->addWhere( 'fa_size>=' . intval( $params['minsize'] ) );
101 - }
102 -
103 - if ( isset( $params['maxsize'] ) ) {
104 - $this->addWhere( 'fa_size<=' . intval( $params['maxsize'] ) );
105 - }
106 -
10799 $sha1 = false;
108100 if ( isset( $params['sha1'] ) ) {
109101 $sha1 = wfBaseConvert( $params['sha1'], 16, 36, 31 );
@@ -180,12 +172,6 @@
181173 return array (
182174 'from' => null,
183175 'prefix' => null,
184 - 'minsize' => array(
185 - ApiBase::PARAM_TYPE => 'integer',
186 - ),
187 - 'maxsize' => array(
188 - ApiBase::PARAM_TYPE => 'integer',
189 - ),
190176 'limit' => array(
191177 ApiBase::PARAM_DFLT => 10,
192178 ApiBase::PARAM_TYPE => 'limit',
@@ -225,8 +211,6 @@
226212 'from' => 'The image title to start enumerating from',
227213 'prefix' => 'Search for all image titles that begin with this value',
228214 'dir' => 'The direction in which to list',
229 - 'minsize' => 'Limit to images with at least this many bytes',
230 - 'maxsize' => 'Limit to images with at most this many bytes',
231215 'limit' => 'How many total images to return',
232216 'sha1' => "SHA1 hash of image. Overrides {$this->getModulePrefix()}sha1base36",
233217 'sha1base36' => 'SHA1 hash of image in base 36 (used in MediaWiki)',

Follow-up revisions

RevisionCommit summaryAuthorDate
r834871.17wmf1: MFT r82696, r83270, r83284, r83374, r83390, r83392, r83402, r83403,...catrope21:44, 7 March 2011
r85354MFT r82518, r82530, r82538, r82547, r82550, r82565, r82572, r82608, r82696, r...demon18:25, 4 April 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r66168* (bug 21346) Make deleted images searchable by hash...reedy22:41, 10 May 2010

Comments

#Comment by Bryan (talk | contribs)   22:06, 6 March 2011

Also note that this module allows both filtering on fa_name and fa_storage_key, which are in separated indices. I'm not sure if index merging is supported by the MySQL that WMF uses.

#Comment by Catrope (talk | contribs)   08:39, 7 March 2011

No, there's no index merging.

Also, this change should be announced as a breaking change on mediawiki-api.

#Comment by Bryan (talk | contribs)   08:41, 7 March 2011

Well, it exists [1] but starting from 5.0.

#Comment by Catrope (talk | contribs)   17:49, 7 March 2011

I'd never heard of that, thanks for enlightening me :)

However, to create a nicely continue-able query you need a single, unique index to work with. For this reason we usually allow filtering by either name or hash but not both (in similar situations in other modules).

#Comment by Bryan (talk | contribs)   17:52, 7 March 2011

If you filter by hash your result set will be very small, so an unindexed filesort would be acceptable imho.

#Comment by Catrope (talk | contribs)   17:54, 7 March 2011

Good point; in this case it'll be fine because one of the fields is a hash.

#Comment by Dantman (talk | contribs)   10:20, 7 March 2011

Wait? This is a only affects large wikis using a MySQL without index merging right? Instead of completely removing it, shouldn't we make disabling this configurable, based on what mysql version is running, or part of miser mode?

#Comment by Bryan (talk | contribs)   10:22, 7 March 2011

This specific query is completely unindexed. It could be made part of $wgMiserMode. However, I don't know if there is any use case of searching for deleted images by size.

#Comment by Dantman (talk | contribs)   10:25, 7 March 2011

Oh, ok, deleted. I guess that's not as important as filtering live images by size. Though if there's index merging available I don't see right now how we can't find a way to index it.

#Comment by Bryan (talk | contribs)   10:27, 7 March 2011

There is nothing to merge, as there is no index on fa_size

#Comment by Dantman (talk | contribs)   11:28, 7 March 2011

Is there a reason we can't create one?

#Comment by Catrope (talk | contribs)   17:50, 7 March 2011

The feature is so obscure that it's not worth creating an index for me IMO

Status & tagging log