r102785 MediaWiki - Code Review archive

Repository:	MediaWiki
Revision:	< r102784‎ \| r102785 \| r102786 >
Date:	14:28, 11 November 2011
Author:	freakolowsky
Status:	resolved (Comments)
Tags:
Comment:	* applyed the patch from bug 26393 (please comment on the bug page)
Modified paths:	/trunk/phase3/includes/installer/MysqlUpdater.php (modified) (history) /trunk/phase3/includes/installer/OracleUpdater.php (modified) (history) /trunk/phase3/includes/installer/SqliteUpdater.php (modified) (history) /trunk/phase3/includes/specials/SpecialShortpages.php (modified) (history) /trunk/phase3/maintenance/archives/patch-page_redirect_namespace_len.sql (added) (history) /trunk/phase3/maintenance/oracle/archives/patch-page_redirect_namespace_len.sql (added) (history) /trunk/phase3/maintenance/oracle/tables.sql (modified) (history) /trunk/phase3/maintenance/sqlite/archives/patch-page_redirect_namespace_len.sql (added) (history) /trunk/phase3/maintenance/tables.sql (modified) (history)

Diff [purge]

Index: trunk/phase3/maintenance/archives/patch-page_redirect_namespace_len.sql
—	—	@@ -0,0 +1,6 @@
	2	+--
	3	+-- Add the page_redirect_namespace_len index
	4	+--
	5	+
	6	+CREATE INDEX /i/page_redirect_namespace_len ON /_/page (page_is_redirect, page_namespace, page_len);
	7	+
Property changes on: trunk/phase3/maintenance/archives/patch-page_redirect_namespace_len.sql
___________________________________________________________________
Added: svn:eol-style
1	8	+ native
Index: trunk/phase3/maintenance/sqlite/archives/patch-page_redirect_namespace_len.sql
—	—	@@ -0,0 +1,7 @@
	2	+--
	3	+-- Add the page_redirect_namespace_len index
	4	+--
	5	+
	6	+CREATE INDEX /i/page_redirect_namespace_len ON /_/page (page_is_redirect, page_namespace, page_len);
	7	+
	8	+
Property changes on: trunk/phase3/maintenance/sqlite/archives/patch-page_redirect_namespace_len.sql
___________________________________________________________________
Added: svn:eol-style
1	9	+ native
Index: trunk/phase3/maintenance/oracle/archives/patch-page_redirect_namespace_len.sql
—	—	@@ -0,0 +1,4 @@
	2	+define mw_prefix='{$wgDBprefix}';
	3	+
	4	+CREATE INDEX &mw_prefix.page_i03 ON &mw_prefix.page (page_is_redirect, page_namespace, page_len);
	5	+
Property changes on: trunk/phase3/maintenance/oracle/archives/patch-page_redirect_namespace_len.sql
___________________________________________________________________
Added: svn:eol-style
1	6	+ native
Index: trunk/phase3/maintenance/oracle/tables.sql
—	—	@@ -79,6 +79,7 @@
80	80	CREATE UNIQUE INDEX &mw_prefix.page_u01 ON &mw_prefix.page (page_namespace,page_title);
81	81	CREATE INDEX &mw_prefix.page_i01 ON &mw_prefix.page (page_random);
82	82	CREATE INDEX &mw_prefix.page_i02 ON &mw_prefix.page (page_len);
	83	+CREATE INDEX &mw_prefix.page_i03 ON &mw_prefix.page (page_is_redirect, page_namespace, page_len);
83	84
84	85	-- Create a dummy page to satisfy fk contraints especially with revisions
85	86	INSERT INTO &mw_prefix.page
Index: trunk/phase3/maintenance/tables.sql
—	—	@@ -265,8 +265,8 @@
266	266	CREATE UNIQUE INDEX /i/name_title ON /_/page (page_namespace,page_title);
267	267	CREATE INDEX /i/page_random ON /_/page (page_random);
268	268	CREATE INDEX /i/page_len ON /_/page (page_len);
	269	+CREATE INDEX /i/page_len_redirect_namespace ON /_/page (page_len, page_is_redirect, page_namespace);
