Updating to 10.5.3 causes gateway timeouts on revisioned content

I confirm that downgrading to 10.5.2 fixes the db issue, thanks.

If you have the slow query log enabled on the database it should show some detail. Knowing which specific queries are slow you can execute EXPLAIN <the slow SQL query> on the database server to see the query plan. That will shine some light on what went wrong with #2950869: Entity queries querying the latest revision very slow with lots of revisions.

What is its query plan?

i have written on original issue comment
that this function should never been introduced to the core QueryInterface .
it doesn't make any logic, the core Query implementation has the concept of current revision and not latest revision .
it is only after you enable workflow and content moderation that the latest revision concept becomes logical .

only when you are editing the entity does latest revision become relevant, but that is achieved using EntityRespository::getActive .

alternatively if we must insist on this latest revision, then add new column to entity base table by adding key to the Attribute ContentEntityType::$entity_keys which in addition to `revision` also saves `latest_revision` .
so node table for example node_field_data which have in addition to vid, also latest_vid .

the only places i found which calls latestRevision are in jsonapi and workspaces and ContentEntityStorageBase::getLatestRevisionId

Ah, nevermind.

Here is an example of database query that is causing problems with 10.5.3:

SELECT base_table.vid AS vid, base_table.nid AS nid FROM node_revision base_table INNER JOIN node_field_data node_field_data ON node_field_data.nid = base_table.nid WHERE (base_table.vid IN (SELECT MAX(base
_table.vid) AS "expression" FROM node_revision base_table GROUP BY base_table.nid)) AND (node_field_data.nid = '114');

but this same query should exist just the same in 10.4.x too, so why should 10.5.3 suddenly start making problem ?
wrong, the MAX / group by was added only in 10.5.x . commit ad6d4462ba6fc9335a59f419f195643960f2b8aa

i also think jsonapi should not provide support for WORKING_COPIES_REQUESTED parameter in entity collection query .
drupal designed to handle each entity latest revision in individual page for editing and the EntityListBuilder for example doesn't support loading a list of latest revisions .
only using views can you add filter for latestTranslationaffected revision .

this is the change in json entity resource between 10.5.3 and 10.4.x
wrong diff, removed

from comment #4

Here is an example of database query that is causing problems with 10.5.3:

SELECT base_table.vid AS vid, base_table.nid AS nid 
FROM node_revision base_table 
INNER JOIN node_field_data node_field_data ON node_field_data.nid = base_table.nid 
WHERE (base_table.vid IN (SELECT MAX(base
_table.vid) AS "expression" FROM node_revision base_table GROUP BY base_table.nid)) AND (node_field_data.nid = '114');

this query is very wrong, since node_field_data has only one entry with vid which points to the current published revision .
so it is completely pointless to join it with latest vid value, since the latest does not exist in node_field_data unless it has been published and in that case we don't need to calculate it since node_field_data already has the latest vid

very sorry, i didn't read the sql carefully, it joins on nid value and not vid .

i suggest as workaround to try and turn off the jsonapi latestRevision call on client side

$working_copy_identifier = 'rel' . VersionNegotiator::SEPARATOR . 'working-copy'
$defaults[static::WORKING_COPIES_REQUESTED] = $resource_version_identifier === $working_copy_identifier;

but i am not sure how, maybe include in the javascript GET request parameter `?resourceVersion=rel:working-copy`

Having an unlimited SELECT subquery is not a good idea, this resulting query for 2 million paragraphs items takes 2 seconds to run:

SELECT base_table.revision_id AS revision_id, base_table.id AS id
FROM
paragraphs_item_revision base_table
INNER JOIN paragraphs_item_field_data paragraphs_item_field_data ON paragraphs_item_field_data.id = base_table.id
WHERE (base_table.revision_id IN (SELECT MAX(base_table.revision_id) AS expression
FROM
paragraphs_item_revision base_table
GROUP BY base_table.id)) AND (paragraphs_item_field_data.id = '6091304');

EXPLAIN output shows the subquery as the only part that does not use a DB index:

Uploading the revert for 10.5.x as stable patch file for composer patches.

For comparison the reverted query, it takes 0.2ms to run (factor 1000 faster):

SELECT base_table.revision_id AS revision_id, base_table.id AS id
FROM
paragraphs_item_revision base_table
LEFT OUTER JOIN paragraphs_item_revision base_table_2 ON base_table.id = base_table_2.id AND base_table.revision_id < base_table_2.revision_id
INNER JOIN paragraphs_item_field_data paragraphs_item_field_data ON paragraphs_item_field_data.id = base_table.id
WHERE (base_table_2.id IS NULL) AND (paragraphs_item_field_data.id = '6091304')

EXPLAIN output showing that an index can be used for all parts:

we can have this super simple fast query if we keep track of latest revision shown in attached update_latest.diff

SELECT base_table.revision_id AS revision_id, base_table.id AS id
FROM
paragraphs_item_field_data base_table
INNER JOIN  paragraphs_item_revision revision_table ON revision_table.revision_id = base_table.latest_revision_id
WHERE paragraphs_item_field_data.id = '6091304'

but this needs more thorough book keeping work and adding the latestRevisionKey definition to revisionable entities trait .

