Problem/Motivation

After #3551308: Combine single cardinality fields into a single database query when loading entities SqlContentEntityStorage executes one database query for the shared table (if there is one - e.g. node_field_data), and one query for fields in dedicated tables.

It should be possible to combine those queries into one - join on the shared table, then the individual field tables, and then extract the field results using very similar logic.

This will require deprecating one or probably both of the protected methods so that the logic can be combined in a single consolidated method.

The end result of this issue plus the previous two issues will be:

Initial loading query - unchanged

One database query to load data/revision/single cardinality fields.

One (optional) database query to load mulitiple cardinality fields.

So either 2 or 3 queries to load entities in all cases, instead of between 2 and over 100.

Steps to reproduce

Proposed resolution

Change the query for single cardinality fields so that it also loads values from the data/revision tables where necessary. This is a relatively small change to the query and allows us to drop (deprecate) ::loadFromSharedTables() entirely.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3561960

Command icon 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

catch created an issue. See original summary.

catch’s picture

Title: Combine the database queries in ::loadFromSharedTables() and ::loadFromDedicatedTables() » [PP-1] Combine the database queries in ::loadFromSharedTables() and ::loadFromDedicatedTables()
Status: Active » Postponed
Related issues: +#3564689: Combine multiple cardinality field loading into a single database query

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

alexpott’s picture

Title: [PP-1] Combine the database queries in ::loadFromSharedTables() and ::loadFromDedicatedTables() » Combine the database queries in ::loadFromSharedTables() and ::loadFromDedicatedTables()
Status: Postponed » Needs work

The blocking issues have gone in.

catch’s picture

Pushed an initial MR here (was an empty issue fork before, think I hit the button by mistake).

This is non-trivial to get right because of the various combinations of base/revision/data tables that entities may or may not have along with whether we're loading revisions or not. However I'm down to a handful of test failures in core's kernel entity tests so worth seeing what else breaks.

catch’s picture

A bit closer - all green on kernel tests although I get one content_moderation kernel test failure locally.

Performance tests are failing in the right way, but won't update those until everything else is passing in case the queries need to change again.

Haven't looked at the functional test failures yet.

catch’s picture

Title: Combine the database queries in ::loadFromSharedTables() and ::loadFromDedicatedTables() » [PP-1] Combine the database queries in ::loadFromSharedTables() and ::loadFromDedicatedTables()
Status: Needs work » Needs review

OK we're green now.

The MR is stacked on #3576129: Reduce calls to FieldDefinition::getColumns() which is RTBC, so marking PP-1 but also needs review because it would nice to get some high level reviews.

This removes a query from pretty much every entity load. It's less than the previous two issues it was postponed on, but it's also a similar-ish percentage of some requests because we've removed so many other queries now - e.g. 5-10% of database queries in various scenarios, higher percentages in the 'cool cache' cases.

The diff is quite big partly due to the stacked MR, but also partly because it removes an if statement when loading single cardinality fields, since we always do run that query now to get the base fields.

The tricky thing is the langcode handling, we have to handle both revision and non-revision queries in the same place, and the base table langcode logic is slightly different from the dedicated tables langcode logic, due to things like revisions and translations being added, removed etc. It's not dramatically more complex than the logic it's replacing but it was hard to get right because there's so many combinations with different entity schemas.

Not very happy with how long ::loadFromDedicatedTables() is becoming, maybe we can find some logic to move to helper methods.

Had to add a new parameter to ::loadFromDedicatedTables(), we might need to search contrib just to check that never actually gets called or overriden anywhere, doubt it does tbh.

We also need to decide if we're going to rename loadFromDedicatedTables() and also whether we do it in this issue or not.

catch’s picture

Status: Needs review » Needs work

Postgres and sqlite are green but MariaDb is unfortunately not https://git.drupalcode.org/project/drupal/-/jobs/8948781 - I saw this failure once or twice on my local but didn't realise it wasn't fixed by changes that fixed other tests.

This is another symptom of:

