r94370 MediaWiki - Code Review archive

Repository:	MediaWiki
Revision:	< r94369‎ \| r94370 \| r94371 >
Date:	19:11, 12 August 2011
Author:	aaron
Status:	reverted (Comments)
Tags:
Comment:	* Added LoggedUpdateMaintenance subclass * Moved PopulateRevisionLength/PopulateRevisionSha1 scripts to $postDatabaseUpdateMaintenance * Fixed bogus "{$prefix}_sha1 != ''" comparison (r94362) * Removed unneeded NOT NULL check (speeds up script a bit) from populateRevisionSha1 script * Various code cleanups
Modified paths:	/trunk/phase3/includes/AutoLoader.php (modified) (history) /trunk/phase3/includes/installer/DatabaseUpdater.php (modified) (history) /trunk/phase3/includes/installer/MysqlUpdater.php (modified) (history) /trunk/phase3/maintenance/Maintenance.php (modified) (history) /trunk/phase3/maintenance/populateRevisionLength.php (modified) (history) /trunk/phase3/maintenance/populateRevisionSha1.php (modified) (history)

Diff [purge]

Index: trunk/phase3/maintenance/populateRevisionSha1.php
—	—	@@ -1,6 +1,7 @@
2	2	<?php
3	3	/**
4		~~- * Fills the rev_sha1 and ar_sha1 columns of revision & archive tables.~~
	4	+ * Fills the rev_sha1 and ar_sha1 columns of revision
	5	+ * and archive tables for revisions created before MW 1.19.
5	6	*
6	7	* This program is free software; you can redistribute it and/or modify
7	8	* it under the terms of the GNU General Public License as published by
—	—	@@ -22,45 +23,56 @@
23	24
24	25	require_once( dirname( __FILE__ ) . '/Maintenance.php' );
25	26
26		~~-class PopulateRevisionSha1 extends Maintenance {~~
	27	+class PopulateRevisionSha1 extends LoggedUpdateMaintenance {
27	28	public function __construct() {
28	29	parent::__construct();
29	30	$this->mDescription = "Populates the rev_sha1 and ar_sha1 fields";
30	31	$this->setBatchSize( 200 );
31	32	}
32	33
33		~~- public function execute() {~~
	34	+ protected function getUpdateKey() {
	35	+ return 'populate rev_sha1';
	36	+ }
	37	+
	38	+ protected function updateSkippedMessage() {
	39	+ return 'rev_sha1 column of revision table already populated.';
	40	+ }
	41	+
	42	+ protected function updatelogFailedMessage() {
	43	+ return 'Could not insert rev_sha1 population row.';
	44	+ }
	45	+
	46	+ protected function doDBUpdates() {
34	47	$db = $this->getDB( DB_MASTER );
	48	+ if ( !$db->tableExists( 'revision' ) ) {
	49	+ $this->error( "revision table does not exist", true );
	50	+ }
	51	+ if ( !$db->tableExists( 'archive' ) ) {
	52	+ $this->error( "archive table does not exist", true );
	53	+ }
35	54
36	55	$this->output( "Populating rev_sha1 column\n" );
37		~~- $this->doSha1Updates( $db, 'revision', 'rev_id', 'rev' );~~
	56	+ $rc = $this->doSha1Updates( $db, 'revision', 'rev_id', 'rev' );
38	57
39	58	$this->output( "Populating ar_sha1 column\n" );
40		~~- $this->doSha1Updates( $db, 'archive', 'ar_rev_id', 'ar' );~~
	59	+ $ac = $this->doSha1Updates( $db, 'archive', 'ar_rev_id', 'ar' );
41	60
42		~~- if ( $db->insert(~~
43		~~- 'updatelog',~~
44		~~- array( 'ul_key' => 'populate rev_sha1' ),~~
45		~~- __METHOD__,~~
46		~~- 'IGNORE'~~
47		~~- )~~
48		~~- ) {~~
49		~~- $this->output( "rev_sha1 and ar_sha1 population complete.\n" );~~
50		~~- return true;~~
51		~~- } else {~~
52		~~- $this->output( "Could not insert rev_sha1 population row.\n" );~~
53		~~- return false;~~
54		~~- }~~
	61	+ $this->output( "rev_sha1 and ar_sha1 population complete [$rc revision rows, $ac archive rows].\n" );
	62	+ return true;
55	63	}
56	64
	65	+ /**
	66	+ * @return Integer Rows changed
	67	+ */
