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

Stop synapse from saving messages in device_inbox for hidden devices. #10097

Merged
anoadragon453 merged 9 commits into from
Nov 1, 2021
Merged

Stop synapse from saving messages in device_inbox for hidden devices. #10097

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0debae7
fix #9348
JohannesKleine May 30, 2021
e403466
add changelog file
JohannesKleine May 30, 2021
da8520d
Update changelog.d/10097.bugfix
JohannesKleine Jun 1, 2021
af12176
adopt recommended changes suggested in the pull reques comments
JohannesKleine Jun 1, 2021
1718981
Merge remote-tracking branch 'origin/develop' into fix-#9348
JohannesKleine Jun 1, 2021
7066cc2
Update changelog.d/10097.bugfix
JohannesKleine Oct 16, 2021
cd661dd
rmeove outdated sql schema update file
JohannesKleine Oct 16, 2021
9258d1f
Merge remote-tracking branch 'origin/develop' into HEAD
JohannesKleine Oct 16, 2021
ac31155
send-to-device events -> to-device messages
anoadragon453 Oct 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/10097.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix a bug which is stopping synapse from storing messages in `device_inbox` for hidden devices.
4 changes: 2 additions & 2 deletions synapse/storage/databases/main/deviceinbox.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -490,7 +490,7 @@ def _add_messages_to_local_device_inbox_txn(
devices = self.db_pool.simple_select_onecol_txn(
txn,
table="devices",
keyvalues={"user_id": user_id},
keyvalues={"user_id": user_id, "hidden": False},
retcol="device_id",
)

Expand All @@ -506,7 +506,7 @@ def _add_messages_to_local_device_inbox_txn(
rows = self.db_pool.simple_select_many_txn(
txn,
table="devices",
keyvalues={"user_id": user_id},
keyvalues={"user_id": user_id, "hidden": False},
column="device_id",
iterable=devices,
retcols=("device_id",),
Expand Down
20 changes: 20 additions & 0 deletions synapse/storage/schema/main/delta/60/99remove_hidden_devices_from_device_inbox.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/* Copyright 2021 The Matrix.org Foundation C.I.C
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

--- Remove messages to hidden devices from the device_inbox table.
--- This schould run as the last task, it may take a little bit longer
--- to finish.

DELETE FROM device_inbox WHERE device_id IN (SELECT device_id FROM devices WHERE hidden = TRUE);
Copy link
Copy Markdown
Member

@anoadragon453 anoadragon453 Jun 1, 2021

Choose a reason for hiding this comment

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

I'm afraid this query is quite slow on large deployments (testing it out in a dry run on matrix.org, I had to cancel after 5 minutes). Database migrations such as this block homeserver startup until they're complete, and thus we tend to avoid having long-running ones.

As such, I think this would be better suited as a background update, which run in the background after the server has already started up.

Here's an example of turning an existing database migration into a background update: https://github.com/matrix-org/synapse/pull/9536/files

Copy link
Copy Markdown
Contributor Author

@JohannesKleine JohannesKleine Jun 1, 2021

Choose a reason for hiding this comment

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

Yes it will run a long time. Here manually with 110 000 000 rows it took few hours. For small homeservers it would be fine. Do i understand it correctly it will iterate over every row in the table? If it does, it would be useful to add a third pr which removes the sql delta files here and in #10098 and do something like this as background update instead.

DELETE FROM device_inbox 
    WHERE device_id IN (SELECT device_id FROM devices WHERE hidden = TRUE)
    OR WHERE device_id NOT IN (SELECT device_id FROM devices);

Copy link
Copy Markdown
Contributor Author

@JohannesKleine JohannesKleine Jun 1, 2021

Choose a reason for hiding this comment

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

And gives the opportunity to test it now, with the slow version.

Copy link
Copy Markdown
Member

@anoadragon453 anoadragon453 Jun 17, 2021

Choose a reason for hiding this comment

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

Good point, it's probably best to encapsulate the background job in a separate PR as it's not required for the main functionality of this PR anyhow.

Do i understand it correctly it will iterate over every row in the table?

The currently active indexes on this table are:

Indexes:
    "device_inbox_stream_id" btree (stream_id)
    "device_inbox_user_stream_id" btree (user_id, device_id, stream_id)

Because of the lack of stream_id, I believe we'd need to iterate over the table.

And gives the opportunity to test it now, with the slow version.

Sure, people can run the SQL during operation manually if they'd wish, and then we just add a background update separately to clean it out in parallel for larger servers 🙂

Copy link
Copy Markdown
Contributor

@bradtgmurray bradtgmurray Aug 31, 2021

Choose a reason for hiding this comment

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

A little off topic for here, but I noticed in our database while trying to do a similar cleanup that even selecting the rows to be removed was a really slow operation. Something like SELECT * FROM device_inbox WHERE device_id = 'foo' took several minutes to run. We might have to be a little clever in how we clean up this table.