related to racking the latest revision id

@ericpoir you have provided the sql which causes the problem,
but can you please describe the execution context whether this happens part of jsonapi call to load a collection of objects or it happens in some other page ?

Just to note here: the commit is also present in 11.2.x from 11.2.4 https://git.drupalcode.org/project/drupal/-/commit/6528a0f3b3211041481a1...

I have opened a merge request with the fix previously provided in #2950869: Entity queries querying the latest revision very slow with lots of revisions.

Two of the commits are making the tests pass. I am not certain that the test was wrong. It may be that the test was correct as it was and the code is not doing what it should.

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.

The patch does apply to 10.5.x as intended.

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.

The IS points to this commit as the cause https://git.drupalcode.org/project/drupal/-/commit/ad6d4462ba6fc9335a59f... but the code changes in the MR are in a different part of the code.

IIUC the commit changed the outcome of calling ->latestRevision() but this MR is changing from latestRevision to allRevisions with a range. While this may fix the performance issue, isn't that just masking other performance issues using latestRevision?

I guess the main issue is that from https://www.drupal.org/project/drupal/issues/3491274 we did not port the condition:
WHERE $id_field = base_table.$id_field
And because of it when there are a lot of entities we have all of them returned in

WHERE ("base_table"."vid" IN 
  (
    SELECT MAX(base_table.vid) AS "expression"
    FROM "node_revision" "base_table"
    GROUP BY "base_table"."nid"
  )
)

So changing it to

WHERE ("base_table"."vid" IN 
  (
    SELECT MAX(base_table.vid) AS "expression"
    FROM "node_revision" "base_table"
    WHERE nid = "base_table"."nid"
    GROUP BY "base_table"."nid"
  )
)

should resolve an issue.
Let me prepare an MR with proposed change.

p.s.

Tried to get rid of "group by"

WHERE ("base_table"."vid" IN 
  (
    SELECT MAX(base_table.vid)
    FROM "node_revision" "base_table"
    WHERE nid = "base_table"."nid"
  )
)

and it cause an error:

Warning: array_flip(): Can only flip string and integer values, entry skipped in Drupal\Core\Entity\ContentEntityStorageBase->loadMultipleRevisions() (line 667 of core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php).
Drupal\Core\Entity\ContentEntityStorageBase->loadMultipleRevisions() (Line: 634)

so not applicable.

attaching a patch.
Could someone test it on Drupal installation with a lot of paragraphs?

@dmitry.korhov Are you able to add tests to merge request 13526?

i suspect this is not correct since some queries does not include condition on entity id field at all .

looks like this where clause behaves like a form of join .

@liam-morland,
I would be happy to add tests, could you help what kind of tests we need for that change?

looks like this where clause behaves like a form of join .

@yakoub,
While it works like join by key but practically it just reduces the results from

    SELECT MAX(base_table.vid) AS "expression"
    FROM "node_revision" "base_table"
    GROUP BY "base_table"."nid"

To only one, which is equal to target entity_id.

And we could consider it as a workaround and not as a real solution for found root cause.
It is ugly, yeah, but, at least, it does not broke other parts or APIs while making performance better by reducing amount of ids loaded into query `WHERE ("base_table"."vid" IN )`

WHERE ("base_table"."vid" IN (SELECT MAX(base_table.vid) AS "expression"
FROM
"node_revision" "base_table"
GROUP BY "base_table"."nid")) 

Ideally we should avoid using "group by" with single `nid`.
Unfortunately, the solution I've tried:

    SELECT MAX(base_table.vid)
    FROM "node_revision" "base_table"
    WHERE nid = "base_table"."nid"

throws an error.
I believe it is more correct approach than where + group by but have no time to deep dive into issue:

Warning: array_flip(): Can only flip string and integer values, entry skipped in Drupal\Core\Entity\ContentEntityStorageBase->loadMultipleRevisions()

But feel free to look into it and address the issue :)

With select used instead of expression a query will be even faster for databases containing huge amount of entities.

`ContentEntityStorageBase->loadMultipleRevisions` doesn't call latestRevision, so the error not making sense to me .
infact ContentEnttiyStorageBase doesn't call entity query at all, but build low level select query .
i will need to dive into it .

@dmitry.korhov The test should be something that surfaces the problem. It would fail without your change and pass with it.

@liam-morlan i have also asked in #17 about the context of this performance problem, it is not clear which data and which pages causes this problem to surface .
as far as i can see, the only context which calls latestRevision is jsonapi .

In my case we found that the performance of the `/admin/content` view (which has been customized for the site) was affected with this change.

In our case the patch from #29 didn't improve the performance, but the revert patch from #13 fixed the performance issue.

views has separate (duplicate) implementation : core/modules/views/Plugin/views/filter/LatestRevision.php

views has separate (duplicate) implementation

The implementation in core/lib/Drupal/Core/Entity/Query/Sql/Query.php is the one that is called in my case, I checked and the query function in the views implementation is not being called.

Hit this issue today when upgrading. Performance on jsonapi calls tanked for site with millions of nodes and revisions. Revert patch from #13 fixed the performance issue.