The tricky thing is the langcode handling, we have to handle both revision and non-revision queries in the same place, and the base table langcode logic is slightly different from the dedicated tables langcode logic, due to things like revisions and translations being added, removed etc. It's not dramatically more complex than the logic it's replacing but it was hard to get right because there's so many combinations with different entity schemas.

catch’s picture

Title: [PP-1] Combine the database queries in ::loadFromSharedTables() and ::loadFromDedicatedTables() » Combine the database queries in ::loadFromSharedTables() and ::loadFromDedicatedTables()
Status: Needs work » Needs review

Fixed the last remaining test failure and the blocker is in.

catch’s picture

Issue summary: View changes
catch’s picture

Issue summary: View changes

Added the ::loadFromSharedTables() deprecation.

catch’s picture

heddn’s picture

Issue tags: +Needs change record

Tagged for adding a CR. If we are going to rename loadFromDedicatedTables, we should do it here so it and shared tables both go away at the same time. Otherwise we risk contrib/custom extension points having to make changes multiple times.

catch’s picture

I checked usages:

https://search.tresbien.tech/search?q=loadFromDedicatedTables%20-f%3Acor...

https://search.tresbien.tech/search?q=loadFromSharedTables%20-f%3Acore&n...

Looking at the actual file: https://git.drupalcode.org/project/civicrm_entity/-/blob/4.0.x/src/CiviE... - this is a complete re-implementation of the storage with the same method name, no override, and it's called from another method in that class.

So there are zero usages in contrib, seems even less likely there is custom code around this for this particular change.

Part of the reason I didn't rename the method yet apart from laziness is not having great ideas. We could do loadFromTables maybe? Or loadFieldRecords ? But I'm not sure these are dramatically better than ::loadFromDedicatedTables()

I think if we rename, we also need to decide whether the old method will call the new one with the new code, or whether to restore the old code to it, which would be deprecated dead code then. Given the zero usages this probably doesn't matter in practice.

catch’s picture

Switching the CR tag to needs CR updates since I think we can update https://www.drupal.org/node/3562172 to cover all three issues - at least as long as this gets into 11.4 (which would be great considering how closely linked the changes are, and it'll be much easier to talk about the performance improvement).

heddn’s picture

Status: Needs review » Needs work

I guess I'm not a big fan of semantically renaming things on a whim. Certainly there aren't a lot (any?) implementations that override the method. So there wouldn't be a lot of repercussions, but the name of the method(s) here are pretty internal to the entity API. I don't think we need to change the name.

If we are going to only update the existing CR, we should add its link to the code instead of https://www.drupal.org/project/drupal/issues/3561960.

catch’s picture

Status: Needs work » Needs review
Issue tags: -Needs change record updates

Arghh I forgot all my previous arguments and ended up opening a separate CR. But now having written it, the previous CR was practical information for site owners instead they suddenly run into OOM https://www.drupal.org/node/3562172 and the new CR for a deprecation of a method we're pretty sure no-one is overriding or calling. So... that is probably fine, moving back to needs review.

Also had to rebase for performance test changes.

needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new91 bytes

The 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.

catch’s picture

Status: Needs work » Needs review

Rebased.

heddn’s picture

Status: Needs review » Reviewed & tested by the community

LGTM

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

This looks amazing. Really nice to get to this point. I have just one small concern about base_langcode and clashes with custom entities. I added some suggestions to the MR about possible mitigations.

catch’s picture

Status: Needs work » Reviewed & tested by the community

Had to fight both myself and gitlab but back to green. Ridiculous amount of MR activity for what is one alias rename but since that's all it actually is, moving back to RTBC.

alexpott’s picture

Version: main » 11.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed 4c021ea and pushed to main. Thanks!

  • alexpott committed 4c021eac on main
    perf: #3561960 Combine the database queries in ::loadFromSharedTables()...

catch’s picture

Status: Patch (to be ported) » Reviewed & tested by the community

Backport MR is up - only query counts in performance tests differed.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 529ca79 and pushed to 11.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • alexpott committed 529ca79c on 11.x
    perf: #3561960 Combine the database queries in ::loadFromSharedTables()...

quietone’s picture

Correct branch on the CR.

Status: Fixed » Closed (fixed)

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