Delete push actions on receipt #13834
Conversation
This matches the existing logic used in the push action processing, now that event push actions is very regularly summarised the table should be relatively lean and the delete low impact.
|
Ah, alas we had to move away from this approach as on busy servers there was too much contention, resulting in lots of |
|
@erikjohnston makes sense, although I was thinking for #13117 was that the contention is on the summary table rather than the push actions table? |
|
Merged in develop to fixup the receipts test, complement still flaking though but unrelated 😬! |
There was a problem hiding this comment.
also caught a bug and added an additional test for that
Yay testing works 🎉
It looks pretty much good to me apart from a few bits of documentation. I'd also like @erikjohnston to have a look since it sounded like he had concern last week.
| result = self.get_success( | ||
| self.store.db_pool.simple_select_list( | ||
| table="event_push_actions", | ||
| keyvalues={"1": 1}, |
There was a problem hiding this comment.
You can also just set keyvalues to None which generates an SQL query with no WHERE clause
| @@ -290,3 +290,76 @@ def test_get_last_receipt_event_id_for_user(self) -> None: | |||
| ) | |||
| ) | |||
| self.assertEqual(res, event2_1_id) | |||
|
|
|||
| def test_receipts_clear_push_actions(self) -> None: | |||
There was a problem hiding this comment.
Could you add a docstring explaining what this test tests please? Same with the other test.
| WHERE room_id = ? | ||
| AND user_id = ? | ||
| AND stream_ordering <= ? | ||
| AND highlight = 0 |
There was a problem hiding this comment.
Could you add a comment explaining why we are special casing highlights here?
| event_1_id = self.create_and_send_event( | ||
| self.room_id1, | ||
| UserID.from_string(OTHER_USER_ID), | ||
| content="our", |
There was a problem hiding this comment.
| content="our", | |
| content=UserID.from_string(OUR_USER_ID).localpart, |
Bit more boilerplatey, but it makes it easier to understand what "our" is supposed to be here (and that it's not just a magic value we're not pulling out of nowhere).
| if stream_ordering is not None and receipt_type in ( | ||
| ReceiptTypes.READ, | ||
| ReceiptTypes.READ_PRIVATE, | ||
| ): | ||
| sql = """ | ||
| DELETE FROM event_push_actions | ||
| WHERE room_id = ? | ||
| AND user_id = ? | ||
| AND stream_ordering <= ? | ||
| AND highlight = 0 | ||
| """ | ||
| txn.execute(sql, (room_id, user_id, stream_ordering)) |
There was a problem hiding this comment.
This query is incorrect if a thread_id is provided.
It should be something like:
| if stream_ordering is not None and receipt_type in ( | |
| ReceiptTypes.READ, | |
| ReceiptTypes.READ_PRIVATE, | |
| ): | |
| sql = """ | |
| DELETE FROM event_push_actions | |
| WHERE room_id = ? | |
| AND user_id = ? | |
| AND stream_ordering <= ? | |
| AND highlight = 0 | |
| """ | |
| txn.execute(sql, (room_id, user_id, stream_ordering)) | |
| if stream_ordering is not None and receipt_type in ( | |
| ReceiptTypes.READ, | |
| ReceiptTypes.READ_PRIVATE, | |
| ): | |
| args = [room_id, user_id, stream_ordering) | |
| where_clause = "" | |
| if thread_id is not None: | |
| where_clause = "AND thread_id = ?" | |
| args.append(thread_id) | |
| sql = f""" | |
| DELETE FROM event_push_actions | |
| WHERE room_id = ? | |
| AND user_id = ? | |
| AND stream_ordering <= ? | |
| AND highlight = 0 | |
| {threads_clause} | |
| """ | |
| txn.execute(sql, args) |
Although I'm not really confident of the interaction between this change and #13776 or how this interacts with _remove_old_push_actions_that_have_rotated which is unchanged in this PR.
Hmm, true. I'm struggling to reason about if this will suffer from a similar problem. Have you tried this on a busy server? |
|
Been distracted with other priorities the last few weeks! I am just rolling this (only the delete query) out to our synapse install now to confirm there isn't a contention issue on the delete. Assuming that's the case I'll update for threads/etc and cleanup the rotation parts too. |
# Conflicts: # synapse/storage/databases/main/event_push_actions.py
|
Hm so the failing test here is a test that push a highlight action, which never gets cleared, but the test is selecting push actions over this time period ( ).Because they don't get deleted this means if a pusher fails for a bit once it recovers all highlight push actions will be notified, irrespective of read receipt position. The only way to maintain support for that situation is to check receipts at fetch time, so maybe there's no point in this 😩. |
|
Based on my last comment about highlights also using the |
Further work on optimising push action processing. Now that the push actions table is regularly summarised moving to delete-on-receipt seems appropriate.
I've not touched any of the receipt handling code in the event push actions store, or deleting from event push summary, both of which might be good future improvements?
Signed off by Nick @ Beeper (@Fizzadar).
Pull Request Checklist
(run the linters)