Track notification counts per thread (implement MSC3773) by clokep · Pull Request #13181 · matrix-org/synapse

clokep · 2022-07-05T14:25:23Z

Track notification counts on a per-thread basis, implementing MSC3773.

The overall design of this is to add a thread_id column to the event_push_actions (+ event_push_actions_staging) and event_push_summary tables. This allows the results to be segmented by the "main" timeline (which is represented by NULL in the database) and any other threads (which have the root event ID in the thread_id column).

When retrieving counts of notifications we can then segment based on the thread, this is opt-in for the client by providing a sync flag. In the case the client doesn't want separate threads we simply sum across all threads + the main timeline.

The summarization code also needs to be updated to be per thread, instead of just per room.

Please see MSC3773 for a description of the API changes.

Part of #12550

To Do

Update sync code to take into account the new flag.
Add an experimental config flag.
Write thread-specific tests.
Remove debug information.

synapse/storage/schema/main/delta/72/03thread_notifications.sql

synapse/storage/databases/main/event_push_actions.py

…ounts.

…tions_by_room_for_user.

synapse/api/filtering.py

clokep · 2022-08-25T16:31:44Z

synapse/storage/databases/main/event_push_actions.py

+            # XXX All threads should have the same stream ordering?
+            max_summary_stream_ordering = max(
+                summary_stream_ordering, max_summary_stream_ordering
+            )


Need to figure out what's going on here.

Looking at my test server a bit, this isn't true (that all event_push_summary for a room/user have the same stream_ordering). I'm not really sure why I thought this, but it likely causes subtle bugs.

I think that stream ordering is the "max" stream ordering that we rotated from the event_push_actions table:

https://github.com/matrix-org/synapse/pull/13181/files#diff-f121377d76a7b35c60092da2c4bf8c849544459a2c676cf8e87057aff24ece54R1218

I think that means its equivalent to the stream ordering we rotated up to? They may differ but there should never be any relevant rows in EPA between the per-thread stream ordering and the max stream ordering?

synapse/push/bulk_push_rule_evaluator.py

clokep · 2022-08-25T16:32:26Z

synapse/storage/databases/main/event_push_actions.py

+            # TODO Delete zeroed out threads completely from the database.
+            elif notif_count or unread_count:
+                thread_counts[thread_id] = NotifCounts(
+                    notify_count=notif_count, unread_count=unread_count
+                )


Might be worth looking at this again briefly not that some other bugs have been fixed.

clokep · 2022-08-31T14:53:30Z

Requesting review of this, but keeping it in draft. I think the implementation is far enough along that we could merge this, but don't want it merged accidentally.

@erikjohnston had a brief conversation of "how bad would it be to back this out if perf tanks". I think it wouldn't be too bad: backing out the code should essentially revert behavior, with the caveat there would be an additional unused column getting tossed around. This column could then be dropped without an issue.

clokep · 2022-08-31T15:39:06Z

tests/storage/test_event_push_actions.py

        _rotate()
        _assert_counts(0, 0, 0)

+    def test_count_aggregation_threads(self) -> None:


(I should possibly put this in a docstring...)

This is a "copy" of the test_count_aggregation test, but adapted to have both a "main" timeline and a thread in the same room.

erikjohnston · 2022-09-02T08:55:24Z

(Oh, woops, I hit the merge develop branch on the wrong PR, sowwy)

erikjohnston

I think this looks sane

erikjohnston · 2022-09-06T07:26:34Z

synapse/storage/databases/main/event_push_actions.py

            )

+            # Then any updated threads get their notification count and unread
+            # count updated.


This presumably handles the "main" thread too?

I'm actually having trouble convincing myself this doesn't need to handle thread_id being null separately.

If the summary exists above for the "main" thread than the upsert would fail, I think? (Since it would attempt to add a new row since null isn't equivalent null.)

I think that's possible, but probably not exercised by our tests?

erikjohnston · 2022-09-06T07:28:00Z

synapse/storage/databases/main/event_push_actions.py

+        }
+
+        # simple_upsert_many_txn doesn't support a predicate clause to force using
+        # the partial index (thread_id = NULL).


