From 4b4afe7cbe83d770c0f5c06406d2110a2d7f56f6 Mon Sep 17 00:00:00 2001 From: daniel Date: Wed, 22 Aug 2018 18:08:23 +0200 Subject: [PATCH] Avoid joins when reading MCR revisions. When loading revision slots from the new MCR schema, slot role names and content model names need to be resolved. Doing so programmatically based on an in-memory cache avoids having to join against two additional table every time we load slot meta-data. This relies on NameTableStore, which was designed for exactly this use case. Bug: T198561 Change-Id: Ic005931b669f9d0173ef47ac17331d44fb1141ca --- includes/Storage/RevisionStore.php | 32 +++++-- .../Storage/McrReadNewRevisionStoreDbTest.php | 85 +++++++++++++------ .../Storage/McrRevisionStoreDbTest.php | 85 +++++++++++++------ 3 files changed, 144 insertions(+), 58 deletions(-) diff --git a/includes/Storage/RevisionStore.php b/includes/Storage/RevisionStore.php index 88d520c091..d219267298 100644 --- a/includes/Storage/RevisionStore.php +++ b/includes/Storage/RevisionStore.php @@ -1547,6 +1547,10 @@ class RevisionStore $slots = []; foreach ( $res as $row ) { + // resolve role names and model names from in-memory cache, instead of joining. + $row->role_name = $this->slotRoleStore->getName( (int)$row->slot_role_id ); + $row->model_name = $this->contentModelStore->getName( (int)$row->content_model ); + $contentCallback = function ( SlotRecord $slot ) use ( $queryFlags, $row ) { return $this->loadSlotContent( $slot, null, null, null, $queryFlags ); }; @@ -2268,6 +2272,9 @@ class RevisionStore * * @param array $options Any combination of the following strings * - 'content': Join with the content table, and select content meta-data fields + * - 'model': Join with the content_models table, and select the model_name field. + * Only applicable if 'content' is also set. + * - 'role': Join with the slot_roles table, and select the role_name field * * @return array With three keys: * - tables: (string[]) to include in the `$table` to `IDatabase->select()` @@ -2304,26 +2311,39 @@ class RevisionStore } } else { $ret['tables'][] = 'slots'; - $ret['tables'][] = 'slot_roles'; $ret['fields'] = array_merge( $ret['fields'], [ 'slot_revision_id', 'slot_content_id', 'slot_origin', - 'role_name' + 'slot_role_id', ] ); - $ret['joins']['slot_roles'] = [ 'INNER JOIN', [ 'slot_role_id = role_id' ] ]; + + if ( in_array( 'role', $options, true ) ) { + // Use left join to attach role name, so we still find the revision row even + // if the role name is missing. This triggers a more obvious failure mode. + $ret['tables'][] = 'slot_roles'; + $ret['joins']['slot_roles'] = [ 'LEFT JOIN', [ 'slot_role_id = role_id' ] ]; + $ret['fields'][] = 'role_name'; + } if ( in_array( 'content', $options, true ) ) { $ret['tables'][] = 'content'; - $ret['tables'][] = 'content_models'; $ret['fields'] = array_merge( $ret['fields'], [ 'content_size', 'content_sha1', 'content_address', - 'model_name' + 'content_model', ] ); $ret['joins']['content'] = [ 'INNER JOIN', [ 'slot_content_id = content_id' ] ]; - $ret['joins']['content_models'] = [ 'INNER JOIN', [ 'content_model = model_id' ] ]; + + if ( in_array( 'model', $options, true ) ) { + // Use left join to attach model name, so we still find the revision row even + // if the model name is missing. This triggers a more obvious failure mode. + $ret['tables'][] = 'content_models'; + $ret['joins']['content_models'] = [ 'LEFT JOIN', [ 'content_model = model_id' ] ]; + $ret['fields'][] = 'model_name'; + } + } } diff --git a/tests/phpunit/includes/Storage/McrReadNewRevisionStoreDbTest.php b/tests/phpunit/includes/Storage/McrReadNewRevisionStoreDbTest.php index cbae4c7eea..492d00c862 100644 --- a/tests/phpunit/includes/Storage/McrReadNewRevisionStoreDbTest.php +++ b/tests/phpunit/includes/Storage/McrReadNewRevisionStoreDbTest.php @@ -173,51 +173,84 @@ class McrReadNewRevisionStoreDbTest extends RevisionStoreDbTestBase { } public function provideGetSlotsQueryInfo() { - yield [ + yield 'no options' => [ [], + [ + 'tables' => [ + 'slots' + ], + 'fields' => [ + 'slot_revision_id', + 'slot_content_id', + 'slot_origin', + 'slot_role_id', + ], + 'joins' => [], + ] + ]; + yield 'role option' => [ + [ 'role' ], [ 'tables' => [ 'slots', 'slot_roles', ], - 'fields' => array_merge( - [ - 'slot_revision_id', - 'slot_content_id', - 'slot_origin', - 'role_name', - ] - ), + 'fields' => [ + 'slot_revision_id', + 'slot_content_id', + 'slot_origin', + 'slot_role_id', + 'role_name', + ], 'joins' => [ - 'slot_roles' => [ 'INNER JOIN', [ 'slot_role_id = role_id' ] ], + 'slot_roles' => [ 'LEFT JOIN', [ 'slot_role_id = role_id' ] ], ], ] ]; - yield [ + yield 'content option' => [ [ 'content' ], [ 'tables' => [ 'slots', - 'slot_roles', + 'content', + ], + 'fields' => [ + 'slot_revision_id', + 'slot_content_id', + 'slot_origin', + 'slot_role_id', + 'content_size', + 'content_sha1', + 'content_address', + 'content_model', + ], + 'joins' => [ + 'content' => [ 'INNER JOIN', [ 'slot_content_id = content_id' ] ], + ], + ] + ]; + yield 'content and model options' => [ + [ 'content', 'model' ], + [ + 'tables' => [ + 'slots', 'content', 'content_models', ], - 'fields' => array_merge( - [ - 'slot_revision_id', - 'slot_content_id', - 'slot_origin', - 'role_name', - 'content_size', - 'content_sha1', - 'content_address', - 'model_name', - ] - ), + 'fields' => [ + 'slot_revision_id', + 'slot_content_id', + 'slot_origin', + 'slot_role_id', + 'content_size', + 'content_sha1', + 'content_address', + 'content_model', + 'model_name', + ], 'joins' => [ - 'slot_roles' => [ 'INNER JOIN', [ 'slot_role_id = role_id' ] ], 'content' => [ 'INNER JOIN', [ 'slot_content_id = content_id' ] ], - 'content_models' => [ 'INNER JOIN', [ 'content_model = model_id' ] ], + 'content_models' => [ 'LEFT JOIN', [ 'content_model = model_id' ] ], ], ] ]; diff --git a/tests/phpunit/includes/Storage/McrRevisionStoreDbTest.php b/tests/phpunit/includes/Storage/McrRevisionStoreDbTest.php index 649e6928ac..af19f722fd 100644 --- a/tests/phpunit/includes/Storage/McrRevisionStoreDbTest.php +++ b/tests/phpunit/includes/Storage/McrRevisionStoreDbTest.php @@ -190,51 +190,84 @@ class McrRevisionStoreDbTest extends RevisionStoreDbTestBase { } public function provideGetSlotsQueryInfo() { - yield [ + yield 'no options' => [ [], + [ + 'tables' => [ + 'slots' + ], + 'fields' => [ + 'slot_revision_id', + 'slot_content_id', + 'slot_origin', + 'slot_role_id', + ], + 'joins' => [], + ] + ]; + yield 'role option' => [ + [ 'role' ], [ 'tables' => [ 'slots', 'slot_roles', ], - 'fields' => array_merge( - [ - 'slot_revision_id', - 'slot_content_id', - 'slot_origin', - 'role_name', - ] - ), + 'fields' => [ + 'slot_revision_id', + 'slot_content_id', + 'slot_origin', + 'slot_role_id', + 'role_name', + ], 'joins' => [ - 'slot_roles' => [ 'INNER JOIN', [ 'slot_role_id = role_id' ] ], + 'slot_roles' => [ 'LEFT JOIN', [ 'slot_role_id = role_id' ] ], ], ] ]; - yield [ + yield 'content option' => [ [ 'content' ], [ 'tables' => [ 'slots', - 'slot_roles', + 'content', + ], + 'fields' => [ + 'slot_revision_id', + 'slot_content_id', + 'slot_origin', + 'slot_role_id', + 'content_size', + 'content_sha1', + 'content_address', + 'content_model', + ], + 'joins' => [ + 'content' => [ 'INNER JOIN', [ 'slot_content_id = content_id' ] ], + ], + ] + ]; + yield 'content and model options' => [ + [ 'content', 'model' ], + [ + 'tables' => [ + 'slots', 'content', 'content_models', ], - 'fields' => array_merge( - [ - 'slot_revision_id', - 'slot_content_id', - 'slot_origin', - 'role_name', - 'content_size', - 'content_sha1', - 'content_address', - 'model_name', - ] - ), + 'fields' => [ + 'slot_revision_id', + 'slot_content_id', + 'slot_origin', + 'slot_role_id', + 'content_size', + 'content_sha1', + 'content_address', + 'content_model', + 'model_name', + ], 'joins' => [ - 'slot_roles' => [ 'INNER JOIN', [ 'slot_role_id = role_id' ] ], 'content' => [ 'INNER JOIN', [ 'slot_content_id = content_id' ] ], - 'content_models' => [ 'INNER JOIN', [ 'content_model = model_id' ] ], + 'content_models' => [ 'LEFT JOIN', [ 'content_model = model_id' ] ], ], ] ]; -- 2.20.1