269	270
270		-
271	271	--
272	272	-- Every edit of a page creates also a revision row.
273	273	-- This stores metadata about the revision, and a reference
Index: trunk/phase3/includes/installer/MysqlUpdater.php
—	—	@@ -189,7 +189,8 @@
190	190	array( 'doMigrateUserOptions' ),
191	191	array( 'dropField', 'user', 'user_options', 'patch-drop-user_options.sql' ),
192	192	array( 'addField', 'revision', 'rev_sha1', 'patch-rev_sha1.sql' ),
193		~~- array( 'addField', 'archive', 'ar_sha1', 'patch-ar_sha1.sql' )~~
	193	+ array( 'addField', 'archive', 'ar_sha1', 'patch-ar_sha1.sql' ),
	194	+ array( 'addIndex', 'page', 'page_redirect_namespace_len', 'patch-page_redirect_namespace_len.sql' )
194	195	);
195	196	}
196	197
Index: trunk/phase3/includes/installer/OracleUpdater.php
—	—	@@ -47,6 +47,7 @@
48	48	array( 'addField', 'revision', 'rev_sha1', 'patch-rev_sha1_field.sql' ),
49	49	array( 'addField', 'archive', 'ar_sha1', 'patch-ar_sha1_field.sql' ),
50	50	array( 'doRemoveNotNullEmptyDefaults2' ),
	51	+ array( 'addIndex', 'page', 'i03', 'patch-page_redirect_namespace_len.sql' ),
51	52
52	53	// till 2.0 i guess
53	54	array( 'doRebuildDuplicateFunction' ),
Index: trunk/phase3/includes/installer/SqliteUpdater.php
—	—	@@ -67,7 +67,9 @@
68	68	array( 'doMigrateUserOptions' ),
69	69	array( 'dropField', 'user', 'user_options', 'patch-drop-user_options.sql' ),
70	70	array( 'addField', 'revision', 'rev_sha1', 'patch-rev_sha1.sql' ),
71		~~- array( 'addField', 'archive', 'ar_sha1', 'patch-ar_sha1.sql' )~~
	71	+ array( 'addField', 'archive', 'ar_sha1', 'patch-ar_sha1.sql' ),
	72	+ array( 'addIndex', 'page', 'page_redirect_namespace_len', 'patch-page_redirect_namespace_len.sql' )
	73	+
72	74	);
73	75	}
74	76
Index: trunk/phase3/includes/specials/SpecialShortpages.php
—	—	@@ -33,14 +33,6 @@
34	34	parent::__construct( $name );
35	35	}
36	36
37		~~- // inexpensive?~~
38		- /**
39		~~- * This query is indexed as of 1.5~~
40		~~- */~~
41		~~- function isExpensive() {~~
42		~~- return true;~~
43		~~- }~~
44		-
45	37	function isSyndicated() {
46	38	return false;
47	39	}
—	—	@@ -51,9 +43,9 @@
52	44	'fields' => array ( 'page_namespace AS namespace',
53	45	'page_title AS title',
54	46	'page_len AS value' ),
55		~~- 'conds' => array ( 'page_namespace' => MWNamespace::getContentNamespaces(),~~
	47	+ 'conds' => array ( 'page_namespace' => NS_MAIN,
56	48	'page_is_redirect' => 0 ),
57		~~- 'options' => array ( 'USE INDEX' => 'page_len' )~~
	49	+ 'options' => array ( 'USE INDEX' => 'page_redirect_namespaces_len' )
58	50	);
59	51	}
60	52

Follow-up revisions

Revision	Commit summary	Author	Date
r102798	Followup r102785, fix wrong index order in tables.sql. The patches are fine	catrope	16:37, 11 November 2011
r103956	Followup r102785: fix typo in index name. Thanks to Platonides and his specia...	catrope	20:51, 22 November 2011

Comments

#Comment by Bawolff (talk | contribs) 15:42, 11 November 2011

I think some tests need to be updated

