Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Mark events as read using threaded read receipts from MSC3771 #13877

Merged
merged 42 commits into from
Oct 4, 2022

Conversation

clokep
Copy link
Member

@clokep clokep commented Sep 22, 2022

Builds on #13782 & #13776 to handle threaded read receipts (which are now stored in the database) when marking events as read.

As MSC3771 discusses, this handles read receipts of two types:

  1. Unthreaded read receipts (aka how read receipts currently work).
  2. Threaded read receipts (which operate only on events in the same thread).

Part of #12550.

@clokep clokep force-pushed the clokep/threads-notif-3b branch from bcbddac to c352f3e Compare September 22, 2022 17:33
@clokep clokep changed the title Handle threaded read receipts from MSC3771 Mark events as read using threaded read receipts from MSC3771 Sep 22, 2022
@clokep clokep force-pushed the clokep/threads-notif-3 branch 2 times, most recently from 72de12e to c677782 Compare September 23, 2022 13:49
Base automatically changed from clokep/threads-notif-3 to develop September 23, 2022 14:33
@clokep clokep force-pushed the clokep/threads-notif-3b branch from c352f3e to ed0871e Compare September 23, 2022 15:31
@clokep clokep changed the base branch from develop to clokep/threads-notif-2 September 23, 2022 15:31
@clokep clokep marked this pull request as ready for review September 23, 2022 19:31
@clokep clokep requested a review from a team as a code owner September 23, 2022 19:31
Copy link
Member

@erikjohnston erikjohnston left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks good, I'm a bit nervous about the queries I commented on so would love to know if we could somehow bound them.

