[MB-8026] Optimizing FetchMany in query builder #6594

reggieriser · 2021-05-07T16:17:03Z

Description

This PR begins to address two performance problems with our current implementation of FetchMany in the query builder:

FetchMany uses Pop's Eager not EagerPreload so it is making way more requests than it should.
FetchMany does not have a way to tell it you want to load zero associations. So it loads ALL associations.

Reviewer Notes

There are a lot of files changed in this PR, but the changes break down as follows:
- NewQueryAssociation was remapped to the new NewQueryAssociationPreload in some places where preloading made sense and seemed to work fine.
- Changed some (accidental) default Eager calls (which loaded all associations) to nil to load no associations. This required changes to the query builder to support the nil.
- A good bit of the file changes are due to adding an explicit fk_id to has_many and belongs_to relationships so we don't have to rely on Pop's guesses for the field names (it was sometimes guessing wrong when preloading).
- Added ability to specify preloading as part of setting up query associations in the query builder (see pkg/services/query/)
I tried to changeFetchOne as well to preload associations, but I got a lot of failures when doing so; we'll have to work through those if we want to make that change. FetchOne probably needs a refactoring anyway as it is loading all associations every time and there's no way to change that behavior on a case-by-case basis. The good news is that since FetchOne is fetching a single base record, default eager loading of associations doesn't cause an explosion of additional queries like it can be with a big list of base records (like FetchMany might have).
The most obvious performance improvements are found in the admin interface list views. In particular, the transportation office listing (and the office drop down when creating a new office user) should be a lot faster. You can see the difference in SQL queries generated on dev by comparing the admin interface on master with this branch.
I ran into a bug in Pop when trying to preload a has_many association with a pointer-based foreign key (like we have when preloading MTOServiceItems.ReService for a shipment). I made a PR for Pop and it's been merged and is in Pop 5.3.4, but we have other issues it looks like we have to solve first before moving to that version.

Setup

The tests should hopefully catch any major issue here; I had to avoid a preload or deleting the default eager in a few places when tests started failing.
Most of the changes were around the admin interface, so try it and make sure you don't see anything new that's broken or data that's missing.

Code Review Verification Steps

The requirements listed in Querying the Database Safely have been satisfied.
Request review from a member of a different team.
Have the Jira acceptance criteria been met for this change?

References

Jira story for this change
Relevant Slack thread

…gainst EagerPreload naming assumptions

…EagerPreload

robot-mymove · 2021-05-07T16:26:19Z

	Messages
📖	🔗 MB-8026

Generated by 🚫 dangerJS against 1b17fdd

gidjin

Changes look good to me! Admin interface seemed to be working as expected. TSPP data page is much faster and a lot less queries. Thanks for the clear Reviewer notes that really helped. I left a couple questions but I don't think it needs to hold up approval form my end. Might be good to get some other eyes on this if possible.

gidjin · 2021-05-07T23:24:57Z

pkg/handlers/internalapi/mto_shipment.go

