~ knotless Search/
↑↓ move↵ open⇧↵ all results

chadtmiller.com/knotless

git that lives in your Atmosphere account

knotless.dev
1 collaborator

knotless 0.13.0 Latest 2 packages, 1 image

1
10

What each notification says is found for its page alone

Merged
Merged chadtmiller.com asked to merge into main from chadtmiller.com/knotless:serve/notifications-after-limit

What changes

The notifications query now picks its page first: every reason's rows, filtered, sorted, cut to the limit. Only then does it work out the two things said of each notification on that page: whether the account is also named in it, and how many line comments came with a review. The notifications returned are the same.

Why

On prod today the notifications statement ran 173 times, with a mean of 277ms and a max of 4.4s, the slowest of the index's statements. It gathers every row the account was ever notified of, sorts them, and keeps a page. SQLite works out a statement's result columns before it sorts (unless it was built with sorter references, which it isn't by default). So those two correlated subqueries ran for every row ever notified, and for the owner of a busy repository that is thousands of rows for a page of 50. Moved outside the limit, they run once per row returned. The inner query keeps its own ORDER BY, so SQLite cannot flatten it back into the outer one.

How it was checked

The notification tests in the index and serve suites cover each reason, line comments, mentions alongside other reasons, kinds and cursors. They run in CI, along with the plans gate. After deploy, the check is the statement's mean and max in stats, against 277ms and 4.4s.

Shortcomings

It still gathers and sorts every row the account was ever notified of. Cutting that down means taking each reason's own limit and cursor, with the filters moved into each reason. That is a larger rewrite, worth doing only if this one leaves it slow.

🤖 Generated with Claude Code

Approved

This wraps the notifications query in a subquery that does the filtering, sorting and LIMIT. The "also named" and line-comment-count subqueries now run in an outer select over only the page's rows. I checked the rewrite against the full statement and found no problem.

  • The outer columns still come out in the order row.get(0..14) reads them. shown, owner and name stand in for the old CASE, r.did and r.name.
  • The line-comment subquery uses n.subject, n.author and n.uri. All three come through n.*.
  • The LEFT JOIN repo r, the filters, the cursor and LIMIT ?4 stay in the inner query. The outer ORDER BY repeats the same order, so page order is unchanged.

I couldn't see CI results, so I haven't confirmed that the notification tests and the plans gate pass. The PR says it relies on them, and the plans gate is the one most likely to be affected by a changed plan.

Merged as 9cd3032