We are seeing the same kind of terrible performance on a site with a lot of custom content entities. This is bringing down the site.

The SQL is basically the same:

SELECT "base_table"."vid" AS "vid", "base_table"."id" AS "id"
FROM
"raft_metadata_revision" "base_table"
INNER JOIN "raft_metadata" "raft_metadata" ON "raft_metadata"."id" = "base_table"."id"
WHERE ("base_table"."vid" IN (SELECT MAX(base_table.vid) AS "expression"
FROM
"raft_metadata_revision" "base_table"
GROUP BY "base_table"."id")) AND ("raft_metadata"."id" = '75467')

Applying the change from merge request 13526 did change the slow SQL we are seeing but it's still very slow, so I do not think this is the right fix. The slow SQL now looks like:

SELECT "base_table"."vid" AS "vid", "base_table"."id" AS "id"
FROM
"raft_metadata_revision" "base_table"
INNER JOIN "raft_metadata" "raft_metadata" ON "raft_metadata"."id" = "base_table"."id"
WHERE ("base_table"."vid" IN (SELECT MAX(base_table.vid) AS "expression"
FROM
"raft_metadata_revision" "base_table"
WHERE (id = base_table.id)
GROUP BY "base_table"."id")) AND ("raft_metadata"."id" = '247092')

Couldn't this be done with a self-join instead of a MAX subquery?

Applying the revert patch from klausi resulted in a dramatic improvement in load. I think the right thing to do here is to revert the change instead of trying to fix it when it seems like there is some fundamental misunderstanding of the performance or, at least, lack of any automated testing to catch performance regressions

I updated one MR for 10.5.x to have the revert patch - maybe it should target 10.6.x to actually get merged?
https://git.drupalcode.org/project/drupal/-/merge_requests/13526

I also created a MR for 11.x
https://git.drupalcode.org/project/drupal/-/merge_requests/13606

These don't fully revert the original commit since leaving the extra test coverage should be preferred.

> I think the right thing to do here is to revert the change

In the interests of fixing the performance regression, yes.

But there should be a follow-up issue to remove the subquery entirely, which will improve performance further.

The subquery is needed because a JOIN to itself can also be slow when you have many revisions. I analyzed that in #2950869: Entity queries querying the latest revision very slow with lots of revisions where this regression was introduced.

We have 2 problems I see:
* The self join is very bad for entities with thousands of revisions
* The subquery is very bad for sites with millions of entities

So it sounds like one way of doing the query is slow with large numbers of revisions and the other is slow with large numbers of nodes. Could this be sped-up by adding indexes? Do we need to have a way of detecting or configuring the choice between the two query styles based on which would be faster?

Thanks for the explanation @klausi. That sort of thing would be useful to have in code comments!

The other way would be to store the revision ID of the latest revision somewhere.

The latest revision ID does sound like a reasonable thing to cache.

You can't cache the latest revision ID for every entity ID. Maybe something changed, but I thought the revision ID in the entity base table was always the latest?

also, we only have 427,961 custom entities, which I would not say is that big a number. The change to a subquery (even with the fix to add the ID where condition) was bringing down our DB server.

(note - this is with mysql 8.0)

Doesn't seem to be mentioned, but are most encountering this running MySQL/MariaDB? Running PostgreSQL myself, but have had this issue brought to my attention, being vaguely reminded of an issue that occurred somewhat recently in another/associated project, regarding MySQL/MariaDB choosing to run subqueries as "correlated" subqueries, which is to say, effectively running the subquery separately for each row. The other project in question: https://github.com/Islandora/islandora/pull/1089/files

I wonder if a similar approach here might be sufficient, to add the list of `->fields()` bit to the subquery, to prevent it from being executed as a "correlated' query?

EDIT: Realized I missed part of the other solution somewhat, not only the `->fields()` bit, but also maybe having to nest the subquery inside another `->select()`, to more completely prevent it from being correlated?

you all talking about optimizing the sql, but none are explaining why do you need the latest revision in your application to begin with ?!
just turn it off ! use the current revision instead of latest revision and all your performance problems will be gone .
see my comment above how to turn off latestrevision in jsonapi

Can we not fix this by changing the group by to where and join on the ID field? Then we will not need to table scan.

Group by (HEAD)

mysql> explain SELECT base_table.vid AS vid, base_table.nid AS nid FROM node_revision base_table INNER JOIN node_field_data node_field_data ON node_field_data.nid = base_table.nid WHERE (base_table.vid IN (SELECT MAX(base_table2.vid) AS expression FROM node_revision base_table2 group by base_table.nid)) AND (node_field_data.nid = 1);
+----+--------------------+-----------------+------------+-------+----------------------------------------------+-----------+---------+-------+------+----------+------------------------------+
| id | select_type | table | partitions | type | possible_keys | key | key_len | ref | rows | filtered | Extra |
+----+--------------------+-----------------+------------+-------+----------------------------------------------+-----------+---------+-------+------+----------+------------------------------+
| 1 | PRIMARY | base_table | NULL | ref | node__nid | node__nid | 4 | const | 1 | 100.00 | Using where; Using index |
| 1 | PRIMARY | node_field_data | NULL | ref | PRIMARY,node__id__default_langcode__langcode | PRIMARY | 4 | const | 1 | 100.00 | Using index |
| 2 | DEPENDENT SUBQUERY | base_table2 | NULL | index | NULL | node__nid | 4 | NULL | 1 | 100.00 | Using index; Using temporary |
+----+--------------------+-----------------+------------+-------+----------------------------------------------+-----------+---------+-------+------+----------+------------------------------+
3 rows in set, 2 warnings (0.01 sec)