user_id = ?
AND room_id = ?
AND {receipt_types_clause}
GROUP BY thread_id
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this query is going to load all receipts in a room? Can we add bound it somehow, like adding a stream_id clause or something?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, they'll load everything in the room. Will think about how we can limit it!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can limit this to only fetch threaded receipts after the unthreaded receipt, I think? If the threaded receipt is earlier than the unthreaded receipt then it doesn't matter. (This would essentially replace the GREATEST(threaded_receipt, unthreaded_receipt)?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@erikjohnston How does 041fe7f look to you?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think that looks good. Do you have an example query plan?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I meant to include that, this an EXPLAIN ANALYZE of this query on Matrix HQ with a user. There aren't really currently threaded read receipts in this room though so, maybe this isn't useful?

Matrix HQ
 Hash Left Join  (cost=12.49..16.52 rows=1 width=21) (actual time=1.619..1.799 rows=1 loops=1)
   Hash Cond: (event_push_summary.thread_id = receipts_linearized.thread_id)
   Filter: (((event_push_summary.last_receipt_stream_ordering IS NULL) AND (event_push_summary.stream_ordering > COALESCE((max(events.stream_ordering)), '3156421630'::bigint))) OR (event_push_summary.last_receipt_stream_ordering = COALESCE((max(events.
stream_ordering)), '3156421630'::bigint)))
   ->  Index Scan using event_push_summary_unique_index2 on event_push_summary  (cost=0.56..4.58 rows=1 width=37) (actual time=0.569..0.746 rows=1 loops=1)
         Index Cond: ((user_id = '@e:matrix.org'::text) AND (room_id = '!OGEhHVWSdvArJzumhm:matrix.org'::text))
         Filter: ((notif_count <> 0) OR (COALESCE(unread_count, '0'::bigint) <> 0))
   ->  Hash  (cost=11.92..11.92 rows=1 width=40) (actual time=1.036..1.037 rows=0 loops=1)
         Buckets: 1024  Batches: 1  Memory Usage: 8kB
         ->  GroupAggregate  (cost=11.89..11.91 rows=1 width=40) (actual time=1.036..1.037 rows=0 loops=1)
               Group Key: receipts_linearized.thread_id
               ->  Sort  (cost=11.89..11.90 rows=1 width=40) (actual time=1.034..1.035 rows=0 loops=1)
                     Sort Key: receipts_linearized.thread_id
                     Sort Method: quicksort  Memory: 25kB
                     ->  Nested Loop  (cost=1.27..11.88 rows=1 width=40) (actual time=1.027..1.027 rows=0 loops=1)
                           ->  Index Scan using receipts_linearized_uniqueness_thread on receipts_linearized  (cost=0.56..7.16 rows=1 width=104) (actual time=0.324..0.336 rows=1 loops=1)
                                 Index Cond: ((room_id = '!OGEhHVWSdvArJzumhm:matrix.org'::text) AND (receipt_type = ANY ('{m.read,m.read.private}'::text[])) AND (user_id = '@e:matrix.org'::text))
                           ->  Index Scan using events_event_id_key on events  (cost=0.70..4.73 rows=1 width=78) (actual time=0.687..0.687 rows=0 loops=1)
                                 Index Cond: (event_id = receipts_linearized.event_id)
                                 Filter: ((stream_ordering > '3156421630'::bigint) AND (room_id = '!OGEhHVWSdvArJzumhm:matrix.org'::text))
                                 Rows Removed by Filter: 1
 Planning time: 1.432 ms
 Execution time: 1.892 ms
(22 rows)

I also ran one with some threaded read receipts:

Test room w/ threaded receipts
 Hash Left Join  (cost=26.83..36.92 rows=1 width=56) (actual time=0.030..0.032 rows=0 loops=1)
   Hash Cond: (event_push_summary.thread_id = receipts_linearized.thread_id)
   Filter: (((event_push_summary.last_receipt_stream_ordering IS NULL) AND (event_push_summary.stream_ordering > COALESCE
((max(events.stream_ordering)), '1856'::bigint))) OR (event_push_summary.last_receipt_stream_ordering = COALESCE((max(events.stream_ordering)), '1856'::bigint)))
   ->  Bitmap Heap Scan on event_push_summary  (cost=4.31..14.39 rows=4 width=72) (actual time=0.030..0.030 rows=0 loops=1)
         Recheck Cond: ((user_id = '@clokep:threads-dev.lab.element.dev'::text) AND (room_id = '!BCtxSvqOvcQQYhkLsB:threads-dev.lab.element.dev'::text))
         Filter: ((notif_count <> 0) OR (COALESCE(unread_count, '0'::bigint) <> 0))
         Rows Removed by Filter: 4
         Heap Blocks: exact=2
         ->  Bitmap Index Scan on event_push_summary_unique_index2  (cost=0.00..4.31 rows=4 width=0) (actual time=0.018..0.018 rows=4 loops=1)
               Index Cond: ((user_id = '@clokep:threads-dev.lab.element.dev'::text) AND (room_id = '!BCtxSvqOvcQQYhkLsB:threads-dev.lab.element.dev'::text))
   ->  Hash  (cost=22.50..22.50 rows=1 width=24) (never executed)
         ->  GroupAggregate  (cost=22.47..22.49 rows=1 width=24) (never executed)
               Group Key: receipts_linearized.thread_id
               ->  Sort  (cost=22.47..22.48 rows=1 width=24) (never executed)
                     Sort Key: receipts_linearized.thread_id
                     ->  Nested Loop  (cost=4.88..22.46 rows=1 width=24) (never executed)
                           Join Filter: (receipts_linearized.event_id = events.event_id)
                           ->  Index Scan using events_stream_ordering on events  (cost=0.28..7.83 rows=1 width=87) (never executed)
                                 Index Cond: (stream_ordering > '3156421630'::bigint)
                                 Filter: (room_id = '!BCtxSvqOvcQQYhkLsB:threads-dev.lab.element.dev'::text)
                           ->  Bitmap Heap Scan on receipts_linearized  (cost=4.59..14.57 rows=5 width=100) (never executed)
                                 Recheck Cond: ((room_id = '!BCtxSvqOvcQQYhkLsB:threads-dev.lab.element.dev'::text) AND (user_id = '@clokep:threads-dev.lab.element.dev'::text))
                                 Filter: (receipt_type = ANY ('{m.read,m.read.private}'::text[]))
                                 ->  Bitmap Index Scan on receipts_linearized_uniqueness_thread  (cost=0.00..4.59 rows=5 width=0) (never executed)
                                       Index Cond: ((room_id = '!BCtxSvqOvcQQYhkLsB:threads-dev.lab.element.dev'::text) AND (user_id = '@clokep:threads-dev.lab.element.dev'::text))
 Planning Time: 1.027 ms
 Execution Time: 0.121 ms
(27 rows)

@clokep clokep requested a review from erikjohnston September 29, 2022 15:32
Base automatically changed from clokep/threads-notif-2 to develop October 4, 2022 13:47
@clokep clokep merged commit a7ba457 into develop Oct 4, 2022
@clokep clokep deleted the clokep/threads-notif-3b branch October 4, 2022 14:46
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Oct 29, 2022
Upstream changes:

Synapse 1.70.1 (2022-10-28)
===========================

(bugfixes)


Synapse 1.70.0 (2022-10-26)
===========================

Features
--------

- Support for
  [MSC3856](matrix-org/matrix-spec-proposals#3856):
  threads list
  API. ([\#13394](matrix-org/synapse#13394),
  [\#14171](matrix-org/synapse#14171),
  [\#14175](matrix-org/synapse#14175))

- Support for thread-specific notifications & receipts
  ([MSC3771](matrix-org/matrix-spec-proposals#3771)
  and
  [MSC3773](matrix-org/matrix-spec-proposals#3773)). ([\#13776](matrix-org/synapse#13776),
  [\#13824](matrix-org/synapse#13824),
  [\#13877](matrix-org/synapse#13877),
  [\#13878](matrix-org/synapse#13878),
  [\#14050](matrix-org/synapse#14050),
  [\#14140](matrix-org/synapse#14140),
  [\#14159](matrix-org/synapse#14159),
  [\#14163](matrix-org/synapse#14163),
  [\#14174](matrix-org/synapse#14174),
  [\#14222](matrix-org/synapse#14222))

- Stop fetching missing `prev_events` after we already know their
  signature is
  invalid. ([\#13816](matrix-org/synapse#13816))

- Send application service access tokens as a header (and query
  parameter). Implements
  [MSC2832](matrix-org/matrix-spec-proposals#2832). ([\#13996](matrix-org/synapse#13996))

- Ignore server ACL changes when generating pushes. Implements
  [MSC3786](matrix-org/matrix-spec-proposals#3786). ([\#13997](matrix-org/synapse#13997))

- Experimental support for redirecting to an implementation of a
  [MSC3886](matrix-org/matrix-spec-proposals#3886)
  HTTP rendezvous
  service. ([\#14018](matrix-org/synapse#14018))

- The `/relations` endpoint can now be used on
  workers. ([\#14028](matrix-org/synapse#14028))

- Advertise support for Matrix 1.3 and 1.4 on
  `/_matrix/client/versions`. ([\#14032](matrix-org/synapse#14032),
  [\#14184](matrix-org/synapse#14184))

- Improve validation of request bodies for the [Device
  Management](https://spec.matrix.org/v1.4/client-server-api/#device-management)
  and [MSC2697 Device
  Dehyrdation](matrix-org/matrix-spec-proposals#2697)
  client-server API
  endpoints. ([\#14054](matrix-org/synapse#14054))

- Experimental support for
  [MSC3874](matrix-org/matrix-spec-proposals#3874):
  Filtering threads from the `/messages`
  endpoint. ([\#14148](matrix-org/synapse#14148))

- Improve the validation of the following PUT endpoints:
  [`/directory/room/{roomAlias}`](https://spec.matrix.org/v1.4/client-server-api/#put_matrixclientv3directoryroomroomalias),
  [`/directory/list/room/{roomId}`](https://spec.matrix.org/v1.4/client-server-api/#put_matrixclientv3directorylistroomroomid)
  and
  [`/directory/list/appservice/{networkId}/{roomId}`](https://spec.matrix.org/v1.4/application-service-api/#put_matrixclientv3directorylistappservicenetworkidroomid). ([\#14179](matrix-org/synapse#14179))


Deprecations and Removals
-------------------------

- Remove the experimental implementation of
  [MSC3772](matrix-org/matrix-spec-proposals#3772). ([\#14094](matrix-org/synapse#14094))

- Remove the unstable identifier for
  [MSC3715](matrix-org/matrix-spec-proposals#3715). ([\#14106](matrix-org/synapse#14106),
  [\#14146](matrix-org/synapse#14146))
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants