Problem/Motivation

The content moderation views joins only on the revision ID. However, the table only has an index on both entity ID and revision ID. This was leading to very slow queries.

To reproduce, create a view of entity revisions and add a content moderation field.

Proposed resolution

Adding the join on entity ID and revision ID makes these queries faster, since the queries generated by views can now use the index.

Remaining tasks

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

CommentFileSizeAuthor
#8 3112916-8.patch2 KBSam152
#5 core-content_moderation-join-on-entity-id.patch795 bytesSam152
#4 core-content_moderation-join-on-entity-id.patch.txt795 bytesSam152
core-content_moderation-join-on-entity-id.patch795 bytesrealkevinoshea
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fatvkevin created an issue. See original summary.

Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented
Status: Needs review » Needs work
Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented
Status: Needs work » Needs review
Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented
FileSize
core-content_moderation-join-on-entity-id.patch.txt795 bytes

Hm, not sure why the patch can't be tested. Reuploading might help?

Really interesting issue. It would be really helpful to do some profiling on a large view before and after this change to validate it for committers.

I'm not sure testing this makes sense. Since functionally things should work the same before and after the patch, any test would just be copying details of the implementation into a test, so this is the kind of thing I'd be comfortable marking RTBC without explicit test coverage.

Lets see what the bot says.

Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented
Category: Bug report » Task
FileSize
core-content_moderation-join-on-entity-id.patch795 bytes

Wrong extension, lets see about this one.

Status: Needs review » Needs work

The last submitted patch, 5: core-content_moderation-join-on-entity-id.patch, failed testing. View results

realkevinoshea’s picture

realkevinoshea CreditAttribution: realkevinoshea at Ocelot commented

@Sam152: as it had been a couple months it took me a moment to remember the details of this. I think the issue is pretty clear from the explain. If I can find some time I could write a quick example code to trigger the issue. Take a look at this:

mysql> show indexes from content_moderation_state_field_revision;                                                                                              +-----------------------------------------+------------+----------------------------------------------------------+--------------+----------------------------+-----------+-------------+----------+--------+------+------------+---------+---------------+
| Table                                   | Non_unique | Key_name                                                 | Seq_in_index | Column_name                | Collation | Cardinality | Sub_part | Packed | Null | Index_type | Comment | Index_comment |
+-----------------------------------------+------------+----------------------------------------------------------+--------------+----------------------------+-----------+-------------+----------+--------+------+------------+---------+---------------+
| content_moderation_state_field_revision |          0 | PRIMARY                                                  |            1 | revision_id                | A         |      145477 |     NULL | NULL   |      | BTREE      |         |               |
| content_moderation_state_field_revision |          0 | PRIMARY                                                  |            2 | langcode                   | A         |      145964 |     NULL | NULL   |      | BTREE      |         |               |
| content_moderation_state_field_revision |          0 | content_moderation_state__lookup                         |            1 | content_entity_type_id     | A         |           2 |     NULL | NULL   | YES  | BTREE      |         |               |
| content_moderation_state_field_revision |          0 | content_moderation_state__lookup                         |            2 | content_entity_id          | A         |       40104 |     NULL | NULL   | YES  | BTREE      |         |               |
| content_moderation_state_field_revision |          0 | content_moderation_state__lookup                         |            3 | content_entity_revision_id | A         |      146193 |     NULL | NULL   | YES  | BTREE      |         |               |
| content_moderation_state_field_revision |          0 | content_moderation_state__lookup                         |            4 | workflow                   | A         |      139498 |     NULL | NULL   | YES  | BTREE      |         |               |
| content_moderation_state_field_revision |          0 | content_moderation_state__lookup                         |            5 | langcode                   | A         |      141901 |     NULL | NULL   |      | BTREE      |         |               |
| content_moderation_state_field_revision |          1 | content_moderation_state__id__default_langcode__langcode |            1 | id                         | A         |       37624 |     NULL | NULL   |      | BTREE      |         |               |
| content_moderation_state_field_revision |          1 | content_moderation_state__id__default_langcode__langcode |            2 | default_langcode           | A         |       38319 |     NULL | NULL   |      | BTREE      |         |               |
| content_moderation_state_field_revision |          1 | content_moderation_state__id__default_langcode__langcode |            3 | langcode                   | A         |       38319 |     NULL | NULL   |      | BTREE      |         |               |
| content_moderation_state_field_revision |          1 | content_moderation_state__09628d8dbc                     |            1 | workflow                   | A         |           3 |     NULL | NULL   | YES  | BTREE      |         |               |
| content_moderation_state_field_revision |          1 | content_moderation_state_field__uid__target_id           |            1 | uid                        | A         |           3 |     NULL | NULL   |      | BTREE      |         |               |
+-----------------------------------------+------------+----------------------------------------------------------+--------------+----------------------------+-----------+-------------+----------+--------+------+------------+---------+---------------+
12 rows in set (0.00 sec)