Where (3548313-change-group-by-to-where)

mysql> explain SELECT base_table.vid AS vid, base_table.nid AS nid FROM node_revision base_table INNER JOIN node_field_data node_field_data ON node_field_data.nid = base_table.nid WHERE (base_table.vid IN (SELECT MAX(base_table2.vid) AS expression FROM node_revision base_table2 where base_table2.nid = base_table.nid)) AND (node_field_data.nid = 1);
+----+--------------------+-----------------+------------+------+----------------------------------------------+-----------+---------+---------------------------+------+----------+--------------------------+
| id | select_type | table | partitions | type | possible_keys | key | key_len | ref | rows | filtered | Extra |
+----+--------------------+-----------------+------------+------+----------------------------------------------+-----------+---------+---------------------------+------+----------+--------------------------+
| 1 | PRIMARY | base_table | NULL | ref | node__nid | node__nid | 4 | const | 1 | 100.00 | Using where; Using index |
| 1 | PRIMARY | node_field_data | NULL | ref | PRIMARY,node__id__default_langcode__langcode | PRIMARY | 4 | const | 1 | 100.00 | Using index |
| 2 | DEPENDENT SUBQUERY | base_table2 | NULL | ref | node__nid | node__nid | 4 | drupal8alt.base_table.nid | 1 | 100.00 | Using index |
+----+--------------------+-----------------+------------+------+----------------------------------------------+-----------+---------+---------------------------+------+----------+--------------------------+
3 rows in set, 2 warnings (0.00 sec)

Running this against a larger db with paragraphs... (277723 rows in paragraphs_item_revision)

HEAD

explain SELECT base_table.revision_id AS revision_id, base_table.id AS id FROM paragraphs_item_revision base_table INNER JOIN paragraphs_item_field_data paragraphs_item_field_data ON paragraphs_item_field_dat
a.id = base_table.id WHERE (base_table.revision_id IN (SELECT MAX(base_table.revision_id) AS expression FROM paragraphs_item_revision base_table GROUP BY base_table.id)) AND (paragraphs_item_field_data.id = '1232');
*************************** 1. row ***************************
           id: 1
  select_type: PRIMARY
        table: paragraphs_item_field_data
   partitions: NULL
         type: ref
possible_keys: PRIMARY,paragraph__id__default_langcode__langcode
          key: paragraph__id__default_langcode__langcode
      key_len: 4
          ref: const
         rows: 1
     filtered: 100.00
        Extra: Using index
*************************** 2. row ***************************
           id: 1
  select_type: PRIMARY
        table: base_table
   partitions: NULL
         type: ref
possible_keys: paragraph__id
          key: paragraph__id
      key_len: 4
          ref: const
         rows: 2
     filtered: 100.00
        Extra: Using where; Using index
*************************** 3. row ***************************
           id: 2
  select_type: SUBQUERY
        table: base_table
   partitions: NULL
         type: range
possible_keys: paragraph__id
          key: paragraph__id
      key_len: 4
          ref: NULL
         rows: 19766
     filtered: 100.00
        Extra: Using index for group-by
3 rows in set, 1 warning (0.00 sec)

MR

explain SELECT base_table.revision_id AS revision_id, base_table.id AS id FROM paragraphs_item_revision base_table INNER JOIN paragraphs_item_field_data paragraphs_item_field_data ON paragraphs_item_field_data.id = base_table.id WHERE (base_table.revision_id IN (SELECT MAX(subquery_base_table.revision_id) AS expression FROM paragraphs_item_revision subquery_base_table WHERE base_table.id = subquery_base_table.id)) AND (paragraphs_item_field_data.id = '1232');
*************************** 1. row ***************************
           id: 1
  select_type: PRIMARY
        table: paragraphs_item_field_data
   partitions: NULL
         type: ref
possible_keys: PRIMARY,paragraph__id__default_langcode__langcode
          key: paragraph__id__default_langcode__langcode
      key_len: 4
          ref: const
         rows: 1
     filtered: 100.00
        Extra: Using index
*************************** 2. row ***************************
           id: 1
  select_type: PRIMARY
        table: base_table
   partitions: NULL
         type: ref
possible_keys: paragraph__id
          key: paragraph__id
      key_len: 4
          ref: const
         rows: 2
     filtered: 100.00
        Extra: Using where; Using index
*************************** 3. row ***************************
           id: 2
  select_type: DEPENDENT SUBQUERY
        table: subquery_base_table
   partitions: NULL
         type: ref
possible_keys: paragraph__id
          key: paragraph__id
      key_len: 4
          ref: db.base_table.id
         rows: 14
     filtered: 100.00
        Extra: Using index

And the query is noticeably faster...

