Launchpad itself

Merge ~pappacena/launchpad:storm_select_related into launchpad:master

Proposed by Thiago F. Pappacena on 2020-01-22

Status:	Needs review
Proposed branch:	~pappacena/launchpad:storm_select_related
Merge into:	launchpad:master
Diff against target:	902 lines (+525/-20) 12 files modified database/schema/security.cfg (+4/-1) lib/lp/services/database/configure.zcml (+60/-0) lib/lp/services/database/select_related.py (+145/-0) lib/lp/services/database/tests/test_select_related.py (+57/-0) lib/lp/soyuz/browser/queue.py (+28/-4) lib/lp/soyuz/browser/tests/test_queue.py (+39/-4) lib/lp/soyuz/configure.zcml (+7/-1) lib/lp/soyuz/interfaces/queue.py (+32/-1) lib/lp/soyuz/model/queue.py (+79/-5) lib/lp/soyuz/stories/soyuz/xx-queue-pages.txt (+25/-1) lib/lp/soyuz/templates/distroseries-queue.pt (+10/-0) lib/lp/soyuz/tests/test_packageupload.py (+39/-3)
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Launchpad code reviewers		2020-01-22	Pending
Review via email: mp+377968@code.qastaging.launchpad.net

Commit message

[DONT MERGE] Adding an easy way to select related objects on Storm (discussion only)

Description of the change

I'm creating this MP in order to have a place to discuss an initial idea. Please don't merge it, since it's probably a better idea to propose this change on Storm, if others would think this could be a good idea.

One problem we have today in LP is that, in order to prefetch related objects (reviewer Person from PackageUploadLog, for example), we are first fetching the list of the first objects (PackageUploadLog), getting the related IDs and issuing another query to fetch the second class of objects (reviewers/Person).

This way is obviously better than doing one query per PackageUploadLog, but it's still one query worst than it would be if we just LEFT JOIN PackageUploadLog with Person in this simple scenario (ignoring the other complexities of Person entity here).

Django has an easy way to solve this with something like `PackageUploadLog.objects.all().select_related('reviewer')`. I couldn't find an easy way to do it on Storm ORM without manually joining the table and setting the condition. Way more verbose than Django requires.

The idea here is to implement something similar to what we have in Django:
`resultset = store.find(PackageUploadLog).select_related(PackageUploadLog.reviewer)` would join both tables with a left join, fill the store with all objects in a way that, when doing `[p.reviwer for p in resultset]` after, it would fetch from local store and no extra query would be needed.

This branch already works and doesn't introduce regressions on LP (although the code is terribly ugly and not well tested; I would need to work the implementation details with Storm maintainers - if any!)

Implementing something similar to Django's `prefetch_related` (to fetch multiple objects) could be a future possibility.

Revision history for this message

Colin Watson (cjwatson) wrote on 2020-01-28:

I think I see where you're going with this, and in principle it seems like a reasonable thing to add. This is almost a worst case for a third-party extension to Storm though; you're having to contort things a bit to cope with being in a subclass, and I'd find it a *lot* easier to review as an MP against lp:storm itself. (The people more or less active in maintaining Storm at the moment seem to be me and Simon Poirier.)

I'd also suggest talking with Free Ekanayaka, who's still around (though not working on Storm much) and you should be able to get hold of him. It was a long time ago and I'm not quite sure where it went, but https://bazaar.launchpad.net/~free.ekanayaka/storm/eager-loading/revision/361 seems like it's trying to do some of the same things that you're trying to do, and to my mind looks rather cleaner and better-integrated with Storm's reference mechanisms; so it would be worth asking him whether there were any fundamental obstacles there. I haven't reviewed either approach in a lot of fine detail, though, and both would need lots of testing.