In the `content_moderation_state__lookup` index the revision ID is the 3rd in the sequence. So originally that index could not be used because the content_entity_id was not present in the join.

Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented
Status: Needs work » Needs review
FileSize
3112916-8.patch2 KB

Nice, thanks for the explanation.

I argued against including assertions like this in the views tests, but since they already exist, using for the new join.

Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented
Status: Needs review » Reviewed & tested by the community

I ran some profiling on the queries generated by views before and after the patch applied:

+----------+------------+------------------------+
| Query_ID | Duration   | Query                  |
+----------+------------+------------------------+
|        1 | 0.00159200 |  (query without patch) |
|        2 | 0.00009200 |  (query with patch)    |
+----------+------------+------------------------+

This seems like a dramatic speed up. I'm comfortable RTBCing, since I did not write the patch and the test fix is essentially duplicating the implementation.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: 3112916-8.patch, failed testing. View results

Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented
Status: Needs work » Reviewed & tested by the community

Random fail.

jonnyeom’s picture

jonnyeom CreditAttribution: jonnyeom as a volunteer commented

Thanks for the patch!
This is a good one.

+1 RTBC

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented
Issue summary: View changes
godotislate’s picture

godotislate CreditAttribution: godotislate commented

+1 RTBC

Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented

Realising I wrote the original patch in #5, but it was @ocelotkevin who came up with the fix 🤦

I probably shouldn't have RTBC'd, but #12 and #15 have been posted since.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: 3112916-8.patch, failed testing. View results

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
CreditAttribution: bkosborne at Princeton University commented
Status: Needs work » Reviewed & tested by the community

+1 RTBC. I tested this on a very complex view.

Before patch, query execute time was ~170 seconds.
After patch, query execute time was ~3 seconds.

The results of the view were identical before and after the patch.

Sam152 credited corneboele.

Sam152 credited yovince.

Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented
Related issues: +#3097303: Poor performance when using moderation state views filter

Thanks for following up and testing @bkosborne.

Adding users who were credited in #3097303: Poor performance when using moderation state views filter to this issue, since they were solving the same problem but in slightly different ways.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: 3112916-8.patch, failed testing. View results

godotislate’s picture

godotislate CreditAttribution: godotislate commented
Status: Needs work » Reviewed & tested by the community

Random test failure.

jantoine’s picture

jantoine CreditAttribution: jantoine as a volunteer commented

+1 RTBC. I just ran into this issue with a query taking 6+ minutes to complete. With the patch from #8, the query takes less than .62 seconds.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Nice find. Committed/pushed to 9.1.x and cherry-picked back to 8.9.x, thanks!

  • catch committed dcf1f22 on 9.1.x 
    Issue #3112916 by Sam152, ocelotkevin, corneboele, jantoine, yovince,...

  • catch committed 01d91a5 on 9.0.x 
    Issue #3112916 by Sam152, ocelotkevin, corneboele, jantoine, yovince,...

  • catch committed b37ec33 on 8.9.x 
    Issue #3112916 by Sam152, ocelotkevin, corneboele, jantoine, yovince,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented

while it's nice to have the join, if we want to sort by moderation state some modes of MySQL (defaults for certain versions) require the sort column to be in the select statement. This is a constraint oracle is adding into MySQL releases.

See core/modules/views/src/Plugin/views/field/EntityField.php
"Column is not in query; add a sort on it (without adding the column)."

More info on sql mode changes in 5.7:
https://dev.mysql.com/doc/refman/5.7/en/sql-mode.html#sql-mode-changes

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented
Related issues: +#2862041: Provide useful Views filters for Content Moderation State fields