[EDIT] Improving formatting of the explain output.

Thanks a lot Alex, your merge request makes perfect sense.

I can confirm that all query parts are now always hitting the index, as you also posted in your garbled explain output.

Unfortunately I cannot reproduce the original performance problem anymore since we have upgraded from MariaDB 10.5 to 10.11 (standard in Debian 12). It looks like MariaDB 10.11 got smarter about the problematic query in the meantime, it executes in 0.3ms on my 2 million paragraphs database.

Anyone struggling with this issue could also try upgrading MariaDB/MySQL to fix the performance problem.

But the merge request is a good fix, so I think we should do it in any case.

I would set this to RTBC, the only thing I would recommend is a good comment in code so that this does not get changed by accident again. Any good ideas for a comment?

Here is a start:

Using a sub-query ensures all query parts hit the index, greatly improving speed. Do not change this without testing with thousands of revisions and millions of entities.

you can use flag `\G` at end of you query to produce vertical output of the explain command
https://dev.mysql.com/doc/refman/8.4/en/mysql-commands.html

you guys know sql, but don't know drupal
the latest revision is not needed in most applications since the current revision already exists in the field data table .

@yakoub we know Drupal very well, thanks for asking. Paragraphs and other contrib modules make use of the latest revision, which we cannot easily change. You can also see that in the test queries me and Alex have posted.

@yakoub thanks for the \G idea... will update the comment to make it easier to read.

@yakoub is also not wrong that we should be trying to eliminate usage of the latestRevision() functionality - but that's hard and takes time given the usages - so if we can fix the query we should. And the good news is I think we can with the current MR.

From my knowledge of Drupal; there moves to refactor content moderation on top of workspaces and move away from the current approach.

@klausi i did grep latestRevision on paragraphs code and didn't find anything .
so far, only place i found was jsonapi .
if you expose api from core to contrib then of course they will end up depending on it
but this is wrong, any contrib module who require some exotic sql query should maintain it on their own .
i have written this on the original issue and no one agreed and here we are couple of years latter with this performance bug .

anyway, @alexpott and @klausi of course thank you for you contribution and as you say you must patch it now for quick fix
but this also an opportunity to research who is actually using this method ? and ask people who report the problem to provide more information

@yakoub latestRevision starts getting used in param converters the moment you have content moderation installed for any moderated content type - see \Drupal\content_moderation\Routing\ContentModerationRouteSubscriber::setLatestRevisionFlag ... the way that content moderation and core works means that paragraphs doesn't directly have to call the code for it to be used.

@alexpott the param converter rely on EntityRepostory::getActive and not this QueryInterface::latestRevision method, so no this is not the place in my opinion .

update : it ends up calling ContentEntityStorageBase::getLatestRevisionId which calls QueryInterface::latestRevision
but if all we are doing is get latest for just a single entity id, then this whole sub query aggregation max is completely not needed !!!

@yakoub yes but then we'll need new methods. Let's just fix the query to be performant and move on.

The main cause of the latestRevision query in core, at least it running frequently, is the 'load_latest_revision' route param upcaster, which is added to node routes by content_moderation module as @alexpott points out.

#3486378: [Plan] Allow for / implement simplified content workflow with workspaces is one or two steps away from making that a non-issue (by making content_moderation depend on workspaces, which tracks draft revisions rather than only dealing with the 'latest', and therefore can remove that upcaster from those routes), but there will still be contrib uses around to get rid of after that and the handful of individual calls to it in core.

#3549946: Use simple query mode for entity queries with limit 1 and no offset may be relevant here since it's trying to simplify single-entity queries.

Looks like we have a random test fail with ckeditor, I could not find the rerun tests button in Gitlab.

Comment looks good and since this was passing before I think we can RTBC.

@catch again i have to disagree, since the `load_latest_revision` on route is called for single entity edit route or show latest revision tab
in such cases it goes through EntityRepositroy::getActive method which iterates on multiple ids and calls individually getLatestRevisionId
if all we are doing is call individually getLatestRevisionId, then just change that method NOT to use QueryInterface::latestRevision !
and no performance problem going to happen when you are just editing a single entity page
something else is causing this, and in my opinion it is jsonapi .

didn't mean to change status

Catch's comment and re-opening of the original issue kinda made my heart skip a beat since that patch saved a site dying under the load of content moderation. I'll keep this short and just say glad y'all got to a solution so quick, thanks!

i don't know if i am still not comprehending this correctly, but i think the correct solution is to refactor ContentEntityStorageBase::getLatestRevisionId not to use QueryInterface::latestRevision .
since all it does is get latest revision for just a single entity id, then the query for this is much much more straight forward and simple

@yakoub we also have to consider the usage in \Drupal\jsonapi\Controller\EntityResource::getCollection() and \Drupal\workspaces\WorkspacePublisher::getDifferringRevisionIdsOnTarget() both of which can target more than just a single entity.

... and that's just the core usage... there is plenty of usage in contrib too.

@yakoub I do not disagree that the introduction of method was flawed... and yes I'm definitely one of the people responsible for its introduction. But hindsight is often 20:20...