Isn't that easy to add?

I tried a bit and was having issues with figuring out how the API should look -- a simple implementation which just accepts a string is pretty easy (I think), but a more complicated one which accepts keyvalues: Dict[str, Any] is a bit harder because you need to do conversion of = NULL to IS NULL. I'm a bit surprised we don't already have code for this?

Anyway, it is doable, yes. I can do it as a separate PR if you'd like.

Ah, never mind if it isn't trivial. Was just a bit surprised is all.

Looking at it, it should be doable. It would really simplify this code, so will take a look. 👍

erikjohnston · 2022-09-06T07:31:41Z

synapse/storage/databases/main/event_push_actions.py

+            # XXX All threads should have the same stream ordering?
+            max_summary_stream_ordering = max(
+                summary_stream_ordering, max_summary_stream_ordering
+            )


I think that stream ordering is the "max" stream ordering that we rotated from the event_push_actions table:

https://github.com/matrix-org/synapse/pull/13181/files#diff-f121377d76a7b35c60092da2c4bf8c849544459a2c676cf8e87057aff24ece54R1218

I think that means its equivalent to the stream ordering we rotated up to? They may differ but there should never be any relevant rows in EPA between the per-thread stream ordering and the max stream ordering?

erikjohnston · 2022-09-06T07:33:38Z

synapse/storage/schema/main/delta/72/03thread_notifications.sql

+
+-- Update the unique index for `event_push_summary`.
+INSERT INTO background_updates (ordering, update_name, progress_json) VALUES
+  (7003, 'event_push_summary_unique_index2', '{}');


I'm a bit worried that a bunch of the code will break while we don't have the proper index on event_push_summary? In particular I think if it encounters threads will try and insert multiple rows and fail due to the existing unique index?

Yes, that might be an issue -- would the solution be to drop the current index here and then add then new one in the background?

I don't think so, as we need a unique index to have upserts work? I have a horrible suspicion that we might need to either do this as a two step PR, one to add the column and new unique index, and the other to drop the index and start populating the thread rows. Either that or have a flag that changes the behaviour from the current behaviour to the new behaviour depending on if the index has finished being created?

Either of those seems doable. Do we have a preferred way of doing that? I think we'd have to wait for the new index regardless since we don't know for sure that an index was done just because we have a new version of Synapse?

clokep · 2022-09-06T18:21:06Z

synapse/storage/schema/main/delta/72/03thread_notifications.sql

+
+ALTER TABLE event_push_actions_staging ADD COLUMN thread_id TEXT;
+
+ALTER TABLE event_push_actions ADD COLUMN thread_id TEXT;


I was not planning to do a background update to "fix" the thread_id for any existing event_push_actions rows -- I think with the way we summarize that's a somewhat futile effort and it should correct itself over time.

clokep · 2022-09-06T18:22:23Z

synapse/storage/schema/main/delta/72/03thread_notifications.sql

+ALTER TABLE event_push_actions_staging ADD COLUMN thread_id TEXT;
+
+ALTER TABLE event_push_actions ADD COLUMN thread_id TEXT;


I wonder if the thread_id column should really live on the events table and not event_push_actions and event_push_actions_staging? This would be a bigger migration, but potentially more useful in the future? I don't have a strong opinion though.

clokep · 2022-09-12T13:13:04Z

Super-seeded by #13776, which is a manual rebase of this on top of #13753.

clokep force-pushed the clokep/thread-notifs branch 6 times, most recently from 80d632a to 23a632e Compare July 6, 2022 17:26

clokep mentioned this pull request Jul 6, 2022

Support thread IDs on receipts (implement MSC3771) #13202

Closed

5 tasks

clokep force-pushed the clokep/thread-notifs branch 2 times, most recently from b325dfc to 067563b Compare July 13, 2022 17:46

clokep force-pushed the clokep/thread-notifs branch 4 times, most recently from a6d67c2 to 9b92fce Compare August 3, 2022 14:46