57	68	protected function doSha1Updates( $db, $table, $idCol, $prefix ) {
58		~~- $start = $db->selectField( $table, "MIN($idCol)", "$idCol IS NOT NULL", __METHOD__ );~~
59		~~- if ( !$start ) {~~
60		~~- $this->output( "Nothing to do.\n" );~~
	69	+ $start = $db->selectField( $table, "MIN($idCol)", false, __METHOD__ );
	70	+ $end = $db->selectField( $table, "MAX($idCol)", false, __METHOD__ );
	71	+ if ( !$start \|\| !$end ) {
	72	+ $this->output( "...revision table seems to be empty.\n" );
61	73	return true;
62	74	}
63		~~- $end = $db->selectField( $table, "MAX($idCol)", "$idCol IS NOT NULL", __METHOD__ );~~
64	75
	76	+ $count = 0;
65	77	# Do remaining chunk
66	78	$end += $this->mBatchSize - 1;
67	79	$blockStart = $start;
—	—	@@ -68,7 +80,7 @@
69	81	while ( $blockEnd <= $end ) {
70	82	$this->output( "...doing $idCol from $blockStart to $blockEnd\n" );
71	83	$cond = "$idCol BETWEEN $blockStart AND $blockEnd
72		~~- AND $idCol IS NOT NULL AND {$prefix}_sha1 != ''";~~
	84	+ AND $idCol IS NOT NULL AND {$prefix}_sha1 = ''";
73	85	$res = $db->select( $table, '*', $cond, __METHOD__ );
74	86
75	87	$db->begin();
—	—	@@ -87,6 +99,7 @@
88	100	array( "{$prefix}_sha1" => Revision::base36Sha1( $text ) ),
89	101	array( $idCol => $row->$idCol ),
90	102	__METHOD__ );
	103	+ $count++;
91	104	}
92	105	}
93	106	$db->commit();
—	—	@@ -95,6 +108,7 @@
96	109	$blockEnd += $this->mBatchSize;
97	110	wfWaitForSlaves();
98	111	}
	112	+ return $count;
