Page MenuHomePhabricator

[keyserver] Go back to using window function for getMessageInfos
ClosedPublic

Authored by ashoat on Oct 11 2022, 9:27 PM.
Tags
None
Referenced Files
F3400562: D5349.id17512.diff
Mon, Dec 2, 7:47 AM
Unknown Object (File)
Mon, Nov 25, 10:35 PM
Unknown Object (File)
Mon, Nov 25, 10:35 PM
Unknown Object (File)
Mon, Nov 25, 10:34 PM
Unknown Object (File)
Sun, Nov 24, 12:38 PM
Unknown Object (File)
Sat, Nov 2, 8:12 PM
Unknown Object (File)
Oct 7 2024, 11:44 AM
Unknown Object (File)
Oct 7 2024, 11:44 AM
Subscribers

Details

Summary

The window function is a much more modern and civilized approach to this query.

I first introduced this in D4675, but then reverted it in D4796 because it caused ENG-957.

However, once I found that the window function approach solved ENG-2009, I spent a second thinking about how to change the window function to avoid the issue from ENG-957. The easy solution is just to move the LEFT JOIN uploads line to the outer query.

In addition, I noticed the window function also needed an ORDER BY, as it was exhibiting the same out-of-order weirdness that caused ENG-2009. Although it wasn't affecting the partition function, our JS code for parsing the results expects that they appear in a specific order.

Test Plan

I tried to test this thoroughly given previous experiences.

  • I manually composed a query that reproduces the current issue (ENG-2009)
  • I edited it for the window function approach and saw that the issue was resolved
  • I manually composed a query that reproduces the old issue (ENG-957)
  • I edited it for the window function approach and saw that the old issue did not reappear
  • I patched my production keyserver live after midnight ET to confirm that the missing content now appears
  • I also live-tested to make sure the old issue was resolved by checking a chat that recently had a message with three images

Diff Detail

Repository
rCOMM Comm
Branch
ashoat/fix_message_query
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

keyserver/src/fetchers/message-fetchers.js
289–292

These two joins are fine to keep here because they can match a max of one line based on the UNIQUE KEY constraint in memberships. We need to keep the first join here for sure since it's inside in the inner WHERE clause

This revision is now accepted and ready to land.Oct 12 2022, 4:35 AM