Reduce N+1s by caching submitted project count (Part 1 of 2)#804
Reduce N+1s by caching submitted project count (Part 1 of 2)#804zetter-rpf wants to merge 2 commits into
Conversation
Test coverage89.46% line coverage reported by SimpleCov. |
There was a problem hiding this comment.
Pull request overview
Introduces a cached submitted_projects_count on Lesson and keeps it updated when SchoolProject transitions into/out of submitted, plus a rake task + specs to backfill/verify counts to support eliminating N+1 queries in controllers.
Changes:
- Add
lessons.submitted_projects_countcolumn (default 0) and aLesson#recalculate_submitted_projects_count!helper. - Trigger lesson count recalculation on
SchoolProjectstate transitions to/fromsubmitted. - Add
lessons:backfill_submitted_projects_countrake task and RSpec coverage for the model/state machine/task.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| app/models/lesson.rb | Adds remix/school_project associations and count recalculation method for the new cached column. |
| app/models/school_project.rb | Adds lesson lookup and a callback helper used by the state machine. |
| app/state_machines/school_project_state_machine.rb | Hooks after_transition callbacks for submitted-enter/leave events. |
| lib/tasks/lessons.rake | Adds backfill task to populate cached counts in bulk via SQL. |
| db/migrate/20260429120000_add_submitted_projects_count_to_lessons.rb | Adds the new cached count column. |
| db/schema.rb | Reflects the new lessons column and schema version bump. |
| spec/models/lesson_spec.rb | Adds specs for #recalculate_submitted_projects_count!. |
| spec/state_machines/school_project_state_machine_spec.rb | Adds specs ensuring transitions update the cached lesson count. |
| spec/lib/tasks/lessons_spec.rb | Adds rake task spec for the backfill behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There's currently N+1 queries on both the lessons and classes controller as they both try to show the number of submitted projects. Originally I tired to solve this by using 'include' to load the correct records, however I don't think this is a very scalable solutions as it means loading every submitted project object memory for classes or lessons (which there may be 1000s). Instead use the pattern of a counter cache. Each time a project transitions to/from submitted, we recalculate the number. This is the first part of two. I've broken up the work so we can deploy this and check that the counts are correct. If we deployed this in one go there would be a chance the counts are inaccurate if a project transition happened during the deploy. ### After deploy Run `rails lessons:backfill_submitted_projects_count` and verify the results by checking a few lessons and comparing it to `lesson#submitted_count` Co-authored-by: Copilot <copilot@github.com>
Add lock to prevent stale reads in the case this runs concurrently for the same project Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
9def7db to
6ff71db
Compare
In [1] we started caching the value of submitted projects. Now we can use that value to simplify the code and reduce N+1 queries. [1] #804 Note that I've had to split the loading of remixes for teachers and students as teachers no longer need remixes (which causes bullet to complain) Co-authored-by: Copilot <copilot@github.com>
| task backfill_submitted_projects_count: :environment do | ||
| Lesson.connection.execute <<~SQL.squish | ||
| WITH submitted_counts AS ( | ||
| SELECT lesson_projects.lesson_id, COUNT(*) AS submitted_projects_count |
There was a problem hiding this comment.
I would have though that this needs to do the same thing as recalculate_lesson_submitted_projects_count!, across all projects. Are we writing our own query for efficiency, or is it something else that I'm missing?
There was a problem hiding this comment.
Yes I defaulted to doing it this way for efficiency without even checking how many lessons there are.
I've checked and there are 28114 - not loads, but thats enough that updating them all one-after-the-other could create database contention and slow down the site for the couple of minutes it's running.
Status
What's changed?
There's currently N+1 queries on both the lessons and classes controller as they both try to show the number of submitted projects.
Originally I tired to solve this by using 'include' to load the correct records, however I don't think this is a very scalable solutions as it means loading every submitted project object memory for classes or lessons (which there may be 1000s, especially when listing classes)
Instead use the pattern of a counter cache. Each time a project transitions to/from submitted, we recalculate the number.
This is the first part of two. I've broken up the work so we can deploy this and check that the counts are correct. If we deployed this in one go there would be a chance the counts are inaccurate if a project transition happened during the deploy.
After deploy
Run
rails lessons:backfill_submitted_projects_countand verify the results by checking a few lessons and comparing it tolesson#submitted_count