well jsonapi ::getCollection should not fetch latest revision !
jsonapi needs to reflect Core behavior and nowhere in core does a collection of latest revision being loaded .
nonethless i have written above that there seems to be client side url parameter which disables jsonapi latest revision .
so any "decoupled" website experiencing load problems needs to make sure they load current revision and not latest revision
in my opinion all those decoupled website are not even aware they are loading latest revision and have no need for it !

as for workspace, i don't know what this getDifferringRevisionIdsOnTarget but why should we destroy performance of 99% of drupal websites upgrade just for 1% who are actually using workspace ?!
let workspace maintain their own query, that is what i written above .

JSONAPI will only use the latest revision code if the json API request is made using the rel:working-copy query string parameter. In order to support this functionality and working with content moderation the JSONAPI module must use latestRevision(). Maybe we should not have implemented that functionality but I'm guessing it was implemented for a reason - to hazard a guess - it is probably used when trying to preview things - but I'm not sure as I did not implement it.

even if we consider there are already lots of contrib modules depending on this method, we can start thinking about mitigating its use and limiting it .
it is great we already have better sql suggestions to fix it,
but also lets start changing ContentEntityStorageBase::getLatestRevisionId not to use it at all when it is not needed .

if some 10% of websites are using some esoteric contrib module which uses latestRevision then they will get this new sql fix
but for the rest of 90% that don't need any of those contrib modules, lets understand how can we already protect them from latestRevision

working with content moderation the JSONAPI module must use latestRevision()

NO .
why should a decoupled client side form need to load a collection of latest revision entities ?
same as you edit single entity in regular drupal html page, so should decoupled application make separate rest calls to present an entity edit form .
people who work in decoupled application like to think they now can do "magic" stuff, but that is not how it works
frontend developers still need to follow same crud rules like normal drupal does .
you are not going to change how backend logic works just for your fancy javascript forms and expect core to support it .

@yakoub please file an issue to change ContentEntityStorageBase::getLatestRevisionId and do the work if you want.

We should still go ahead here.

FWIW I think maybe your percentage estimates need some work and thought. You will only run into this problem with core if you; use workspaces, use content moderation, or use jsonapi and have a request that adds the query param... AND have a site with a very large number of entities. This is not going to be the percentage of Drupal sites you suggest.

You will only run into this problem with core if you; use workspaces, use content moderation, or use jsonapi and have a request that adds the query param... AND have a site with a very large number of entities

this is exactly what i was asking from the beginning, for all who have reported this problem on upgrade then how exactly does this gets triggered ?
if what is happening that getLatestRevisionId runs subquery on the WHOLE node table and gets stuck on editing a single node page, then the most releavant use case here for those users indeed to fix getLatestREvisionId and not bother at all with QueryInterface::latestRevision since they should not been using it at all on there website .

@alexpott, this seems to not be true:"JSONAPI will only use the latest revision code if the json API request is made using the rel:working-copy query string parameter. "
We saw a huge impact from the SQL change on JSONAPI requests and we never use that query string parameter.

the requests that were taking > 60 sec look like this:

/jsonapi/raft_metadata/sds_external?filter[reference][condition][path]=association.id&filter[reference][condition][value]=6690bd54-23b9-4bae-9b3e-f17f96d9f9fa&filter[field_external_sds_file][condition][path]=field_external_sds_file&filter[field_external_sds_file][condition][operator]=IS NOT NULL&include=field_external_sds_file

i would like to note as well the whole point of content moderation is NOT to expose the latest revision until it have been approved .
so if jsonapi responds with the latest revision without it have been explicitly requested then this is logical error and may even be a security breach .

   // If the request is for the latest revision, toggle it on entity query.
    if ($request->get(ResourceVersionRouteEnhancer::WORKING_COPIES_REQUESTED, FALSE)) {
      $query->latestRevision();
    }

if there is specific problem with paragraphs versioning then maybe it can be fixed using custom logic in EntityReferenceFieldItemListInterface::referencedEntities
or define custom storage handler to Paragraph entity
i am not sure, since i didn't work much with paragaph .

but the point is, there are other better solutions to all those contrib modules than this QueryInterface::latestRevision

You're getting to the root of the problem and why the previous issue was open for so long and why this is so difficult to fix.

Really there's two problems
1) its painfully generalized for all entity types
2) The assumption of content moderation/workflow/etc processes in core for that the "active" revision is the latest revision and mix and match that concept in their interfaces. And then contrib that builds on top of these inherits that complexity as Klausi pointed out #64.

Really 1 shouldn't matter because I'm pretty sure we're all in agreement we shouldn't even be using it as "latest" doesn't really have any useful meaning.

And 2... Well unraveling that is an entire community project which is why I was hopeful about the Community Initiative mentioned. But also it's why Alex requested refactoring things happen in its own follow up issue. Everyone wants it, but we also understands how much work that's going to be once you start lifting up various rocks and finding all the bugs and assumptions hidden underneath.

