Problem/Motivation
Follow-up from #3551308: Combine single cardinality fields into a single database query when loading entities.
We can apply the same treatment to multiple cardinality fields as single cardinality fields to save even more database queries.
The UNION ALL in the issue summary of that issue doesn't look necessary, just needs a left join.
Steps to reproduce
Proposed resolution
Load all multiple cardinality fields at once. Looks like:
EXPLAIN SELECT node_field_data.nid AS nid, node_field_data.langcode AS langcode, node__field_ingredients.field_ingredients_value AS field_ingredients_value, node__field_ingredients.delta AS field_ingredients_delta, node__field_recipe_category.field_recipe_category_target_id AS field_recipe_category_target_id, node__field_recipe_category.delta AS field_recipe_category_delta, node__field_tags.field_tags_target_id AS field_tags_target_id, node__field_tags.delta AS field_tags_delta, node__layout_builder__layout.layout_builder__layout_section AS layout_builder__layout_section, node__layout_builder__layout.delta AS layout_builder__layout_delta FROM node_field_data node_field_data LEFT OUTER JOIN node__field_ingredients node__field_ingredients ON node__field_ingredients.entity_id = node_field_data.nid AND node__field_ingredients.langcode = node_field_data.langcode AND node__field_ingredients.deleted = 0 LEFT OUTER JOIN node__field_recipe_category node__field_recipe_category ON node__field_recipe_category.entity_id = node_field_data.nid AND node__field_recipe_category.langcode = node_field_data.langcode AND node__field_recipe_category.deleted = 0 LEFT OUTER JOIN node__field_tags node__field_tags ON node__field_tags.entity_id =
node_field_data.nid AND node__field_tags.langcode = node_field_data.langcode AND node__field_tags.deleted = 0 LEFT OUTER JOIN node__layout_builder__layout node__layout_builder__layout ON node__layout_builder__layout.entity_id = node_field_data.nid AND node__layout_builder__layout.langcode = node_field_data.langcode AND node__layout_builder__layout.deleted = 0 WHERE (node_field_data.nid IN (1)) AND (node_field_data.langcode IN ("en", "es", "und", "zxx"));
+----+-------------+------------------------------+------------+------+----------------------------------------------+--------------------------------------+---------+-------------+------+----------+--------------------------+
| id | select_type | table | partitions | type | possible_keys | key | key_len | ref | rows | filtered | Extra |
+----+-------------+------------------------------+------------+------+----------------------------------------------+--------------------------------------+---------+-------------+------+----------+--------------------------+
| 1 | SIMPLE | node_field_data | NULL | ref | PRIMARY,node__id__default_langcode__langcode | node__id__default_langcode__langcode | 4 | const | 2 | 40.00 | Using where; Using index |
| 1 | SIMPLE | node__field_ingredients | NULL | ref | PRIMARY | PRIMARY | 5 | const,const | 26 | 100.00 | Using where |
| 1 | SIMPLE | node__field_recipe_category | NULL | ref | PRIMARY | PRIMARY | 5 | const,const | 1 | 100.00 | Using where |
| 1 | SIMPLE | node__field_tags | NULL | ref | PRIMARY | PRIMARY | 5 | const,const | 2 | 100.00 | Using where |
| 1 | SIMPLE | node__layout_builder__layout | NULL | ref | PRIMARY | PRIMARY | 5 | const,const | 1 | 100.00 | Using where |
+----+-------------+------------------------------+------------+------+----------------------------------------------+--------------------------------------+---------+-------------+------+----------+--------------------------+
5 rows in set, 1 warning (0.00 sec)
Remaining tasks
User interface changes
Introduced terminology
API changes
Data model changes
Release notes snippet
Issue fork drupal-3564689
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
catchEntity and field module kernel tests all pass, let's see how the rest looks.
Comment #4
catchGot some errors on CI I couldn't reproduce locally even with supposedly the same mysql and php combination, but pretty sure it's ordering.
Added a sort on delta ASC for each field table, still uses an index but it requires a temporary table and filesort - probably just on the rows that are returned, but not ideal.
However, that does fix all the test failures (except for the obvious performance test failures because those aren't updated yet) on CI.
Comment #5
catchWe don't need to do #4 because we can ksort() the results. MR should be green now.
Since the logic here is very similar, it should actually be possible to combine the single and multiple cardinality queries into one as well. However once we do that, we end up with all columns for all fields in all rows of the result, which would slightly increase the risk of hitting max_allowed_packet as discussed in the previous issue. I think that all that's necessary for this is removing the split and using the multiple cardinality logic only, might give it a go in a separate MR.
edit: tried this, but there is indeed too much duplication in the results, let's stick with the original idea here.
Comment #8
catchMR is green and ready for review.
After this, we can do #3561960: Combine the database queries in ::loadFromSharedTables() and ::loadFromDedicatedTables() which should remove one more query-per-entity-load (e.g. probably 3-5 queries per page in core tests), which would mean all entities are loaded with a maximum of three queries - the initial load query, the single cardinality fields, and the multiple cardinality fields.
Comment #9
catchComment #10
catchThis can go on the same change record as https://www.drupal.org/node/3562172 - tagging to update that assuming this lands for 11.4.x
Comment #11
catchComment #12
catchNearly 10% reduction in queries on the 'front page cool cache' performance test justifies major I think.
Comment #13
godotislateLooks like build failed on composer authentication failure to github, probably just needs a re-run.
One question on the MR about the performance test for now. Quick review of the rest seems to make sense, but still needs a more in depth look later.
Comment #14
catchKicked off a fresh pipeline and tried to answer the question on the MR.
Comment #16
godotislateA couple more questions on the MR, but I think this is close.
Comment #17
godotislatelgtm!
Comment #18
alexpottCommitted and pushed b68de1a5f60 to main and c840c458bc3 to 11.x. Thanks!
Time to update the CR.
Comment #22
catchUpdated the CR: https://www.drupal.org/node/3562172
Comment #23
catch