r98927 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r98926‎ | r98927 | r98928 >
Date:21:31, 4 October 2011
Author:aaron
Status:ok (Comments)
Tags:
Comment:
(bug 31179) Fixed problems with undeletion making bad page rows:
* Moved up isCountable() call, since insertOn() sets the page ID, it safer to do this call first. It now uses master DB data, as it should for reads to write data. Furthermore, calling it after insertOn() was triggered loadPageData() on a slave, breaking the page ID cache just set in insertOn().
* Preemptively call loadPageData( 'fromdbmaster' ) as Article::doEdit() does (r98880) to avoid any lazy loading from the slave which can corrupt the page ID cache.
Modified paths:
  • /trunk/phase3/includes/specials/SpecialUndelete.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/specials/SpecialUndelete.php
——@@ -399,6 +399,10 @@
400400
401401 # Does this page already exist? We'll have to update it...
402402 $article = new Article( $this->title );
 403+ # Load latest data for the current page (bug 31179)
 404+ $article->loadPageData( 'fromdbmaster' );
 405+ $oldcountable = $article->isCountable();
 406+
403407 $options = 'FOR UPDATE'; // lock page
404408 $page = $dbw->selectRow( 'page',
405409 array( 'page_id', 'page_latest' ),
——@@ -532,7 +536,6 @@
533537 }
534538
535539 $created = (bool)$newid;
536 - $oldcountable = $article->isCountable();
537540
538541 // Attach the latest revision to the page...
539542 $wasnew = $article->updateIfNewerOn( $dbw, $revision, $previousRevId );

Sign-offs

UserFlagDate
Brion VIBBERinspected21:41, 4 October 2011

Follow-up revisions

RevisionCommit summaryAuthorDate
r98928MFT r98927aaron21:33, 4 October 2011
r98932live hack to log for bug 31179 instancesaaron21:53, 4 October 2011
r100375REL1_18 MFT r98927, r98990, r99081, r99082, r99091, r99102, r99104, r99126reedy21:08, 20 October 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r98880Call loadPageData() with 'fromdbmaster' in doEdit() to avoid old slave dataaaron18:40, 4 October 2011

Comments

#Comment by Brion VIBBER (talk | contribs)   21:40, 4 October 2011

Should help -- fingers crossed. :)

Change to the isCountable() fetch position shouldn't actually make a difference as we haven't actually updated the article's page record with the new latest revision -- but I think it's clearer to ask for it up front the new way.

#Comment by Aaron Schulz (talk | contribs)   21:47, 4 October 2011

Well, there is an off chance of isCountable() returning stale data if the slave is lagged...not likely though.

#Comment by Brion VIBBER (talk | contribs)   22:08, 4 October 2011

Well now that the loadPageData is forced to master, the only thing that isCountable would get from a slave is the count of pagelinks entries (which is explicitly pulled from DB_SLAVE) and I'm not sure it matters whether you ask up front or down low...

#Comment by Aaron Schulz (talk | contribs)   22:13, 4 October 2011

Gah, I meant "there *was* an off chance". I was referring to the call being *after* the new loadFromDB() call to avoid slave lag.

The isCountable() position shouldn't make a difference given the new loadFromDB() call, since no load will be trigger. I just thought it was clearer, as you said.

Status & tagging log