the most critical code is this
EDIT : i am wrong, i tested jsoapi and it serves the published version by default
(sorry i just didn't have ready development environment to test this)

// If the request is for the latest revision, toggle it on entity query.
    if ($request->get(ResourceVersionRouteEnhancer::WORKING_COPIES_REQUESTED, FALSE)) {
      $query->latestRevision();
    }

taken from jsonapi\Controller\EntityResource::getCollection
if i am correct jsonapi will serve the most recent revision by default
while the correct behavior should be serve the working copy by default
unless i don't understand what `WORKING_COPIES_REQUESTED` means

if this is the case then changing this rule will fix the performance problems and make all this subquery latest revision fix irrelevant .
can someone confirm we can change jsonapi default behavior ?

so any "decoupled" website experiencing load problems needs to make sure they load current revision and not latest revision
in my opinion all those decoupled website are not even aware they are loading latest revision and have no need for it !

@yakoub,
Please don’t get me wrong, but your statement about all decoupled Drupal applications is not correct.
There is an official way to load a target version using rel:working-copy and rel:latest-version — see the documentation here: Drupal JSON:API Revisions: https://www.drupal.org/docs/core-modules-and-themes/core-modules/jsonapi...

This is what we use to fetch "draft" revisions for the preview feature under our content previewer role, and to fetch published-only entities (returning a 403 error when an entity has no published revision) for our production API role.

NO .
why should a decoupled client side form need to load a collection of latest revision entities ?
same as you edit single entity in regular drupal html page, so should decoupled application make separate rest calls to present an entity edit form .
people who work in decoupled application like to think they now can do "magic" stuff, but that is not how it works
frontend developers still need to follow same crud rules like normal drupal does .
you are not going to change how backend logic works just for your fancy javascript forms and expect core to support it .

@yakoub,
The latest revision != the default revision.
There are valid use cases for viewing, for example, archived entities from within API.
Drupal provides these APIs, so it should also guarantee that they work efficiently and reliably.

can someone confirm we can change jsonapi default behavior ?

Of course not, that would be a backward compatibility (BC) break.
It should be handled through versioning; for example, by introducing a new “default” behavior and deprecating the old one, while keeping it enabled until (for instance) Drupal 12.

@dmitry.korhov the main point here is a *collection* of latest revisions of entities and not just a single entity latest revision .
certainly it is valid, but as developer you need to be aware that of you request through jsonapi a collection of latest revision of entities and that call makes your website break because of performance and the amount of revisions stored on your database, then don't make such requests .

when loading single entity latest revision, then all this sub query group by sql is not needed
so problem is this strange requirement of filtering a list or collection of entities to its latest revision values .

i want to make clear my opinion that currently it is not known what context and condition exactly triggers this performance problem
if we are talking about single entity edit page (which does require loading latest revision ...) then the fix is to change ContentEntityStorageBase::getLatestRevisionId NOT to call the method QueryInterface::latestRevision
while if we are talking about jsonapi loading a collection of latets revision, then people need to be aware this comes with performance cost and should make sure their decoupled application doesn't make such calls if the database can not handled them .

@pwolanin

We saw a huge impact from the SQL change on JSONAPI requests and we never use that query string parameter.

one possible explanation is that rel:latest-version gets injected into the request server side by some contrib module without client side being aware of it .

@dimitry.korhov
according to your knowledge about meaning of "working version", does this code look correct to you ?

// If the request is for the latest revision, toggle it on entity query.
    if ($request->get(ResourceVersionRouteEnhancer::WORKING_COPIES_REQUESTED, FALSE)) {
      $query->latestRevision();
    }

it seems to call the latest revision aka working version, if the parameter does NOT exist in the request !
EDIT : ooops, i just realized that FALSE is the second argument which is the default value in case parameter does not exist
i keep making mistakes, sorry

Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!

Will need a backport MR for 10.6.x

Pushed up a 10.5.x branch that'll work for 10.6.x too.

@pwolanin are you able to post the slow database query that goes along with the 60s request from #85? edit: and a backtrace for how it ends up running?

Whilst reading this issue through after @catch's commit (thanks), I had a think about #52 and think that this might be the reason why some sites where affected and others not. And the solution that we've merged doesn't fix the situation. When doing a query on a collection the new query we've added here will definitely by correlated. It is going to be way more efficient than the previous query but it is still going to be run lots of times. Maybe we should try the solution identified on #52. To that end I've opened #3555720: Latest revision subquery optimisation which reverts the changes here and does that.

@pwolanin / somebody else with an affected site: it would be immensely helpful if you could compare the two solutions on a site which has the performance issue. I've have some suspicions that #3555720: Latest revision subquery optimisation will be better for the collection query @pwolanin says is their problem in #85.

Found out why jsonapi sites have been affected by this even if they're not using rel:working-copy... @catch's asked a great question...

Also why does json:api performance test even run this query at all? It's not sending working-copy

Well thats because of code in \Drupal\jsonapi\JsonApiResource\ResourceObject::buildLinksFromEntity() which calls $entity->isLatestRevision() which ends up doing this query...

Opened #3555732: Don't check if we're on the latest revision before adding the working copy link.

So fun days... I think we should consider rolling this back. I've created a db with 40000+ nodes and all nodes with at least 100 revisions (some with over 1000) and can confirm that this fix introduces a correlated sub-query that the original fix was designed to avoid and this fix risks slows queries too.

I've tried to reproduce the slow queries reported on MariaDB 10.5 as well and it is not happening - even with 4 million revisions... I think we should explore @kristiaanvandeneynde suggestions on #3555720: Latest revision subquery optimisation.

I started working on a revert but then thought about it more:

Original query - slow with lots of revisions per entity but did not bring sites down.

#2950869: Entity queries querying the latest revision very slow with lots of revisions fixed original query for lots of revisions per entity, but very bad with lots of entities to the point it could bring sites down.

Status quo following the commit here: Still possibly slow for lots of revisions per entity, although not necessarily worse and possibly better than the original query, fixes the regression we introduced in #2950869: Entity queries querying the latest revision very slow with lots of revisions.

I think the original state and the current state are better than the regression, since we don't have indication that the current state is worse than the original slow query, I think we should stick with where we are until we work out something better in #3555720: Latest revision subquery optimisation.

Additionally #3555732: Don't check if we're on the latest revision before adding the working copy link will (dramatically) reduce the frequency that this runs on sites with JSON:API enabled, which will be a small improvement once everything is fixed for all possible versions, but could be a bigger improvement in the interim.

#109 makes sense. Thanks for workign this through. I just pushed an MR that implements a new idea with a specific optimisation for the latest revision ID stuff. I'd love to land #3555720: Latest revision subquery optimisation before the next set of releases as I think it delivers the best performance for latest revision queries where a single ID is involved and where many are.

who ... needs ... this latestRevision ?
no one has yet to answer me .
maybe i am just wrong to try and contribute at all to drupal .
only place i found was ContentEntityStorageBase::getLatestRevisionId
but in that case you don't need a subquery whatever since it is just one single entity id that you want the latest revision .

other than that, why bother ? no one should ever try and load a *collection* to latest revision of multiple entities .

@yakoub you've had several responses and you've just brushed them off with non-sequiturs.

I replied to you in #3548313-71: Updating to 10.5.3 causes gateway timeouts on revisioned content with an example of where we're trying to completely factor out use of this query in content_moderation module (no lookups of latest revision at all in the common code paths, not even for individual entities), and also opened #3555732: Don't check if we're on the latest revision before adding the working copy link which would completely remove some calls from JSON:API. Your reply to that was 'I have to disagree' although it was not clear with what you were disagreeing.

More to the point, adding a separate API to get only the latest revision of an entity ID and potentially drop the case of allowing queries against multiple entities and deprecating the current API is something we could only do in a minor release, and we want to fix the regression for 10.5.3 sites asap, without re-introducing the slow query we had prior to 10.5.3, ideally this week.

Per #109 I've committed the 10.6.x MR and cherry-picked it to 10.5.x so that 10.x is on a level playing field with 11.x

We can continue in #3555720: Latest revision subquery optimisation which is a significant improvement again over this issue. And also #3555732: Don't check if we're on the latest revision before adding the working copy link which is down to one or two test fails.

@catch good work, makes lots of sense, thank you!

i apologize for my comment in #111

Reporting @neclimdul's comment from the other issue as it points to a fix that I think would have worked to fix this before we even attempted to fix this here:

Tested this on a site. It was causing serious performance problems on a node with ~20k revisions. The slow queries that where showing up didn't make sense either b/c they should have been the optimized case.

After discussing with Alex on slack, and confirming the poor performance went away when the database was copied to a different server(by way of a database dump) we decided this was likely an issue with the storage causing it to choose a bad subquery logic.

After hours I ran the following. A warning for those that follow, after hours was a good instinct. The queries ran immediately but then took down the site as all other queries touching those tables where queued until it finished running background tasks on the table.

ANALYZE TABLE node_field_data PERSISTENT FOR ALL;
ANALYZE TABLE node_revision PERSISTENT FOR ALL;

This seems to have fixed the problem and the slow node that was timing out now loads immediately and the query the slow query returned instantly.

See #3555720-38: Latest revision subquery optimisation

Note this explains what @klausi experienced too - the database upgrade probably recreated the table statistics and then the slow query was fixed.

I'm not sure how best to get this information to people. I'm going to open a follow-up issue to discuss this.

Created the follow-up #3556351: Stale table statistics can result in extremely slow queries

i would like to write summary as far as i understand :

1. you are maintaining an sql query in drupal core which is not used anywhere else in drupal core but only by some contrib modules .
2. no, route param converter `load_latest_revision` and content moderation DOES NOT require this query .
   since route parameter concerns a single entity id and in that case you don't need a sub query sql at all, but a simple "order by"
   in other words ContentEntityStorageBase::getLatestRevisionId should not call latestRevision but just do an orderBy desc .
3. this query seems to have been used in drupal contrib modules, but we don't know what those modules trying to do
   most likely the developers of those contrib module don't comprehend how drupal revisions work and do not need the latest revision at all,not only this, but if you load latest revision without checking content moderation state then you are risking exposing draft data and breaching data privacy .
4. last, even if you want to maintain this functionality in core, you are doing it very very wrong using a sub query .
   because the correct way is to add new column to entity data table which tracks the latest revision .
   currently the entity data table tracks only the current published revision and you should add similar column for the latest revision .