@@ -182,6 +182,12 @@ func (h ListMTOShipmentsHandler) Handle(params mtoshipmentops.ListMTOShipmentsPa
 	queryFilters = []services.QueryFilter{
 		query.NewQueryFilter("move_id", "=", moveTaskOrderID.String()),
 	}
+
+	// In some places, we used this unbound eager call accidentally and loaded all associations when the


Should we prefix this with TODO? Just wondering if that would help us find it later, which may not be true.

Sure, I just added it.

gidjin · 2021-05-07T23:30:50Z

pkg/models/re_domestic_linehaul_price.go

@@ -26,8 +26,8 @@ type ReDomesticLinehaulPrice struct {
 	UpdatedAt             time.Time       `json:"updated_at" db:"updated_at"`

 	// Associations
-	Contract            ReContract            `belongs_to:"re_contract"`
-	DomesticServiceArea ReDomesticServiceArea `belongs_to:"re_domestic_service_area"`
+	Contract            ReContract            `belongs_to:"re_contract" fk_id:"contract_id"`


Looking at all these fk_id additions and I can't help but wonder if there are guidelines on when to add fk_id to a model column. Or maybe we should always do it?

There's this section from the Pop documentation. After working on several EagerPreload efforts, I think it's probably better to always be explicit with the fk_id than to let Pop figure it out. Eager seems to have a different method for guessing the right field than EagerPreload as I've gotten errors with EagerPreload given the same model setup that don't happen with Eager. The most recent thing I saw was that it tried to make transportation_offices singular by changing it to something like transportation_officex (and that only seemed to happen with EagerPreload)

The use of fk_id is also mentioned in our Wiki as a workaround when the foreign key name is different from the relation name.

gidjin · 2021-05-07T23:36:03Z

pkg/services/query/query_builder.go

+	if associations == nil {
+		return query
+	}
+
+	if associations.Preload() {
+		return query.EagerPreload(associations.StringGetAssociations()...)
+	}


Is there a test suite for the query builder? If so is it feasible to add a test or is this covered enough by other testing?

I added some tests that get to this via FetchMany.

gidjin

Thanks for adding tests! 🚀

reggieriser added 6 commits May 5, 2021 19:19

Added in ability to not load any associations for FetchMany

591a3de

New associations builder that allows selective preloading

332e9a9

Added explicit fk_id struct tag to has_many associations as a hedge a…

187cc41

…gainst EagerPreload naming assumptions

Added fk_id to all belongs_to associations in anticipationn of using …

30e289d

…EagerPreload

Preload a few queries used in the admin

8ee2e54

Merge branch 'master' into rr-MB-8026-fetchmany-optimization

e6b38c7

reggieriser added the wip label May 7, 2021

reggieriser added 3 commits May 7, 2021 18:28

Reverting back to using default eager in one case that was failing

26e9676

Another place where we could preload

17c6d5e

Added comment about preload and Pop 5.3.4 for one preload candidate

7a3fba1

reggieriser removed the wip label May 7, 2021

reggieriser requested review from a team, shimonacarvalho, monfresh, duncan-truss and donaldthai May 7, 2021 21:18

reggieriser marked this pull request as ready for review May 7, 2021 21:20

reggieriser added the A-Team label May 7, 2021

gidjin approved these changes May 7, 2021

View reviewed changes

reggieriser added 2 commits May 10, 2021 14:33

Added TODO marker to comment

5818598

Added tests for loading associations with FetchMany

1b17fdd

gidjin approved these changes May 10, 2021

View reviewed changes

reggieriser merged commit b9a36df into master May 10, 2021

reggieriser deleted the rr-MB-8026-fetchmany-optimization branch May 10, 2021 17:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MB-8026] Optimizing FetchMany in query builder #6594

[MB-8026] Optimizing FetchMany in query builder #6594

reggieriser commented May 7, 2021 •

edited

robot-mymove commented May 7, 2021 •

edited

gidjin left a comment

gidjin May 7, 2021

reggieriser May 10, 2021

gidjin May 7, 2021

reggieriser May 10, 2021

reggieriser May 10, 2021

gidjin May 7, 2021

reggieriser May 10, 2021

gidjin left a comment

[MB-8026] Optimizing FetchMany in query builder #6594

[MB-8026] Optimizing FetchMany in query builder #6594

Conversation

reggieriser commented May 7, 2021 • edited

Description

Reviewer Notes

Setup

Code Review Verification Steps

References

robot-mymove commented May 7, 2021 • edited

gidjin left a comment

Choose a reason for hiding this comment

gidjin May 7, 2021

Choose a reason for hiding this comment

reggieriser May 10, 2021

Choose a reason for hiding this comment

gidjin May 7, 2021

Choose a reason for hiding this comment

reggieriser May 10, 2021

Choose a reason for hiding this comment

reggieriser May 10, 2021

Choose a reason for hiding this comment

gidjin May 7, 2021

Choose a reason for hiding this comment

reggieriser May 10, 2021

Choose a reason for hiding this comment

gidjin left a comment

Choose a reason for hiding this comment

reggieriser commented May 7, 2021 •

edited

robot-mymove commented May 7, 2021 •

edited