99	113	}
100	114	}
101	115
Index: trunk/phase3/maintenance/populateRevisionLength.php
—	—	@@ -22,29 +22,39 @@
23	23
24	24	require_once( dirname( __FILE__ ) . '/Maintenance.php' );
25	25
26		~~-class PopulateRevisionLength extends Maintenance {~~
	26	+class PopulateRevisionLength extends LoggedUpdateMaintenance {
27	27	public function __construct() {
28	28	parent::__construct();
29		~~- $this->mDescription = "Populates rev_len";~~
	29	+ $this->mDescription = "Populates the rev_len field";
30	30	$this->setBatchSize( 200 );
31	31	}
32	32
33		~~- public function execute() {~~
	33	+ protected function getUpdateKey() {
	34	+ return 'populate rev_len';
	35	+ }
	36	+
	37	+ protected function updateSkippedMessage() {
	38	+ return 'rev_len column of revision table already populated.';
	39	+ }
	40	+
	41	+ protected function updatelogFailedMessage() {
	42	+ return 'Could not insert rev_len population row.';
	43	+ }
	44	+
	45	+ public function doDBUpdates() {
34	46	$db = $this->getDB( DB_MASTER );
35	47	if ( !$db->tableExists( 'revision' ) ) {
36	48	$this->error( "revision table does not exist", true );
37	49	}
38	50	$this->output( "Populating rev_len column\n" );
39		~~- $start = $db->selectField( 'revision', 'MIN(rev_id)', false, __FUNCTION__ );~~
40		~~- $end = $db->selectField( 'revision', 'MAX(rev_id)', false, __FUNCTION__ );~~
41		~~- if ( is_null( $start ) \|\| is_null( $end ) ) {~~
	51	+
	52	+ $start = $db->selectField( 'revision', 'MIN(rev_id)', false, __METHOD__ );
	53	+ $end = $db->selectField( 'revision', 'MAX(rev_id)', false, __METHOD__ );
	54	+ if ( !$start \|\| !$end ) {
42	55	$this->output( "...revision table seems to be empty.\n" );
43		~~- $db->insert( 'updatelog',~~
44		~~- array( 'ul_key' => 'populate rev_len' ),~~
45		~~- __METHOD__,~~
46		~~- 'IGNORE' );~~
47		~~- return;~~
	56	+ return true;
48	57	}
	58	+
49	59	# Do remaining chunks
50	60	$blockStart = intval( $start );
51	61	$blockEnd = intval( $start ) + $this->mBatchSize - 1;
—	—	@@ -80,17 +90,9 @@
81	91	$blockEnd += $this->mBatchSize;
82	92	wfWaitForSlaves();
83	93	}
84		~~- $logged = $db->insert( 'updatelog',~~
85		~~- array( 'ul_key' => 'populate rev_len' ),~~
86		~~- __METHOD__,~~
87		~~- 'IGNORE' );~~
88		~~- if ( $logged ) {~~
89		~~- $this->output( "rev_len population complete ... {$count} rows changed ({$missing} missing)\n" );~~
90		~~- return true;~~
91		~~- } else {~~
92		~~- $this->output( "Could not insert rev_len population row.\n" );~~
93		~~- return false;~~
94		~~- }~~
	94	+
	95	+ $this->output( "rev_len population complete ... {$count} rows changed ({$missing} missing)\n" );
	96	+ return true;
95	97	}
96	98	}
97	99
Index: trunk/phase3/maintenance/Maintenance.php
—	—	@@ -1271,3 +1271,60 @@
1272	1272	return;
1273	1273	}
1274	1274	}
	1275	+
	1276	+abstract class LoggedUpdateMaintenance extends Maintenance {
	1277	+ public function __construct() {
	1278	+ parent::__construct();
	1279	+ $this->addOption( 'force', 'Run the update even if it was completed already' );
	1280	+ }
	1281	+
	1282	+ public function execute() {
	1283	+ $db = $this->getDB( DB_MASTER );
	1284	+ $key = $this->getUpdateKey();
	1285	+
	1286	+ if ( !$this->hasOption( 'force' ) &&
	1287	+ $db->selectRow( 'updatelog', '1', array( 'ul_key' => $key ), __METHOD__ ) )
	1288	+ {
	1289	+ $this->output( $this->updateSkippedMessage() . "\n" );
	1290	+ return true;
	1291	+ }
	1292	+
	1293	+ if ( !$this->doDBUpdates() ) {
	1294	+ return false;
	1295	+ }
	1296	+
	1297	+ if (
	1298	+ $db->insert( 'updatelog', array( 'ul_key' => $key ), __METHOD__, 'IGNORE' ) )
	1299	+ {
	1300	+ return true;
	1301	+ } else {
	1302	+ $this->output( $this->updatelogFailedMessage() . "\n" );
	1303	+ return false;
	1304	+ }
	1305	+ }
	1306	+
	1307	+ /**
	1308	+ * Do the actual work. All child classes will need to implement this.
	1309	+ * Return true to log the update as done or false on failure.
	1310	+ * @return Bool
	1311	+ */
	1312	+ abstract protected function doDBUpdates();
	1313	+
	1314	+ /**
	1315	+ * Get the update key name to go in the update log table
	1316	+ * @return String
	1317	+ */
	1318	+ abstract protected function getUpdateKey();
	1319	+
	1320	+ /**
	1321	+ * Message to show that the update was done already and was just skipped
	1322	+ * @return String
	1323	+ */
	1324	+ abstract protected function updateSkippedMessage();
	1325	+
	1326	+ /**
	1327	+ * Message to show the the update log was unable to log the completion of this update
	1328	+ * @return String
	1329	+ */
	1330	+ abstract protected function updatelogFailedMessage();
	1331	+}
