Reduce dataloader queries #3192
Open
+149
−46
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yndajas
force-pushed
the
reduce-dataloader-queries
branch
2 times, most recently
from
2d84d96 to
7e7c61f
Compare
brucebolt
reviewed
Mar 11, 2025
There was a problem hiding this comment.
The code changes all seem ok to me.
But I've run the code locally and queried the prime minister page, which gives the role_appointments links in a different order to those in the existing content item. It could be the order has changed since my old dump of the database, but I'd suggest trying this code on integration to double check the order has stays the same when querying against the same database.
yndajas
force-pushed
the
reduce-dataloader-queries
branch
from
7e7c61f to
50c28a4
Compare
mike3985
force-pushed
the
reduce-dataloader-queries
branch
from
50c28a4 to
7894c42
Compare
Co-authored-by: Mike Patrick <mike.patrick2@digital.cabinet-office.gov.uk>
I'm not 100% sure about this change. We're experimenting with SELECTing subsets of an edition's columns and JOINed-tables' columns to improve the performance of our GraphQL resolvers. When you SELECT JOIN columns, Active Record helpfully just seems to dynamically add them as attributes to the model you're working with, which means that, e.g. SELECT editions.*, documents.content_id, documents.locale FROM editions INNER JOIN documents ON document.id = editions.document_id would result in instances of Edition with their own, native #content_id and #locale attributes. In those specific examples, the existing `delegate` doesn't appear to acknowledge these attributes and still attempts to lazy-load the edition's document to fetch them. Since that lazy-loading kinda defeats the point of the approach we're experimenting with, here's an alternative to `delegate` that first checks for pre-existing attributes of the same name. There could be better approaches than this worth exploring in the long run if we do stick with this SELECTing optimisation. (I don't think it's wise to use these Active Record models in such dramatically different ways in different parts of the codebase.)
We explicitly order the results by position, but we'll test this separately. We changed the Active Record code to use a UNION query, which has meant interweaving edition and link set links, breaking the order that these tests were implicitly depending on Co-authored-by: Mike Patrick <mike.patrick2@digital.cabinet-office.gov.uk>
We encountered an issue when creating an edition link separately to its edition (i.e. not using the `links_hash` transient/argument), whereby both a link set association was created even when explicitly creating an edition association Co-authored-by: Mike Patrick <mike.patrick2@digital.cabinet-office.gov.uk>
We found that the previous eager loading approach wasn't using joins effectively, so we're more manually crafting the queries to join in an efficient way. We also found that some associations we're still lazy loaded after the initial queries - this new approach avoids that issue For now we're selecting all columns from the editions table. We'll reduce the selections down to a subset in a later change The change to using a UNION affects the order of the resulting editions, so we've added a test to document our expectations Co-authored-by: Mike Patrick <mike.patrick2@digital.cabinet-office.gov.uk>
yndajas
force-pushed
the
reduce-dataloader-queries
branch
from
7894c42 to
70f046a
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Trello
This optimises(?) the database queries in
LinkedToEditionsSourceandReverseLinkedToEditionsSource, reducing them down to one query each (in practice, one query per level of depth in the GraphQL query). This is a change en route to selecting a subset of columns - we'll open a separate pull request for that changeWe're looking into the actual performance of this change at the minute but thought it was worth opening the pull request since the core bones are in place
This application is owned by the publishing platform team. Please let us know in #govuk-publishing-platform when you raise any PRs.
Follow these steps if you are doing a Rails upgrade.