clokep force-pushed the clokep/thread-notifs branch 2 times, most recently from 405e030 to 7d3e937 Compare August 4, 2022 19:51

clokep added 4 commits August 5, 2022 08:18

Extract the thread ID when processing push rules.

2c7a568

Return thread notification counts down sync.

dfd921d

Add an experimental config option.

e0ed95a

Add a sync flag for unread thread notifications

d56296a

clokep force-pushed the clokep/thread-notifs branch from 7d3e937 to d56296a Compare August 5, 2022 12:18

clokep commented Aug 5, 2022

View reviewed changes

synapse/storage/schema/main/delta/72/03thread_notifications.sql Outdated Show resolved Hide resolved

clokep commented Aug 5, 2022

View reviewed changes

synapse/storage/databases/main/event_push_actions.py Show resolved Hide resolved

clokep commented Aug 5, 2022

View reviewed changes

synapse/storage/databases/main/event_push_actions.py Outdated Show resolved Hide resolved

clokep added 7 commits August 5, 2022 12:58

Reset the notif/unread counts for all summaries before updating the c…

18ea92b

…ounts.

Merge remote-tracking branch 'origin/develop' into clokep/thread-notifs

8978bb7

Sync tests with non-thread version.

dd96e07

Merge remote-tracking branch 'origin/develop' into clokep/thread-notifs

9349642

Make thread_id nullable.

2e85ec6

Properly count the number of items cached by get_unread_event_push_ac…

15afd70

…tions_by_room_for_user.

Merge remote-tracking branch 'origin/develop' into clokep/thread-notifs

621b300

clokep commented Aug 24, 2022

View reviewed changes

synapse/api/filtering.py Outdated Show resolved Hide resolved

clokep added 4 commits August 25, 2022 11:18

Tweaks to index on nulls.

42e6da0

Fix join when rotating notifications.

9ae86cb

Add a where clause when upserting with nulls.

b6e0e68

Merge remote-tracking branch 'origin/develop' into clokep/thread-notifs

000fed4

clokep commented Aug 25, 2022

View reviewed changes

synapse/push/bulk_push_rule_evaluator.py Outdated Show resolved Hide resolved

clokep commented Aug 25, 2022

View reviewed changes

clokep added 3 commits August 25, 2022 13:05

Remove an XXX comment -- this should be fine.

6dcf16d

Merge remote-tracking branch 'origin/develop' into clokep/thread-notifs

430cc0b

Add a versions flag.

4c21565

clokep mentioned this pull request Aug 31, 2022

MSC3773: Notifications for threads matrix-org/matrix-spec-proposals#3773

Merged

clokep requested a review from a team August 31, 2022 14:52

clokep added 2 commits August 31, 2022 11:03

Use an unstable identifier in the sync response.

15bdb62

Use unstable prefixes in filters.

dbb1df3

clokep commented Aug 31, 2022

View reviewed changes

Merge branch 'develop' into clokep/thread-notifs

247132d

erikjohnston reviewed Sep 6, 2022

View reviewed changes

clokep commented Sep 6, 2022

View reviewed changes

clokep added 2 commits September 6, 2022 14:43

Fix tests.

7d29206

Merge remote-tracking branch 'origin/develop' into clokep/thread-notifs

b789c00

This was referenced Sep 8, 2022

Update event push action and receipt tables to support threads. #13753

Merged

Track notification counts per thread (implement MSC3773) (redo) #13776

Merged

clokep closed this Sep 12, 2022

clokep deleted the clokep/thread-notifs branch September 14, 2022 19:18


		ALTER TABLE event_push_actions_staging ADD COLUMN thread_id TEXT;

		ALTER TABLE event_push_actions ADD COLUMN thread_id TEXT;

Uh oh!

Conversation

clokep commented Jul 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

To Do

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clokep commented Aug 31, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erikjohnston commented Sep 2, 2022

Uh oh!

erikjohnston left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clokep commented Sep 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

clokep commented Jul 5, 2022 •

edited

Loading