http://www.mediawiki.org/wiki/Special:Code/MediaWiki/96722
Revision: 96722
Author: catrope
Date: 2011-09-10 15:26:11 +0000 (Sat, 10 Sep 2011)
Log Message:
-----------
Epic followup to r94404, r94509: change the ArticleFeedback schema such that
locking is no longer necessary. This should fix the counter drift problems,
too. Thanks to Tim for pointing me in this direction.
* Change the schema of article_feedback to no longer have a primary key that
ensures one rating per user per revision, but to have a primary key on the
auto-incremented field aa_id instead
** This allows multiple ratings per user per revision in the article_feedback
table, which makes the counter drift fix possible
** This schema change is so radical that the patch has to create another table
with the new schema, copy the data over, and swap the tables
* Changed ApiArticleFeedback::insertUserRatings() to insert all the ratings
rows at once, using one query. This ensures they get consecutive aa_id values
* Moved the SELECT in ApiArticleFeedback after the insertUserRatings() call,
and added an "aa_id < $id" clause so it always looks at the rating set right
before the one that was just inserted, even if another thread has already
inserted another rating set in the meantime. This ensures that each rating
delta is handled by exactly one thread, which should fix counter drift.
* Removed the useless and broken behavior of decrementing values in
article_feedback_revisions. It was implemented inconsistently and never worked
properly for that reason. This makes insertRevisionRating() much simpler
* Due to the insertRevisionRating() change. $lastRevision is no longer needed.
To properly update the revision delta, though, we still need to look at whether
the previous rating's revid matches this one's, and only take that rating into
account if it does. The previous logic was probably broken and, now that I
think about it, was probably the cause of all the weird behavior, but it's too
late to go back now. I'll patch 1.17wmf1 for just this problem later, see if
that helps
* Take this opportunity to redo the indexes on article_feedback so the queries
run on it are actually indexed properly. Also drop useless aa_user_id
conditions from queries.
Modified Paths:
--------------
trunk/extensions/ArticleFeedback/ArticleFeedback.hooks.php
trunk/extensions/ArticleFeedback/api/ApiArticleFeedback.php
trunk/extensions/ArticleFeedback/api/ApiQueryArticleFeedback.php
trunk/extensions/ArticleFeedback/sql/ArticleFeedback.sql
Added Paths:
-----------
trunk/extensions/ArticleFeedback/sql/RecreatePK.sql
Modified: trunk/extensions/ArticleFeedback/ArticleFeedback.hooks.php
===================================================================
--- trunk/extensions/ArticleFeedback/ArticleFeedback.hooks.php 2011-09-10
13:04:07 UTC (rev 96721)
+++ trunk/extensions/ArticleFeedback/ArticleFeedback.hooks.php 2011-09-10
15:26:11 UTC (rev 96722)
@@ -230,6 +230,15 @@
$dir .
'/sql/AddArticleFeedbackTimestampIndex.sql',
true
) );
+
+ // This change recreates the PK on a new field. Check
for that new field's existence
+ $updater->addExtensionUpdate( array(
+ 'addField',
+ 'article_feedback',
+ 'aa_id',
+ $dir . '/sql/RecreatePK.sql',
+ true
+ ) );
}
return true;
}
Modified: trunk/extensions/ArticleFeedback/api/ApiArticleFeedback.php
===================================================================
--- trunk/extensions/ArticleFeedback/api/ApiArticleFeedback.php 2011-09-10
13:04:07 UTC (rev 96721)
+++ trunk/extensions/ArticleFeedback/api/ApiArticleFeedback.php 2011-09-10
15:26:11 UTC (rev 96722)
@@ -38,57 +38,60 @@
}
$dbw = wfGetDB( DB_MASTER );
-
- // Query the latest ratings by this user for this page,
+
+ $pageId = $params['pageid'];
+ $revisionId = $params['revid'];
+
+ // Build array( rating ID => rating value )
+ $ratings = array();
+ foreach ( $wgArticleFeedbackRatings as $ratingID ) {
+ $ratings[$ratingID] = intval( $params["r{$ratingID}"] );
+ }
+ // Insert the new ratings
+ $id = $this->insertUserRatings( $pageId, $revisionId, $wgUser,
$token, $ratings, $params['bucket'] );
+
+ // Query the previous ratings by this user for this page,
// possibly for an older revision
- // Select from the master to prevent replag-induced bugs
+ // Use aa_id < $id to make sure we get the one before ours even
if other ratings were inserted after
+ // insertUserRatings() but before selectRow(); this prevents
race conditions from messing things up.
$res = $dbw->select(
'article_feedback',
array( 'aa_rating_id', 'aa_rating_value', 'aa_revision'
),
array(
- 'aa_user_id' => $wgUser->getId(),
'aa_user_text' => $wgUser->getName(),
'aa_page_id' => $params['pageid'],
'aa_rating_id' => $wgArticleFeedbackRatings,
'aa_user_anon_token' => $token,
+ 'aa_id < ' . intval( $id )
),
__METHOD__,
array(
- 'ORDER BY' => 'aa_revision DESC',
+ 'ORDER BY' => 'aa_id DESC',
'LIMIT' => count( $wgArticleFeedbackRatings ),
)
);
-
$lastRatings = array();
-
foreach ( $res as $row ) {
$lastRatings[$row->aa_rating_id]['value'] =
$row->aa_rating_value;
$lastRatings[$row->aa_rating_id]['revision'] =
$row->aa_revision;
}
-
- $pageId = $params['pageid'];
- $revisionId = $params['revid'];
-
- foreach( $wgArticleFeedbackRatings as $rating ) {
- $lastRating = false;
- $lastRevision = false;
+
+ foreach ( $wgArticleFeedbackRatings as $rating ) {
+ $lastPageRating = false;
+ $lastRevRating = false;
if ( isset( $lastRatings[$rating] ) ) {
- $lastRating = intval(
$lastRatings[$rating]['value'] );
- $lastRevision = intval(
$lastRatings[$rating]['revision'] );
+ $lastPageRating = intval(
$lastRatings[$rating]['value'] );
+ if ( intval( $lastRatings[$rating]['revision']
) == $revisionId ) {
+ $lastRevRating = $lastPageRating;
+ }
}
-
- $thisRating = false;
- if ( isset( $params["r{$rating}"] ) ) {
- $thisRating = intval( $params["r{$rating}"] );
- }
-
- $this->insertRevisionRating( $pageId, $revisionId,
$lastRevision, $rating, ( $thisRating - $lastRating ),
- $thisRating, $lastRating
- );
+ $thisRating = intval( $params["r{$rating}"] );
- $this->insertPageRating( $pageId, $rating, (
$thisRating - $lastRating ), $thisRating, $lastRating );
-
- $this->insertUserRatings( $pageId, $revisionId,
$wgUser, $token, $rating, $thisRating, $params['bucket'] );
+ // Update counter tables
+ $this->insertRevisionRating( $pageId, $revisionId,
$rating, $thisRating - $lastRevRating,
+ $thisRating, $lastRevRating
+ );
+ $this->insertPageRating( $pageId, $rating, $thisRating
- $lastPageRating, $thisRating, $lastPageRating );
}
$this->insertProperties( $revisionId, $wgUser, $token, $params
);
@@ -157,13 +160,12 @@
*
* @param $pageId Integer: Page Id
* @param $revisionId Integer: Revision Id
- * @param $lastRevision Integer: Revision Id of last rating
* @param $ratingId Integer: Rating Id
* @param $updateAddition Integer: Difference between user's last
rating (if applicable)
* @param $thisRating Integer|Boolean: Value of the Rating
* @param $lastRating Integer|Boolean: Value of the last Rating
*/
- private function insertRevisionRating( $pageId, $revisionId,
$lastRevision, $ratingId, $updateAddition, $thisRating, $lastRating ) {
+ private function insertRevisionRating( $pageId, $revisionId, $ratingId,
$updateAddition, $thisRating, $lastRating ) {
$dbw = wfGetDB( DB_MASTER );
// Try to insert a new "totals" row for this page,rev,rating set
@@ -179,58 +181,21 @@
__METHOD__,
array( 'IGNORE' )
);
-
- // If that succeded in inserting a row, then we are for sure
rating a previously unrated
- // revision, and we need to add more information about this
rating to the new "totals" row,
- // as well as remove the previous rating values from the
previous "totals" row
- if ( $dbw->affectedRows() ) {
- // If there was a previous rating, there should be a
"totals" row for it's revision
- if ( $lastRating ) {
- // Deduct the previous rating values from the
previous "totals" row
- $dbw->update(
- 'article_feedback_revisions',
- array(
- 'afr_total = afr_total - ' .
intval( $lastRating ),
- 'afr_count = afr_count - 1',
- ),
- array(
- 'afr_page_id' => $pageId,
- 'afr_rating_id' => $ratingId,
- 'afr_revision' => $lastRevision
- ),
- __METHOD__
- );
- }
- // Add this rating's values to the new "totals" row
- $dbw->update(
- 'article_feedback_revisions',
- array(
- 'afr_total' => $thisRating,
- 'afr_count' => $thisRating ? 1 : 0,
- ),
- array(
- 'afr_page_id' => $pageId,
- 'afr_rating_id' => $ratingId,
- 'afr_revision' => $revisionId,
- ),
- __METHOD__
- );
- } else {
- // Apply the difference between the previous and new
ratings to the current "totals" row
- $dbw->update(
- 'article_feedback_revisions',
- array(
- 'afr_total = afr_total + ' .
$updateAddition,
- 'afr_count = afr_count + ' .
$this->getCountChange( $lastRating, $thisRating ),
- ),
- array(
- 'afr_page_id' => $pageId,
- 'afr_rating_id' => $ratingId,
- 'afr_revision' => $revisionId,
- ),
- __METHOD__
- );
- }
+
+ // Apply the difference between the previous and new ratings to
the current "totals" row
+ $dbw->update(
+ 'article_feedback_revisions',
+ array(
+ 'afr_total = afr_total + ' . $updateAddition,
+ 'afr_count = afr_count + ' .
$this->getCountChange( $lastRating, $thisRating ),
+ ),
+ array(
+ 'afr_page_id' => $pageId,
+ 'afr_rating_id' => $ratingId,
+ 'afr_revision' => $revisionId,
+ ),
+ __METHOD__
+ );
}
/**
* Calculate the difference between the previous rating and this one
@@ -247,55 +212,45 @@
}
/**
- * Inserts (or Updates, where appropriate) the users ratings for a
specific revision
+ * Inserts the user's ratings for a specific revision
*
* @param $pageId Integer: Page Id
* @param $revisionId Integer: Revision Id
* @param $user User: Current User object
* @param $token Array: Token if necessary
- * @param $ratingId Integer: Rating Id
- * @param $ratingValue Integer: Value of the Rating
+ * @param $ratings Array: Keys are rating IDs, values are rating values
* @param $bucket Integer: Which rating widget was the user shown
+ * @return int Return value of $dbw->insertID(). This is the aa_id of
the first (MySQL) or last (SQLite) inserted row
*/
- private function insertUserRatings( $pageId, $revisionId, $user,
$token, $ratingId, $ratingValue, $bucket ) {
+ private function insertUserRatings( $pageId, $revisionId, $user,
$token, $ratings, $bucket ) {
$dbw = wfGetDB( DB_MASTER );
$timestamp = $dbw->timestamp();
-
- $dbw->insert(
- 'article_feedback',
- array(
+
+ $rows = array();
+ foreach ( $ratings as $ratingID => $ratingValue ) {
+ $rows[] = array(
'aa_page_id' => $pageId,
'aa_user_id' => $user->getId(),
'aa_user_text' => $user->getName(),
'aa_user_anon_token' => $token,
'aa_revision' => $revisionId,
'aa_timestamp' => $timestamp,
- 'aa_rating_id' => $ratingId,
+ 'aa_rating_id' => $ratingID,
'aa_rating_value' => $ratingValue,
'aa_design_bucket' => $bucket,
- ),
- __METHOD__,
- array( 'IGNORE' )
- );
-
- if ( !$dbw->affectedRows() ) {
- $dbw->update(
- 'article_feedback',
- array(
- 'aa_timestamp' => $timestamp,
- 'aa_rating_value' => $ratingValue,
- ),
- array(
- 'aa_page_id' => $pageId,
- 'aa_user_text' => $user->getName(),
- 'aa_revision' => $revisionId,
- 'aa_rating_id' => $ratingId,
- 'aa_user_anon_token' => $token,
- ),
- __METHOD__
);
}
+
+ // In MySQL, there is native support for multi-row inserts and
our rows
+ // will automatically get consecutive insertIDs. In SQLite this
seems
+ // to be the case if you use a transaction, but I couldn't find
anything
+ // on the web that states whether this is true.
+ $dbw->begin();
+ $dbw->insert( 'article_feedback', $rows, __METHOD__ );
+ $dbw->commit();
+
+ return $dbw->insertID();
}
/**
Modified: trunk/extensions/ArticleFeedback/api/ApiQueryArticleFeedback.php
===================================================================
--- trunk/extensions/ArticleFeedback/api/ApiQueryArticleFeedback.php
2011-09-10 13:04:07 UTC (rev 96721)
+++ trunk/extensions/ArticleFeedback/api/ApiQueryArticleFeedback.php
2011-09-10 15:26:11 UTC (rev 96722)
@@ -187,14 +187,13 @@
array(
'aa_page_id' => $params['pageid'],
'aa_rating_id' => $wgArticleFeedbackRatings,
- 'aa_user_id' => $wgUser->getId(),
'aa_user_text' => $wgUser->getName(),
'aa_user_anon_token' => $this->getAnonToken(
$params ),
),
__METHOD__,
array(
'LIMIT' => count( $wgArticleFeedbackRatings ),
- 'ORDER BY' => 'aa_revision DESC',
+ 'ORDER BY' => 'aa_id DESC',
),
array(
'article_feedback_ratings' => array( 'LEFT
JOIN', array( 'aar_id=aa_rating_id' ) )
Modified: trunk/extensions/ArticleFeedback/sql/ArticleFeedback.sql
===================================================================
--- trunk/extensions/ArticleFeedback/sql/ArticleFeedback.sql 2011-09-10
13:04:07 UTC (rev 96721)
+++ trunk/extensions/ArticleFeedback/sql/ArticleFeedback.sql 2011-09-10
15:26:11 UTC (rev 96722)
@@ -14,6 +14,8 @@
-- Store article feedbacks (user rating per revision)
CREATE TABLE IF NOT EXISTS /*_*/article_feedback (
+ -- Row ID (primary key)
+ aa_id integer unsigned NOT NULL PRIMARY KEY AUTO_INCREMENT,
-- Foreign key to page.page_id
aa_page_id integer unsigned NOT NULL,
-- User Id (0 if anon)
@@ -31,11 +33,10 @@
-- Value of the rating (0 is "unrated", else 1-5)
aa_rating_value int unsigned NOT NULL,
-- Which rating widget the user was given. Default of 0 is the "old" design
- aa_design_bucket int unsigned NOT NULL DEFAULT 0,
- -- 1 vote per user per revision
- PRIMARY KEY (aa_revision, aa_user_text, aa_rating_id, aa_user_anon_token)
+ aa_design_bucket int unsigned NOT NULL DEFAULT 0
) /*$wgDBTableOptions*/;
-CREATE INDEX /*i*/aa_user_page_revision ON /*_*/article_feedback (aa_user_id,
aa_page_id, aa_revision);
+CREATE INDEX /*i*/aa_page_user_token_id ON /*_*/article_feedback (aa_page_id,
aa_user_text, aa_user_anon_token, aa_id);
+CREATE INDEX /*i*/aa_revision ON /*_*/article_feedback (aa_revision);
-- Create an index on the article_feedback.aa_timestamp field
CREATE INDEX /*i*/article_feedback_timestamp ON /*_*/article_feedback
(aa_timestamp);
Added: trunk/extensions/ArticleFeedback/sql/RecreatePK.sql
===================================================================
--- trunk/extensions/ArticleFeedback/sql/RecreatePK.sql
(rev 0)
+++ trunk/extensions/ArticleFeedback/sql/RecreatePK.sql 2011-09-10 15:26:11 UTC
(rev 96722)
@@ -0,0 +1,38 @@
+-- Add aa_id as the new primary key to article_feedback and drop the old one.
+-- Also change the indexing while we're at it
+
+-- In order to safely change the primary key even in replicated environments,
+-- we have to create a new table with the new structure, copy over the data,
+-- then rename the table. This is to ensure that the values of aa_id are
+-- consistent across all slaves.
+
+-- Create new table
+-- Would've used CREATE TABLE ... LIKE here but SQLite doesn't support ALTER
TABLE ... DROP PRIMARY KEY
+-- so we're stuck with duplicating the table structure.
+CREATE TABLE /*_*/article_feedback2 (
+ aa_id integer unsigned NOT NULL PRIMARY KEY AUTO_INCREMENT,
+ aa_page_id integer unsigned NOT NULL,
+ aa_user_id integer NOT NULL,
+ aa_user_text varbinary(255) NOT NULL,
+ aa_user_anon_token varbinary(32) NOT NULL DEFAULT '',
+ aa_revision integer unsigned NOT NULL,
+ aa_timestamp binary(14) NOT NULL DEFAULT '',
+ aa_rating_id int unsigned NOT NULL,
+ aa_rating_value int unsigned NOT NULL,
+ aa_design_bucket int unsigned NOT NULL DEFAULT 0
+) /*$wgDBTableOptions*/;
+CREATE INDEX /*i*/aa_page_user_token_id ON /*_*/article_feedback2 (aa_page_id,
aa_user_text, aa_user_anon_token, aa_id);
+CREATE INDEX /*i*/aa_revision ON /*_*/article_feedback2 (aa_revision);
+CREATE INDEX /*i*/article_feedback_timestamp ON /*_*/article_feedback2
(aa_timestamp);
+
+-- Copy the data, ordered by the old primary key
+-- Need to specify the fields explicitly to avoid confusion with aa_id
+INSERT INTO /*_*/article_feedback2
+ (aa_page_id, aa_user_id, aa_user_text, aa_user_anon_token, aa_revision,
aa_timestamp, aa_rating_id, aa_rating_value, aa_design_bucket)
+ SELECT aa_page_id, aa_user_id, aa_user_text, aa_user_anon_token,
aa_revision, aa_timestamp, aa_rating_id, aa_rating_value, aa_design_bucket
+ FROM /*_*/article_feedback
+ ORDER BY aa_revision, aa_user_text, aa_rating_id, aa_user_anon_token;
+
+-- Drop the old table and rename the new table to the old name
+DROP TABLE /*_*/article_feedback;
+ALTER TABLE /*_*/article_feedback2 RENAME TO /*_*/article_feedback;
Property changes on: trunk/extensions/ArticleFeedback/sql/RecreatePK.sql
___________________________________________________________________
Added: svn:eol-style
+ native
_______________________________________________
MediaWiki-CVS mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-cvs
