Page MenuHomePhabricator

[keyserver] Set last_message correctly for threads
ClosedPublic

Authored by michal on Oct 25 2023, 6:52 AM.
Tags
None
Referenced Files
F3535579: D9598.id32399.diff
Wed, Dec 25, 4:09 PM
Unknown Object (File)
Sat, Dec 14, 5:43 AM
Unknown Object (File)
Tue, Dec 10, 12:47 AM
Unknown Object (File)
Mon, Dec 9, 11:20 AM
Unknown Object (File)
Nov 9 2024, 5:26 PM
Unknown Object (File)
Nov 9 2024, 1:25 PM
Unknown Object (File)
Nov 9 2024, 11:41 AM
Unknown Object (File)
Nov 9 2024, 10:53 AM
Subscribers

Details

Summary

ENG-4101

Previously when adding a new membership (e.g. adding someone to a thread) the memberships.last_message column was set to 1 (or 0 if it was supposed to be unread). But after landing D8492 we need the last_message to have a correct value so we now what latest message to fetch.

This really is only an issue for genesis because for other threads there is new message "<user> joined" that sets the last_message to the correct value.

This is an alternative to D8680 which I will abandon if this diff is landed.

Test Plan

Performance analysis:

  • I've run EXPLAIN and ANALYZE operations on this query
  • The subqueries are of type LATERAL DERIVED which means db applies lateral derived optimization - it uses the constraints from the outer query to limit the number of searches in the subqueries. This means that each subquery only returns a few rows.
  • The type of the queries is ref which according to the documentation is good when the number of matched rows is low (which we get from the previous point)
  • We return early when just adding user normally (in this case there will be a "<user> joined a channel" message that will update the last_message). So the query will run very rarely - only when adding a new user to genesis

Tested creating a user:

  • The query only runs for this one user and for genesis
  • last_message was set to the latest text message and last_read_message remained 0

Changed setNewMembersToUnread in updateThread to false and tried creating a new user again. This time last_read_message was set to the latest text message. On the client the thread was set as "read"

To test the filtering of the private CREATE_SUB_THREAD messages I have changed the default silenceMessages in updateThread to true (so the "<user> joined" message wouldn't override the last_message). Then I added a user to a thread where the latest message is the private CREATE_SUB_THREAD message. User's last_message was set to the message before the private CREATE_SUB_THREAD message.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Oct 25 2023, 7:07 AM
Harbormaster failed remote builds in B23488: Diff 32399!

I think it would be good to test the query in production before landing this. In particular, I'm curious how long it takes. Two ideas come to mind:

  1. We could test an equivalent SELECT query
  2. (Probably better) We could test the real query in a safe way, eg. for @michal's user in production on a thread he's already in

Can you come up with a query that I can test in production, and send it over to me?

keyserver/src/updaters/thread-permission-updaters.js
890 ↗(On Diff #32402)

boolean parameters are not very readable at the callsite. Can you either replace this with some sort of string maybe, eg. ?"update_last_message", or alternately you could replace the param list with a param object

981 ↗(On Diff #32402)

Typo

1013 ↗(On Diff #32402)
  1. Is there a reason we have to wait until the first query completes before running this query?
  2. Why can't we combine this query with the first one?
  3. If we really need to have a separate query, would it make more sense to do a SELECT to figure out what the last_message and last_read_message fields should be, and then set them in the INSERT query?

This gist contains both a SELECT query based on the UPDATE query and an UPDATE query. Both of these queries show equivalent results under EXPLAIN so I think they should have the same performance. They are both set for my user (3033752 which I took from the table in ENG-5287) and the genesis thread (1).

keyserver/src/updaters/thread-permission-updaters.js
1013 ↗(On Diff #32402)
  1. The first query might update the visibility permission of a subthread.

Let's consider a case where user joins a thread where the last message is CREATE_SUB_THREAD message for a public thread. In this case we would create the membership rows for the parent thread and the sub thread at the same time.
Because the membership row for the subthread wouldn't yet exist, the subquery where we search for the latest public CREATE_SUB_THREAD message would return an incorrect result and the last_message would be set to some earlier message.

  1. For the same reason as (1). The subthread membership rows might not yet exist.
  2. I assumed that one query would be faster than having two queries and some js processing between them. We do something similar to your suggestion in the set-last-read-message script which @tomek wrote when migrating to the last_message, last_read_message fields. If this query turns out to be slow, I can test the option.

Both queries had great performance!! The SELECT took only 0.009s, and the UPDATE took even less (0.006s)

Please address all inline comments before landing

keyserver/src/updaters/thread-permission-updaters.js
890 ↗(On Diff #32402)

Reminder on this

981 ↗(On Diff #32402)

Reminder on this

992 ↗(On Diff #32402)

Nit

992–994 ↗(On Diff #32402)

Nit: I think there's no need to give a table an alias if it's there's no JOIN in the subquery

997–1001 ↗(On Diff #32402)

FROM on its own line (same nit as above)

1013 ↗(On Diff #32402)

Thanks for explaining

This revision is now accepted and ready to land.Oct 27 2023, 12:18 PM
keyserver/src/updaters/thread-permission-updaters.js
995 ↗(On Diff #32490)

Nit: I don't understand why you're using messages. prefix. Is there a need for this? I was suggested you remove the prefix in my previous review, not suggesting you replace it with the full table name

keyserver/src/updaters/thread-permission-updaters.js
995 ↗(On Diff #32490)

I misunderstood your comment. I will put a diff for this.