Index: trunk/phase3/includes/installer/DatabaseUpdater.php
—	—	@@ -40,7 +40,9 @@
41	41	protected $shared = false;
42	42
43	43	protected $postDatabaseUpdateMaintenance = array(
44		~~- 'DeleteDefaultMessages'~~
	44	+ 'DeleteDefaultMessages',
	45	+ 'PopulateRevisionLength',
	46	+ 'PopulateRevisionSha1'
45	47	);
46	48
47	49	/**
—	—	@@ -573,14 +575,4 @@
574	576	$task = $this->maintenance->runChild( 'UpdateCollation' );
575	577	$task->execute();
576	578	}
577		-
578		~~- protected function doPopulateRevSha1() {~~
579		~~- if ( $this->updateRowExists( 'populate rev_sha1' ) ) {~~
580		~~- $this->output( "...rev_sha1/ar_sha1 columns already populated.\n" );~~
581		~~- return;~~
582		~~- }~~
583		-
584		~~- $task = $this->maintenance->runChild( 'PopulateRevisionSha1' );~~
585		~~- $task->execute();~~
586		~~- }~~
587	579	}
Index: trunk/phase3/includes/installer/MysqlUpdater.php
—	—	@@ -158,7 +158,6 @@
159	159	array( 'doUpdateTranscacheField' ),
160	160	array( 'renameEuWikiId' ),
161	161	array( 'doUpdateMimeMinorField' ),
162		~~- array( 'doPopulateRevLen' ),~~
163	162
164	163	// 1.17
165	164	array( 'addTable', 'iwlinks', 'patch-iwlinks.sql' ),
—	—	@@ -187,8 +186,7 @@
188	187	array( 'addTable', 'config', 'patch-config.sql' ),
189	188	array( 'addIndex', 'logging', 'type_action', 'patch-logging-type-action-index.sql'),
190	189	array( 'addField', 'revision', 'rev_sha1', 'patch-rev_sha1.sql' ),
191		~~- array( 'addField', 'archive', 'ar_sha1', 'patch-ar_sha1.sql' ),~~
192		~~- array( 'doPopulateRevSha1' )~~
	190	+ array( 'addField', 'archive', 'ar_sha1', 'patch-ar_sha1.sql' )
193	191	);
194	192	}
195	193
—	—	@@ -812,16 +810,6 @@
813	811	$this->output( "done.\n" );
814	812	}
815	813
816		~~- protected function doPopulateRevLen() {~~
817		~~- if ( $this->updateRowExists( 'populate rev_len' ) ) {~~
818		~~- $this->output( "...rev_len column already populated.\n" );~~
819		~~- return;~~
820		~~- }~~
821		-
822		~~- $task = $this->maintenance->runChild( 'PopulateRevisionLength' );~~
823		~~- $task->execute();~~
824		~~- }~~
825		-
826	814	protected function doClFieldsUpdate() {
827	815	if ( $this->updateRowExists( 'cl_fields_update' ) ) {
828	816	$this->output( "...categorylinks up-to-date.\n" );
Index: trunk/phase3/includes/AutoLoader.php
—	—	@@ -832,6 +832,7 @@
833	833	'DeleteArchivedRevisionsImplementation' => 'maintenance/deleteArchivedRevisions.inc',
834	834	'DeleteDefaultMessages' => 'maintenance/deleteDefaultMessages.php',
835	835	'FakeMaintenance' => 'maintenance/Maintenance.php',
	836	+ 'LoggedUpdateMaintenance' => 'maintenance/Maintenance.php',
836	837	'Maintenance' => 'maintenance/Maintenance.php',
837	838	'PopulateCategory' => 'maintenance/populateCategory.php',
838	839	'PopulateLogSearch' => 'maintenance/populateLogSearch.php',

Follow-up revisions

Revision	Commit summary	Author	Date
r94374	More to r94370:...	aaron	19:24, 12 August 2011
r94541	Revert r94289, r94290, r94294, r94333, r94345, r94362, r94370 -- core schema ...	brion	18:24, 15 August 2011
r94546	Restored r94370 changes PopulateRevisionLen, moving it to $postDatabaseUpdate...	aaron	18:52, 15 August 2011
r103033	REL1_18 Partial manual merge of r94370, r94382, r96578, r101019 to bring Logg...	reedy	21:20, 14 November 2011

Past revisions this follows-up on

Revision	Commit summary	Author	Date
r94289	* Added rev_sha1 and ar_sha1 columns to revision/archive tables (useful for b......	aaron	21:52, 11 August 2011
r94362	Fix for r94289: we want to skip rows with non-empty sha1, not non-NULL (which...	aaron	16:50, 12 August 2011

Comments

#Comment by Krinkle (talk | contribs) 19:18, 12 August 2011

Thanks, finally I can update my wiki again :-)

Populating ar_sha1 column
...revision table seems to be empty.
rev_sha1 and ar_sha1 population complete [28 revision rows, 1 archive rows].

The comment seems wrong though, it hardcodes "revision".

#Comment by Krinkle (talk | contribs) 19:21, 12 August 2011

http://ci.tesla.usability.wikimedia.org/cruisecontrol/buildresults/mw:

PHP Warning:  call_user_func_array() expects parameter 1 to be a valid callback, function 'doPopulateRevSha1' not found or invalid function name in /home/ci/cruisecontrol-bin-2.8.3/projects/mw/source/includes/installer/DatabaseUpdater.php on line 230

#Comment by Brion VIBBER (talk | contribs) 18:27, 15 August 2011

undiscussed core schema change, reverted in r94541.

#Comment by 😂 (talk | contribs) 18:31, 15 August 2011

The ideas in this commit are good and should be reapplied :)

#Comment by Aaron Schulz (talk | contribs) 18:32, 15 August 2011

Yeah, I'm trying to do that now.

Status & tagging log

18:27, 15 August 2011 Brion VIBBER (talk | contribs) changed the status of r94370 [removed: new added: reverted]
07:00, 13 August 2011 Aaron Schulz (talk | contribs) changed the status of r94370 [removed: fixme added: new]
19:18, 12 August 2011 Krinkle (talk | contribs) changed the status of r94370 [removed: new added: fixme]