Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix N+1 query in event calendar found by sentry #4104

Merged
merged 2 commits into from Apr 29, 2024
Merged

Conversation

raphaelm
Copy link
Member

This fixes a performance issue introduced by the new settings lookup in #4036

@cla-bot cla-bot bot added the cla-signed label Apr 22, 2024
Copy link

codecov bot commented Apr 22, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 78.08%. Comparing base (fb403da) to head (4c72b40).
Report is 32 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4104   +/-   ##
=======================================
  Coverage   78.07%   78.08%           
=======================================
  Files         429      429           
  Lines       60227    60272   +45     
=======================================
+ Hits        47023    47061   +38     
- Misses      13204    13211    +7     
Files Coverage Δ
src/pretix/base/services/quotas.py 96.61% <100.00%> (+0.01%) ⬆️
src/pretix/presale/views/organizer.py 58.36% <42.85%> (-0.19%) ⬇️

... and 19 files with indirect coverage changes

Comment on lines +565 to +570
# save database lookups later
q.subevent = se
if event is not None:
q.event = event
else:
q.event = se.event
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this necessary? shouldn't this already be filled in by the .select_related('event', 'subevent') in EventMixin.annotated (i see that it isn't, but i don't understand why)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if it would, select_related would give it a new instance of the Event() class. We want, if possible, to use one instance of the Event() class so we only need to fetch the hierarkey data from cache once :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(That's one reason why we sometimes use prefetch_related where select_related would be sufficient, but also why we do some hacks like this here…)

@raphaelm raphaelm merged commit 044f0c5 into master Apr 29, 2024
11 of 12 checks passed
@raphaelm raphaelm deleted the calendar-n+1 branch April 29, 2024 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants