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
Unknown Object (File)
Fri, Sep 6, 10:40 PM
Unknown Object (File)
Fri, Sep 6, 8:08 PM
Unknown Object (File)
Fri, Sep 6, 8:08 PM
Unknown Object (File)
Sep 5 2024, 5:47 AM
Unknown Object (File)
Aug 27 2024, 3:00 PM
Unknown Object (File)
Aug 21 2024, 9:44 AM
Unknown Object (File)
Aug 21 2024, 3:56 AM
Unknown Object (File)
Aug 21 2024, 1:57 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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

keyserver/src/fetchers/message-fetchers.js
289–292 ↗(On Diff #17500)

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