DatabaseSqliteTest::testUpgrades
mismatching indexes for table "page" upgrading from 1.15 to 1.19alpha
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array
 (
     [0] => name_title
     [1] => page_len
-    [2] => page_len_redirect_namespace
-    [3] => page_random
+    [2] => page_random
+    [3] => page_redirect_namespace_len
 )

/var/lib/jenkins/jobs/MediaWiki-phpunit/workspace/mw-core/tests/phpunit/includes/db/DatabaseSqliteTest.php:245
/var/lib/jenkins/jobs/MediaWiki-phpunit/workspace/mw-core/tests/phpunit/MediaWikiTestCase.php:68
/var/lib/jenkins/jobs/MediaWiki-phpunit/workspace/mw-core/tests/phpunit/MediaWikiPHPUnitCommand.php:44
/var/lib/jenkins/jobs/MediaWiki-phpunit/workspace/mw-core/tests/phpunit/phpunit.php:60

#Comment by Brion VIBBER (talk | contribs) 22:27, 22 November 2011

I don't see this failing anymore as of r103942:

$ php phpunit.php includes/db/DatabaseSqliteTest.php 
PHPUnit 3.5.13 by Sebastian Bergmann.

.......

Time: 3 seconds, Memory: 36.25Mb

OK (7 tests, 4489 assertions)

Looks like r102798 fixed it there.

#Comment by MaxSem (talk | contribs) 16:39, 11 November 2011

Note: if SQLite patch is the same as MySQL one, you don't need it.

#Comment by Catrope (talk | contribs) 09:11, 12 November 2011

Heh, right. Not sure why I did that.

#Comment by P858snake (talk | contribs) 11:48, 12 November 2011

In future when committing other people's patches, Please include the patchers name in the commit summary at the end.

#Comment by Platonides (talk | contribs) 22:42, 21 November 2011

The index you added is named 'page_redirect_namespace_len', not page_redirect_namespaces_len

+			'options' => array ( 'USE INDEX' => 'page_redirect_namespaces_len' )

Detected by QueryAllSpecialPagesTest.php

#Comment by Catrope (talk | contribs) 20:49, 22 November 2011

Grr, fixing. Thanks for pointing that out.

#Comment by Brion VIBBER (talk | contribs) 22:25, 22 November 2011

this is fixed by r103956

#Comment by Brion VIBBER (talk | contribs) 22:34, 22 November 2011

With the above things fixed, looking good!

Queries appear to actually be nicely const-indexed:

mysql> explain SELECT /* ShortPagesPage::reallyDoQuery 192.168.38.113 */ page_namespace AS namespace,page_title AS title,page_len AS value FROM `page` FORCE INDEX (page_redirect_namespace_len) WHERE page_namespace = '0' AND page_is_redirect = '0' ORDER BY page_len LIMIT 50;

id	select_type	table	type	possible_keys	key	key_len	ref	rows	Extra
1	SIMPLE	page	ref	page_redirect_namespace_len	page_redirect_namespace_len	5	const,const	71	Using where

1 row in set (0.00 sec)

mysql> explain SELECT /* LongPagesPage::reallyDoQuery 192.168.38.113 */ page_namespace AS namespace,page_title AS title,page_len AS value FROM `page` FORCE INDEX (page_redirect_namespace_len) WHERE page_namespace = '0' AND page_is_redirect = '0' ORDER BY page_len DESC LIMIT 50 ;

id	select_type	table	type	possible_keys	key	key_len	ref	rows	Extra
1	SIMPLE	page	ref	page_redirect_namespace_len	page_redirect_namespace_len	5	const,const	71	Using where

1 row in set (0.00 sec)

Status & tagging log

22:34, 22 November 2011 Brion VIBBER (talk | contribs) changed the status of r102785 [removed: fixme added: resolved]
22:42, 21 November 2011 Platonides (talk | contribs) changed the status of r102785 [removed: new added: fixme]
09:11, 12 November 2011 Catrope (talk | contribs) changed the status of r102785 [removed: fixme added: new]
15:42, 11 November 2011 Bawolff (talk | contribs) changed the status of r102785 [removed: new added: fixme]