book_node_load() ignores the 'book_allowed_types' and trigger excessive SQL queries when a lot of nodes are arbitrarily loaded on the page.

Proposed patch: do not do the SQL query that load book contents on nodes for which the book type is not allowed.

Potential risk: if a node type is a book then is not anymore, some data may stall and remain silent in the book database table, although there is very poor chances that happens on a production site.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pounard’s picture

Issue tags: +Performance

Tagging.

pounard’s picture

So I guess this needs review after all, because book module is weird. I don't really understand why is there two variables, allowed types and child type?

But aside of that, patch really is working and saving us a looooooot of SQL queries.

pounard’s picture

pounard’s picture

Issue summary: View changes

Please someone review this, this seem to do its job quite well and would be a nice performance improvement.

amoebanath’s picture

Status: Needs review » Reviewed & tested by the community

Working nicely as needed

pounard’s picture

Wow, that's fast ! Thank you.

mcdruid’s picture

I could be missing something but it looks like this applies to D9/10 too.

book_node_load() calls \Drupal\book\BookManager::loadBookLinks() which calls \Drupal\book\BookOutlineStorage::loadMultiple() which looks for matching nids in the book table without checking book_allowed_types.

I've not dug into that in any more depth, but it seems like a fairly similar situation.

Having said that, this seems like a big performance win for D7 and I'm inclined to say we should commit it and file a followup for D9/10 rather than work the other way around and potentially never get this improvement in to D7 (@pounard has already been very patient!).

  • mcdruid committed 9801138 on 7.x
    Issue #2070807 by pounard: book_node_load() ignores the '...
mcdruid’s picture

Status: Reviewed & tested by the community » Fixed

FWIW in D9/10 I think book_node_load would check each node's type with book_type_is_allowed($type) before looking up its nid.

In terms of the backport policy, committing this to D7 will not introduce a regression in functionality for any sites migrating from D7 to D9/10, so I'm going to commit it and we can file that followup "forward-port" for consideration in D9/10.

Finally, LOL #7 :)

Thank you!

poker10’s picture

we can file that followup "forward-port" for consideration in D9/10.

Actually I have found the D9/10 issue, but it seems that the approach there will be a bit different, because it uses the hook_ENTITY_TYPE_load, so the only possibility is to loop thru all nodes and filter them by types.

https://www.drupal.org/project/drupal/issues/3096844

This D7 patch used the recommended approach mentioned also in the hook_node_load documentation example (the $types variable).

alrh’s picture

This patch broke some of the books on our sites.
It assumes that a book only contains node types from 'book_allowed_types' but the description for this setting mentions: Users with the "Administer book outlines" permission can add all content types.

poker10’s picture

Thanks for reporting this, we will take a look at that.

If possible, I think there could be a (temporary) workaround for sites experiencing this, if they allow also other content types in book outlines. This should "fix" the problem for the content types currently not allowed, but still used in book outlines.

  • mcdruid committed b189931 on 7.x
    Revert "Issue #2070807 by pounard: book_node_load() ignores the '...
mcdruid’s picture

For now we've reverted this in preparation for #3326836: hotfix release of Drupal 7 (7.94?).

Perhaps for a future release we could consider putting the check behind a flag (defaulting to off) so that sites that use the book module but only with book_allowed_types could opt-in to the optimisation.

mcdruid’s picture

Status: Fixed » Needs work

Back to NW for the above.

joseph.olstad’s picture

@mcdruid,
I think rather than an opt-in, we should just improve the patch.

All that would have to be done would be to invoke the original logic for those with the following permission:
"Administer book outlines"
For everyone else, the "ahem, clearing 9 years of flem in the throat" *new* optimised logic would be invoked making relevant db queries correctly targetted.

I applaud the fact that this is included in D7.93 and the quick feedback we got as an opportunity to improve it quickly for D7.95.

The D7.93 was a very ambitious release and I'm putting it into production tomorrow evening on two sites that don't have 'book' types, however I have another site with books and uses the great book_access module also (still no available Symfonised Drupal version of book_access, it's a Drupal 7 only module, I highly recommend this module).