OpenNeo/impress
OpenNeo/impress
1
0
Fork 1
Code Pull requests Activity

Use delete_all for ClosetList hangers to avoid N+1 on deletion #5

Merged
matchu merged 1 commit from dice/fix-closet-list-delete-performance into main 2026-06-13 18:59:38 -07:00 AGit
Conversation 1 Commits 1 Files changed 1 +1 -1
dice commented 2026-06-13 16:30:58 -07:00
Contributor
Copy link

Hanger after_destroy callbacks only touch last_trade_activity_at, which
is already covered by ClosetList's own after_destroy callback.

Hanger after_destroy callbacks only touch last_trade_activity_at, which is already covered by ClosetList's own after_destroy callback. 
Hanger after_destroy callbacks only touch last_trade_activity_at, which
is already covered by ClosetList's own after_destroy callback.
matchu approved these changes 2026-06-13 18:59:29 -07:00
matchu left a comment
Copy link

👍 Had the robot gut-check what the relevant hooks actually are, looks like we're solid!

Analysis results

Switching ClosetList#hangers to dependent: :delete_all

The problem

Destroying a ClosetList with dependent: :destroy loads every hanger into
memory, instantiates each one, and runs its callbacks one at a time. That's the
hang on large lists. Switching to delete_all issues a single
DELETE FROM closet_hangers WHERE list_id = ? instead.

What delete_all skips

Per-hanger destroy runs these, and delete_all bypasses them:

  1. after_destroy :log_trade_activity, if: :trading? — touches user.last_trade_activity_at.
  2. belongs_to :list, touch: true — touches the list's updated_at.

That's the entire list. Crucially:

  • No nested dependents. ClosetHanger has no has_many ... dependent:, so
    delete_all can't orphan any child rows. (This is the thing that makes
    delete_all dangerous in the general case — and it doesn't apply here.)
  • No counter caches on item or user. Nothing is left stale.

Are the skipped behaviors a real loss?

Basically no.

belongs_to :list, touch: true — touches the list's updated_at, but the
list is being deleted in the same transaction. Pure waste, zero loss.

log_trade_activity — looks scary, but isn't, because of how trading?
resolves. A hanger's trading? delegates to possibly_null_closet_list.trading?,
and since these hangers all have a list, that's just list.trading?. So:

  • If the list is not trading → no hanger would have logged anyway. No change.
  • If the list is trading → ClosetList has its own
    after_destroy :log_trade_activity, if: :trading?, which still fires under
    delete_all (we're only changing the dependent strategy; the list itself is
    still destroyed). So user.last_trade_activity_at is still touched — once,
    instead of once-per-hanger.

log_trade_activity is just touch(:last_trade_activity_at), an idempotent
timestamp bump. Today we do it N+1 times; after the change, once. Same end state.

Verdict

Safe and good. No meaningful behavior lost, and the activity-logging that matters
is preserved by the list's own after_destroy. Atomicity is preserved too —
dependent: :delete_all still runs inside the parent destroy's transaction.

👍 Had the robot gut-check what the relevant hooks actually are, looks like we're solid! <details> <summary>Analysis results</summary> ## Switching `ClosetList#hangers` to `dependent: :delete_all` ### The problem Destroying a `ClosetList` with `dependent: :destroy` loads **every** hanger into memory, instantiates each one, and runs its callbacks one at a time. That's the hang on large lists. Switching to `delete_all` issues a single `DELETE FROM closet_hangers WHERE list_id = ?` instead. ### What `delete_all` skips Per-hanger `destroy` runs these, and `delete_all` bypasses them: 1. **`after_destroy :log_trade_activity, if: :trading?`** — touches `user.last_trade_activity_at`. 2. **`belongs_to :list, touch: true`** — touches the list's `updated_at`. That's the entire list. Crucially: - **No nested dependents.** `ClosetHanger` has no `has_many ... dependent:`, so `delete_all` can't orphan any child rows. (This is the thing that makes `delete_all` dangerous in the general case — and it doesn't apply here.) - **No counter caches** on `item` or `user`. Nothing is left stale. ### Are the skipped behaviors a real loss? Basically no. **`belongs_to :list, touch: true`** — touches the list's `updated_at`, but the list is being deleted in the same transaction. Pure waste, zero loss. **`log_trade_activity`** — looks scary, but isn't, because of how `trading?` resolves. A hanger's `trading?` delegates to `possibly_null_closet_list.trading?`, and since these hangers all have a `list`, that's just **`list.trading?`**. So: - If the list is *not* trading → no hanger would have logged anyway. No change. - If the list *is* trading → `ClosetList` has its **own** `after_destroy :log_trade_activity, if: :trading?`, which still fires under `delete_all` (we're only changing the dependent strategy; the list itself is still `destroy`ed). So `user.last_trade_activity_at` is still touched — once, instead of once-per-hanger. `log_trade_activity` is just `touch(:last_trade_activity_at)`, an idempotent timestamp bump. Today we do it N+1 times; after the change, once. Same end state. ### Verdict Safe and good. No meaningful behavior lost, and the activity-logging that matters is preserved by the list's own `after_destroy`. Atomicity is preserved too — `dependent: :delete_all` still runs inside the parent destroy's transaction. </details>
matchu merged commit 71ed619ba8 into main 2026-06-13 18:59:38 -07:00
Sign in to join this conversation.
Reviewers
No reviewers
matchu
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
OpenNeo/impress!5
No description provided.