Page MenuHomePhabricator

[keyserver] Force sorting of inner fetchMessageInfos query on MariaDB
ClosedPublic

Authored by ashoat on Jul 11 2022, 3:24 PM.
Tags
None
Referenced Files
F2186582: D4500.id14399.diff
Thu, Jul 4, 3:33 AM
Unknown Object (File)
Tue, Jun 25, 5:56 PM
Unknown Object (File)
Sun, Jun 23, 9:57 PM
Unknown Object (File)
Fri, Jun 21, 5:31 PM
Unknown Object (File)
Thu, Jun 20, 9:03 PM
Unknown Object (File)
Tue, Jun 18, 10:41 AM
Unknown Object (File)
Tue, Jun 18, 10:41 AM
Unknown Object (File)
Tue, Jun 18, 10:41 AM

Details

Summary

After switching to MariaDB, I saw an issue where fetchMessageInfos was returning the oldest messages rather than the newest ones.

It was happening because nested queries in MariaDB 10.7 seem to drop the order of the rows. I first tried simply adding an additional ORDER BY clause to the outer query, but found that didn't work because the SQL quirky counting logic we use gets processed before the ORDER BY. (Which is in fact the reason for the nested query.)

I did some Googling and found this. It turns out adding any LIMIT will force MariaDB to preserve the order.

Depends on D4499

Test Plan

I made sure fetchMessageInfos was returning results in the correct order by calling fetchMostRecentMessages from the client on a thread with a lot of messages. I then verified that the results were indeed the most recent messages

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

keyserver/src/fetchers/message-fetchers.js
297 ↗(On Diff #14399)

Since this is very "odd", I would add an in-line comment just so that people have contex as to why it's there.

tomek added inline comments.
keyserver/src/fetchers/message-fetchers.js
297 ↗(On Diff #14399)

Yeah, definitely a comment would be helpful. We can also consider adding a function e.g. nestedOrderedQuery that takes a query, appends LIMIT statement and returns a result. Introducing this function would hide this DB-specific thing.

This revision is now accepted and ready to land.Jul 12 2022, 2:06 AM