warehouse icon indicating copy to clipboard operation
warehouse copied to clipboard

Use `raiseload` option for all queries

Open di opened this issue 2 years ago • 7 comments

Sort of towards https://github.com/pypi/warehouse/issues/3081.

This PR turns on the raiseload option by default for all queries, essentially making N+1 queries impossible in our codebase, and requiring explicit loads instead, rather than per-query as documented in https://docs.sqlalchemy.org/en/20/orm/queryguide/relationships.html#preventing-unwanted-lazy-loads-using-raiseload .

Marking this as draft for now because there are a lot of test failures to address. I'm also not entirely sure yet if this also covers deferred columns as well, because that seems to be a separate setting: https://docs.sqlalchemy.org/en/20/orm/queryguide/columns.html#using-raiseload-to-prevent-deferred-column-loads

di avatar Mar 27 '23 21:03 di

Question: Is there a way to approach this gradually? Perhaps via Warnings?

Request: Can you implement a couple of the necessary fixes as examples? You mentioned that there might be some impacts on code readability. If its too egregious it may not be worth it.

ewdurbin avatar Mar 31 '23 15:03 ewdurbin

Let's pursue this once we've upgraded db libs. see: #13497

miketheman avatar Apr 24 '23 17:04 miketheman

@miketheman Do you think we're ready to revisit this?

di avatar Apr 03 '24 17:04 di

@di I think this is worthwhile, but should probably come after #15634 - since I was trying to ensure a query count before/after to support this effort from a web-side, since many of our potential N+1 come from HTML template rendering iteration loops.

miketheman avatar Apr 03 '24 17:04 miketheman

OK! Let's say this is blocked on https://github.com/pypi/warehouse/issues/15634.

di avatar Apr 03 '24 17:04 di

OK! Let's say this is blocked on #15634.

unblocked now that we can count queries emitted when loading a page via webtest

miketheman avatar Jul 12 '24 15:07 miketheman

Rebased. Quite a number of test failures to work through here if anyone is feeling up for it 🙂

di avatar Jul 12 '24 17:07 di