r103146 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r103145‎ | r103146 | r103147 >
Date:10:59, 15 November 2011
Author:petrb
Status:deferred (Comments)
Tags:
Comment:
New performance fix, now it checks if there is a need to do delete before performing it,
it needs extra select but that is less expensive than delete (croned it's still faster)
this needs to be reviewed by someone who knows sql, it's possible that executing
empty delete (0 rows are matched by condition) doesn't take more time than than executing
empty select + it's even possible that it doesn't replicate db in that case
if that is true, fix me
+ some fixes :)
Modified paths:
  • /trunk/extensions/OnlineStatusBar/OnlineStatusBar.body.php (modified) (history)
  • /trunk/extensions/OnlineStatusBar/OnlineStatusBar.status.php (modified) (history)

Diff [purge]

Index: trunk/extensions/OnlineStatusBar/OnlineStatusBar.status.php
——@@ -38,7 +38,7 @@
3939 if ( $user->isLoggedIn() ) {
4040 $status = $user->getOption( 'OnlineStatusBar_status', $wgOnlineStatusBarDefaultOnline );
4141 if ( $delayed_check ) {
42 - if ( $result < $w_time ) {
 42+ if ( $result < wfTimestamp( TS_MW, $w_time ) ) {
4343 $status = 'write';
4444 }
4545 }
——@@ -131,6 +131,14 @@
132132 return 0;
133133 }
134134 $dbw = wfGetDB( DB_MASTER );
 135+ $t_time = OnlineStatusBar::getTimeoutDate();
 136+ $result = $dbw->selectField( 'online_status', 'timestamp', array( "timestamp < " . $dbw->addQuotes( $dbw->timestamp( $t_time ) ) ),
 137+ __METHOD__, array( 'LIMIT 1' ) );
 138+ if ( $result === false ) {
 139+ // no need for delete
 140+ return 0;
 141+ }
 142+
135143 // calculate time and convert it back to mediawiki format
136144 $time = OnlineStatusBar::getTimeoutDate();
137145 $dbw->delete( 'online_status', array( "timestamp < " . $dbw->addQuotes( $dbw->timestamp( $time ) ) ), __METHOD__ );
Index: trunk/extensions/OnlineStatusBar/OnlineStatusBar.body.php
——@@ -131,7 +131,7 @@
132132 global $wgOnlineStatusBar_WriteTime, $wgOnlineStatusBar_LogoutTime;
133133
134134 if ($delayed) {
135 - return wfTimestamp( TS_UNIX ) + $wgOnlineStatusBar_WriteTime;
 135+ return wfTimestamp( TS_UNIX ) - $wgOnlineStatusBar_WriteTime;
136136 }
137137
138138 return wfTimestamp( TS_UNIX ) - $wgOnlineStatusBar_LogoutTime;

Comments

#Comment by Catrope (talk | contribs)   11:04, 15 November 2011

This is usually a bad idea because it gives rise to race conditions: what if a row is added or changed between the SELECT and DELETE queries? Lazy DELETEs that may or may not delete zero rows tend to be a good idea, not a bad idea.

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

it doesn't matter, the delete is not important, it only clean up the table, so if there is insert between select and delete it wouldn't change much, the function which checks if user is online has the timestamp check too, so if there would be some old records it wouldn't matter anyway, best would be if this delete was performed like every 30 minutes or so. but I don't think php can do cron-like stuff

#Comment by Petrb (talk | contribs)   11:13, 15 November 2011

as Ian suggested to me, it's better not to call too many "changing" sql's, because cluster which consist from master and slaves, needs to replicate data when something get updated, so I just inserted that select there to prevent needless delete, it is there just to reduce table size.

#Comment by Catrope (talk | contribs)   11:14, 15 November 2011

Oh, OK, in that case it's fine.

Status & tagging log