Problem/Motivation
Follow-up from #3607938: Chunking of Op in loadFromDedicatedTables causes Cartesian multiplication.
When there are lots of high-cardinality fields, the number of rows returned is too high, so to get this back down to one query we'll need a different approach.
UNION is unlikely to help because all tables need the same number of columns and column types, and fields can have different column types for value, or multiple field-specific columns.
Might be something we can do with GROUP BY if that's still indexed.
Steps to reproduce
Proposed resolution
Add a join to a sub select which uses a union to build up a list of the all the deltas for all the multiple cardinality fields. It provides a list of possible deltas, which every field table can be compared against, whereas they can't be compared against each other because we can't know what has comparable (or any) data or not. By joining on delta, all values from every field are loaded into the same record per-delta, which prevents the cartesian multiplication altogether - you get a row-per-delta-per-language and nothing else. Also we can use this table to sort on and remove sorting from PHP.
Remaining tasks
User interface changes
None
Introduced terminology
N/a
API changes
None
Data model changes
None
Release notes snippet
N/a
| Comment | File | Size | Author |
|---|
Issue fork drupal-3608184
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
catchSomewhat of an idea.
The reason we get the cartesian multiplication is because we don't join on delta, but we can't join on delta because we can't know which field table is going to have all the deltas we need to load all the deltas for all of the fields.
Have a somewhat test-passing branch that first queries to get the maximum delta of any field per chunk, and once it's got that field table, joins everything on the deltas from that field table.
This causes all the delta 0, 1, 2, 3, 4 etc. to be returned together instead of every combination of those deltas.
Getting that field is an additional query per chunk, that's still a maximum of two queries per chunk instead of a maximum of 30 queries per chunk but it feels wasteful.
Not implemented but what I think might work:
If we created a new table, it can have a single column, 'delta', starting from 0 and reaching 1024 or 8096 or whatever arbitrary delta limit we want to set. We could always join on that table because we'll know it'll be there. No extra query, but the efficiency would be the same. All we need is sequential integers from 0-maximum in there.
Comment #4
catchThings I thought about before/after this that probably would not work at all:
1. UNION / UNION ALL - this requires all columns to be of the same type and the same number of columns, which is not the case for field tables. If we group field table by type we might be able to workaround that, but an entity with three multiple cardinality fields of three types would not benefit at all. Also not sure UNION performance would be better than a join
2. A temp table doing the same thing as the one added here - we'd have to constantly check if the table exists or not and recreate it.
3. MySQL sequence object - not something you can join against
4. GROUP BY - doesn't work because we'd want to group by delta and we don't have a reliable delta field to group on
Comment #5
catchOK this is green.
The extra table is a bit painful, but per #4 I looked at multiple different approaches that wouldn't require actually making a new table and they all came up short. The actual code change here is not very much overall. Also the table allows us to remove the PHP sort on delta which was an annoyance of the previous approach.
The table will automatically get more rows (+100 more than absolutely required each time) when entities are saved for new sites.
For existing sites I set it to have 4000 rows with a $settings override. A quick poll in slack showed that sites can easily have over 100, or 'several hundred' for field deltas (commerce coupons and paragraphs were mentioned). I'd somewhat hope once a site goes over 4000 field deltas it will start to hit other limits (memory, max_post_vars etc.) first. We could always increase it, but if we did we'd need to chunk the insert in the update at some point.
Comment #6
catchComment #7
catchComment #8
catchFound a problem with this approach - it results in languages * rows in the
entity_delta_jointable - e.g. you first of all get the useful rows, then you get an endless parade of useless rows for all the deltas that don't match where all the field values are null. Once I added the update to create 4000 rows I discovered this...Also I'm not entirely sure the original report in #3607938: Chunking of Op in loadFromDedicatedTables causes Cartesian multiplication: was 100% correct. What I think we get is fields * language * delta (one row for every valid delta of every field in each language), which is a lot of rows, but it's not $fields * fields * delta * language.
edit: what we can probably do is page the results. So do a range on entity_delta_join.delta 0-20, if there are more than 20 deltas, get another 20 rows. In the vast majority of situations the number of queries will be the same but the results will be capped.
Comment #11
alexpottI had a feeling that there must be a more elegant approach that creating a table with an arbitrary limit of deltas. So I worked using claude to use a sub-select with unions so that we can achieve the same result. The idea is a to create a spine with all the possible deltas for an entity and to then join to that. I've pushed up this approach in a new branch. Also we only do this when necessary - if there is only one multiple cardinality field we do not do any unnecessary work.
Comment #12
alexpottI've tested this by running the new \Drupal\KernelTests\Core\Entity\MultiCardinalitySpineTest() and counting the number of rows processed. ON 11.4.0 this shows that \Drupal\Core\Entity\Sql\SqlContentEntityStorage::loadMultipleCardinalityFields() processes 15 rows whereas on the union-delta-spine branch is only processes the expected 7. Thats 4 rows for the english deltas and 3 rows for the german deltas.
Comment #13
alexpottComment #15
alexpottBased on the last commit to the spine MR that reduced the amount of table coming from the base table I realised we did not need the base table at all. So I've create another MR on top of the spine MR to show how we can completely remove the base table join. See https://git.drupalcode.org/project/drupal/-/merge_requests/16227
Comment #16
catchLatest approach looks great, we still get the consistent deltas to join on but without the crime against databases which my approach would have introduced, being able to skip the join on the base table is a nice by-product.
Since this isn't any of my code any more, I think I'm good to RTBC here.
Also given this is 100% internal to the new methods we could probably backport to an 11.4 patch release too.
Comment #19
catchI'm fairly sure that the original 'cartesian multiplication' diagnosis on #3607938: Chunking of Op in loadFromDedicatedTables causes Cartesian multiplication was incorrect, so re-titling this issue.
Pretty sure what we had in 11.4.0 was fields * delta * language rows (e.g. a row for every field data in each language, whereas that issue claims fields * fields * delta * language rows (a row for every possible combination of field deltas).
However, that's still a lot more rows than we want to have.
Comment #20
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. The merge request has merge conflicts and cannot be merged. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #21
catchRebased for performance test conflicts.
Comment #22
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #23
joachim commented> Have a somewhat test-passing branch that first queries to get the maximum delta of any field per chunk, and once it's got that field table, joins everything on the deltas from that field table.
Wacky idea: add a column to the entity base table called 'max_delta'. Save a value to it whenever the entity is saved.
Comment #24
catch@joachim I think that probably would have allowed my approach to work if we'd added it to the revisions table and updated all revisions on all entities to have it.
However @alexpott's UNION query in the latest MR sidesteps this entirely - the UNION query figures out the deltas, then we join on that UNION, giving us the minimum possible result set.
In the process of rebasing now.
Comment #25
catchComment #26
godotislateI looked at this before, but I guess I forgot to comment.
Have we profiled this change? I understand that we're processing fewer rows, but are the subqueries and union faster than multiple queries?
Comment #27
catchI ran an EXPLAIN using one of the queries from the Umami performance test against an Umami database, and it looks good:
e.g. everything is indexed/const. The rows consulted matches the number of rows in the result, not any more than that. The query against the UNION uses a filesort, but that's only against the derived rows from the UNION (e.g. the deltas), not the entity/field table.
I don't have an 11.x site with lots of data and the right combination of fields at the moment to check timings against a populated db.
Comment #28
nicxvan commentedOk I have a site with some entities that have large numbers of multi value fields, this is what I found.
I tested a node that had one field with 187 items and another with 80.
I visited the page with the patch and without.
I used web profiler.
With the patch:
78MiB 2135 queries 1.71ms
Showing rows 0 - 24 (187 total, Query took 0.0026 seconds.)
Without the patch stock 11.4.4
98MiB 2137 queries 1.69ms
Showing rows 0 - 24 (80 total, Query took 0.0012 seconds.)
Showing rows 0 - 24 (187 total, Query took 0.0012 seconds.)
Showing rows 0 - 0 (1 total, Query took 0.0013 seconds.)
One field has 114192 rows.
One field has 105768 rows.
Total seems to be 2 less queries and 20 less MiB. For this run.
I then switched back and forth a couple times clearing cache.
I consistently got 80MiB without the patch and 78MiB with the patch.
Comment #29
nicxvan commentedHere is the explain:
Comment #30
godotislateWas the first one of 98 vs 78 an outlier then? 2MB is still an improvement, but it's much different from 20.