KartikMistry has uploaded a new change for review.
https://gerrit.wikimedia.org/r/305190
Change subject: Avoid deadlock patterns in cx_corpora updates
......................................................................
Avoid deadlock patterns in cx_corpora updates
Bug: T134245
Change-Id: I975a2f4698ec0b7045222bcd43df3fac00dcc9bb
(cherry picked from commit 31a5bc98b63312ea6d91eae88855837db5dd9160)
---
M api/ApiContentTranslationSave.php
M includes/Translation.php
M includes/TranslationStorageManager.php
M includes/TranslationUnit.php
4 files changed, 62 insertions(+), 27 deletions(-)
git pull
ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/ContentTranslation
refs/changes/90/305190/1
diff --git a/api/ApiContentTranslationSave.php
b/api/ApiContentTranslationSave.php
index 26491bd..15f3000 100644
--- a/api/ApiContentTranslationSave.php
+++ b/api/ApiContentTranslationSave.php
@@ -56,7 +56,7 @@
$this->translation = $this->saveTranslation( $params );
$translationUnits = $this->getTranslationUnits(
$params['content'] );
- $this->saveTranslationUnits( $translationUnits );
+ $this->saveTranslationUnits( $translationUnits,
$this->translation );
$validationResults = $this->validateTranslationUnits(
$translationUnits );
$translationId = $this->translation->getTranslationId();
@@ -126,6 +126,11 @@
return $results;
}
+ /**
+ * @param array $params
+ * @return Translation
+ * @throws UsageException
+ */
protected function saveTranslation( array $params ) {
$translation = Translation::find(
$params['from'],
@@ -211,9 +216,14 @@
return $translationUnits;
}
- protected function saveTranslationUnits( $translationUnits ) {
+ /**
+ * @param $translationUnits
+ * @param Translation $translation Recently saved parent translation
object
+ */
+ protected function saveTranslationUnits( $translationUnits, Translation
$translation ) {
+ $newTranslation = $translation->lastSaveWasCreate();
foreach ( $translationUnits as $translationUnit ) {
- TranslationStorageManager::save( $translationUnit );
+ TranslationStorageManager::save( $translationUnit,
$newTranslation );
}
}
diff --git a/includes/Translation.php b/includes/Translation.php
index b947bfe..fe4400d 100644
--- a/includes/Translation.php
+++ b/includes/Translation.php
@@ -5,6 +5,8 @@
namespace ContentTranslation;
class Translation {
+ private $lastSaveWasCreate = false;
+
function __construct( $translation ) {
$this->translation = $translation;
}
@@ -78,8 +80,6 @@
* translation exists and chooses either of create or update actions.
*/
public function save() {
- $freshTranslation = false;
-
$existingTranslation = Translation::find(
$this->translation['sourceLanguage'],
$this->translation['targetLanguage'],
@@ -88,6 +88,7 @@
if ( $existingTranslation === null ) {
$this->create();
+ $this->lastSaveWasCreate = true;
} else {
$options = [];
if ( $existingTranslation->translation['status'] ===
'deleted' ) {
@@ -97,9 +98,17 @@
}
$this->translation['id'] =
$existingTranslation->getTranslationId();
$this->update( $options );
+ $this->lastSaveWasCreate = false;
}
}
+ /**
+ * @return bool Whether the last save() call on this object instance
made a new row
+ */
+ public function lastSaveWasCreate() {
+ return $this->lastSaveWasCreate;
+ }
+
/*
* @param string $sourceLanguage
* @param string $targetLanguage
diff --git a/includes/TranslationStorageManager.php
b/includes/TranslationStorageManager.php
index d6f310a..fdcd1a0 100644
--- a/includes/TranslationStorageManager.php
+++ b/includes/TranslationStorageManager.php
@@ -76,29 +76,46 @@
* If the record exist, update it, otherwise create.
*
* @param TranslationUnit $translationUnit
+ * @param bool $newTranslation Whether these are for a brand new
Translation record
*/
- public static function save( TranslationUnit $translationUnit ) {
- $db = Database::getConnection( DB_MASTER );
+ public static function save( TranslationUnit $translationUnit,
$newTranslation ) {
+ $dbw = Database::getConnection( DB_MASTER );
- $db->doAtomicSection( __METHOD__, function ( $db ) use (
$translationUnit ) {
- $conditions = [
- 'cxc_translation_id' =>
$translationUnit->getTranslationId(),
- 'cxc_section_id' =>
$translationUnit->getSectionId(),
- 'cxc_origin' => $translationUnit->getOrigin()
- ];
- // (At least attempt to) avoid inserting duplicate
records in case
- // of race condition between the select query and the
insert query,
- // resulting duplicate record error.
- $options = [ 'FOR UPDATE' ];
+ $dbw->doAtomicSection(
+ __METHOD__,
+ function ( \IDatabase $dbw ) use ( $translationUnit,
$newTranslation ) {
+ if ( $newTranslation ) {
+ // T134245: brand new translations can
also insert corpora data in the same
+ // request. The doFind() query uses
only a subset of a unique cx_corpora index,
+ // causing SH gap locks. Worse, is that
the leftmost values comes from the
+ // auto-incrementing translation_id.
This puts gap locks on the range of
+ // (MAX(cxc_translation_id),+infinity),
which could make the whole API prone
+ // to deadlocks and timeouts. Bypass
this problem by remembering if the parent
+ // translation row is brand new and
skipping doFind() in such cases.
+ $existing = false;
+ } else {
+ // Update the lastest row if there is
one instead of making a new one
+ $conditions = [
+ 'cxc_translation_id' =>
$translationUnit->getTranslationId(),
+ 'cxc_section_id' =>
$translationUnit->getSectionId(),
+ 'cxc_origin' =>
$translationUnit->getOrigin()
+ ];
+ // Note that the only caller of this
method will have already locked the
+ // parent Translation row, serializing
simultaneous duplicate submissions at
+ // this point. Without that row lock,
the two transaction might both acquire
+ // SH gap locks in doFind() and then
deadlock in create() trying to get IX gap
+ // locks (if no duplicate rows were
found).
+ $options = [ 'FOR UPDATE' ];
+ $existing = self::doFind( $dbw,
$conditions, $options, __METHOD__ );
+ }
- $existing = self::doFind( $db, $conditions, $options,
__METHOD__ );
-
- if ( $existing ) {
- self::update( $db, $translationUnit,
$existing->getTimestamp() );
- } else {
- self::create( $db, $translationUnit );
+ if ( $existing ) {
+ self::update( $dbw, $translationUnit,
$existing->getTimestamp() );
+ } else {
+ self::create( $dbw, $translationUnit );
+ }
}
- } );
+ );
}
/**
@@ -121,7 +138,7 @@
return self::doFind( $db, $conditions, [], __METHOD__ );
}
- private static function doFind( $db, $conditions, $options, $method ) {
+ private static function doFind( \IDatabase $db, $conditions, $options,
$method ) {
$options['ORDER BY'] = 'cxc_timestamp DESC';
$row = $db->selectRow( 'cx_corpora', '*', $conditions, $method,
$options );
@@ -131,6 +148,5 @@
}
return null;
-
}
}
diff --git a/includes/TranslationUnit.php b/includes/TranslationUnit.php
index 08f9238..2efd2ee 100644
--- a/includes/TranslationUnit.php
+++ b/includes/TranslationUnit.php
@@ -37,7 +37,7 @@
}
/**
- * @param stdClass $row
+ * @param \stdClass $row
* @return TranslationUnit
*/
public static function newFromRow( $row ) {
--
To view, visit https://gerrit.wikimedia.org/r/305190
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I975a2f4698ec0b7045222bcd43df3fac00dcc9bb
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/ContentTranslation
Gerrit-Branch: wmf/1.28.0-wmf.15
Gerrit-Owner: KartikMistry <[email protected]>
Gerrit-Reviewer: Aaron Schulz <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
