Improve query for getting users for review #1891
Conversation
So in my comment I was wrong about |
|
|
| SELECT u.username AS username | ||
| FROM users AS u | ||
| LEFT JOIN review_prefs AS r ON u.user_id = r.user_id | ||
| AND COALESCE(CARDINALITY(r.assigned_prs), 0) < COALESCE(r.max_assigned_prs, 100000) | ||
| WHERE u.username = ANY('{{ {} }}') | ||
| ", |
There was a problem hiding this comment.
Just an observation: before your change, this function didn't return any record if no "joined" user was found. Changing the query as you suggest also changes the API, now it always return something so the caller must check if the returned candidate actually has capacity.
There is a TODO at the top of has_user_capacity() (which does more or less the same work) and that was implemented in #1893. I'll probably split that patch to make it clearer.
EDIT: in commit b93064a
There was a problem hiding this comment.
Right, so my query assumes that the only way the capacity could be affected is through the review_prefs column. Which I assume should be true? Apart from the on vacation list, that has to be checked elsewhere (until we get rid of it).
In other words, if you don't have an entry in review_prefs, then you have capacity.
The previous query had a problem where if some user was missing in the
review_prefstable, it would not be returned from the query. This is problematic, because for several reasons, thereview_prefstable might be incomplete, and in general, it should be possible to get users even if they are not in thereview_prefstable.There is one problem with the new query though, because there is no index on the
users.usernamecolumn, this will result in a sequential scan on theuserstable. It's probably still fast enough, but I wanted to mention it.The
LEASTclause was not required, I think. The addedCOALESCEis needed to treat the number of PRs for people missing inreview_prefsas zero, otherwise they would be again ignored.You can experiment with this in https://onecompiler.com/postgresql/43722tb3w.