r83812 MediaWiki - Code Review archive

Marking fixme, patch doesn't allow table prefixed ;-)

Sqlite does not support enums. DatabaseSqlite::replaceVars() replaces ENUM types with TEXT types, so it should not need a patch.

maintenance/update.php just failed on my empty trunk test wiki:

...ar_page_revid key doesn't exist. ...ar_revid key already set on archive table. ...ll_lang is up-to-date. Changing cl_type from enum to varchar...A database query syntax error has occurred. The last attempted database query was: "ALTER TABLE `categorylinks` MODIFY cl_type varchar(6) NOT NULL default 'page' " from within function "DatabaseBase::sourceFile( /home/wiki/root/rcw/maintenance/archives/patch-cl_type.sql )". Database returned error "1071: Specified key was too long; max key length is 1000 bytes (localhost)"

Are you running with $wgMysql5 = true?

Yes, of course I do.

It's possible to

ALTER TABLE /*_*/categorylinks MODIFY cl_type INT NOT NULL default 1;

Running this on plwiki_p categorylinks table (2237191 rows) takes 3 minutes on the toolserver:

Job 244779 (altercltypes) Complete
 User             = saper
 Queue            = all.q@yarrow.toolserver.org
 Host             = yarrow.toolserver.org
 Start Time       = 03/14/2011 03:16:56
 End Time         = 03/14/2011 03:19:22
 User Time        = 00:00:00
 System Time      = 00:00:00
 Wallclock Time   = 00:02:26
 CPU              = 00:00:00
 Max vmem         = 10.840M
 Exit Status      = 0

You actually get values 1 for page, 2 frr subcat, 3 for file.

Before conversion (a copy of current plwiki_p on the toolserver):

mysql> select cl_type,count(cl_type) from plwiki_p.categorylinks group by cl_type;
+---------+----------------+
| cl_type | count(cl_type) |
+---------+----------------+
| page    |        2099095 |
| subcat  |         137672 |
| file    |            424 |
+---------+----------------+
3 rows in set (1 min 4.96 sec)

After conversion:

mysql> select cl_type,count(cl_type) from categorylinks group by cl_type;
+---------+----------------+
| cl_type | count(cl_type) |
+---------+----------------+
|       1 |        2099095 |
|       2 |         137672 |
|       3 |            424 |
+---------+----------------+
3 rows in set (1.10 sec)

Adding another table (clt_desc?) where cl_type a foreign key to the clt_id would be beneficial, so that code still uses ('page', 'subcat', 'file')

Does varbinary(6) work?

Would it be possible to solve the API paging problem with the existing schema, by splitting the query up into several queries, one for each type? ApiQueryGeneratorBase::select() is short and simple, it could be overridden in ApiQueryCategoryMembers or just not called. Instead of

				$this->addWhere( "cl_type $op $escType OR " .
					"(cl_type = $escType AND " .

you would leave the cl_type out of the condition initially, and have something like:

$allTypes = array( 'page', 'file', 'subcat' );
$contTypeIndex = array_search( $cont[0], $allTypes );
$queryTypes = array_slice( $allTypes, $contTypeIndex );

And then instead of $this->select():

foreach ( $queryTypes as $type ) {
    $where = array_merge( $this->where, array( 'cl_type' => $type ) );
    $res = $dbr->select( ... );
    $rows = array_merge( $rows, iterator_to_array( $res ) );
    if ( count( $rows ) >= $limit ) {
        break;
    }
}

Changing the schema is time-consuming and disruptive, and in the light of a recent feature requirements discussion on IRC, it's not really clear to me whether this schema is sufficient to meet future demands. Also, we don't know yet what the size distribution of ICU sort keys will be, and whether 230 bytes will be enough. A stopgap measure like this would give us some time to review and design.

That'd certainly be possible, yes. I'll implement it.

Done in r84392. Your example code was very reusable (I copypasted it and had to change relatively little to make it work), thanks for that.

Do we need to create something to change the varchar back to an enum, as it'll possibly affect development wikis, and possibly places like TW.

Some sort of transient check based on type to force it back again?

There's no compelling reason to change it back, other than maybe index length overruns if cl_sortkey is enlarged in the future.