Use a database table to hold the users that should have full presence sent to them, instead of something in-memory

[anoadragon453]

The ModuleApi.send_local_online_presence_to method allows Synapse modules to state that a local or remote user should receive all online presence that they are considered "interested in" on their next incremental sync. This would be the same presence information they would receive then if they were doing an initial sync.

The method would simply add user IDs to the in-memory set ModuleApi._send_full_presence_to_local_users. The SyncHandler would then pop them from there when that specific user did a sync. However, this information would only be local to the worker that the module was running on.

A quirk of ModuleApi.send_local_online_presence_to is that it should only be run on workers that have the capability to send federation, such that we can send presence updates to remote users. However, it also needs to be called on sync-capable workers so that they can get access to the in-memory data structure of which users to send presence to on their next sync.

This was a real footgun and something we don't really want to put on the shoulders of sysadmins when they're designing their worker topology. Thus, instead of using the in-memory set, we store this information in a database table instead, which acts as one central list that's shared amongst all users. We also tackle the problem of the list having users added to it, but potentially never removing those users if they don't perform a sync, by expiring entries in the table. This has changed, but there will only ever be at most X rows in the table, where X is the total count of local users.

Reviewable commit-by-commit. Blocked on #9819, as it relies on some changes made in ModuleApi.send_local_online_presence_to. Specifically the bit where it allows non-federation-capable workers to call the function. This has now merged.

[erikjohnston]

Thanks for fixing this up. Can we have some documentation around how to write presence module for workers? I'm trying to figure out of how this all works in practice.

[anoadragon453]

@erikjohnston Great suggestions! We now store a stream ID in the table instead, so that receiving full presence from multiple devices works. A couple outstanding questions I had:

• I'm not sure whether I need an index for (user_id, presence_stream_id) on users_to_send_full_presence_to, as there's already an index on user_id via PRIMARY KEY. I tried EXPLAIN ANALYZEing on a test table, but the primary and foreign keys in there makes inserting test data a bit complicated.
• Do I need to do anything special to have the new database methods run properly on workers? I'm currently configuring the test_send_local_online_presence_to and test_send_local_online_presence_to_federation unit tests to run on workers.

[erikjohnston]

@erikjohnston Great suggestions! We now store a stream ID in the table instead, so that receiving full presence from multiple devices works. A couple outstanding questions I had:

• I'm not sure whether I need an index for (user_id, presence_stream_id) on users_to_send_full_presence_to, as there's already an index on user_id via PRIMARY KEY. I tried EXPLAIN ANALYZEing on a test table, but the primary and foreign keys in there makes inserting test data a bit complicated.

No you don't need an index as there is only one row per user_id, i.e. looking up all rows that match the user_id in the index will return a single row and so there is no need to split the index further. (If it returned many many rows than adding the index (user_id, presence_stream_id) would allow you to split the returned rows by stream ID).

• Do I need to do anything special to have the new database methods run properly on workers? I'm currently configuring the test_send_local_online_presence_to and test_send_local_online_presence_to_federation unit tests to run on workers.

The DB function needs to be in a class that gets added to the generic worker store in synapse.app.generic_worker

[anoadragon453]

Right, this PR has been updated to fix the worker-based tests. They should pass now.

No you don't need an index as there is only one row per user_id, i.e. looking up all rows that match the user_id in the index will return a single row and so there is no need to split the index further. (If it returned many many rows than adding the index (user_id, presence_stream_id) would allow you to split the returned rows by stream ID).

Thanks for the answer, and for explaining how multi-column indexes work in an intuitive way!

The DB function needs to be in a class that gets added to the generic worker store in synapse.app.generic_worker

PresenceStore.add_users_to_send_full_presence_to looks to work on both workers and the main process. Rejoice!

stale

[anoadragon453]

I'd like to also update the worker docs to mention that we can write to the presence stream now, since we link to them here. But that can be done in a separate PR.