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

Document easy room purge benefit of using (room_id, event_id)#13771

Closed
MadLittleMods wants to merge 4 commits intodevelopfrom
madlittlemods/document-benefit-for-event_id-room_id-pair-for-purging
Closed

Document easy room purge benefit of using (room_id, event_id)#13771
MadLittleMods wants to merge 4 commits intodevelopfrom
madlittlemods/document-benefit-for-event_id-room_id-pair-for-purging

Conversation

@MadLittleMods
Copy link
Contributor

@MadLittleMods MadLittleMods commented Sep 9, 2022 •
edited
Loading

Document easy room purge benefit of using (room_id, event_id)

Discussed in the backend chapter sync as mentioned by @erikjohnston,
https://docs.google.com/document/d/1kmGRzPFfg_gRY6l0sxjYkSLW6UpMFn9ELQX5CtTLWlA/edit#bookmark=id.ciuq6xs2t47

Follow-up to #13701

Dev notes

Database tables which don't have a (room_id, event_id) index,

synapse/synapse/storage/databases/main/purge_events.py

Lines 388 to 412 in fa2f3d8

# Now we delete tables which lack an index on room_id but have one on event_id
for table in (
"event_auth",
"event_edges",
"event_json",
"event_push_actions_staging",
"event_relations",
"event_to_state_groups",
"event_auth_chains",
"event_auth_chain_to_calculate",
"redactions",
"rejections",
"state_events",
):
logger.info("[purge] removing %s from %s", room_id, table)
txn.execute(
"""
DELETE FROM %s WHERE event_id IN (
SELECT event_id FROM events WHERE room_id=?
)
"""
% (table,),
(room_id,),
)

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
    • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
  • Pull request includes a sign off
  • Code style is correct
    (run the linters)

@MadLittleMods MadLittleMods added T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks. A-Docs things relating to the documentation T-Enhancement New features, changes in functionality, improvements in performance, or user-facing enhancements. and removed T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks. labels Sep 9, 2022
@MadLittleMods MadLittleMods marked this pull request as ready for review September 9, 2022 19:54
@MadLittleMods MadLittleMods requested a review from a team as a code owner September 9, 2022 19:54
DMRobertson
DMRobertson reviewed Sep 12, 2022
View reviewed changes
docs/development/database_schema.md
`event_id` in the database schema, it should be also accompanied by `room_id`
(`PRIMARY KEY (room_id, event_id)`) and lookups should be done through the pair
`(room_id, event_id)`.
`(room_id, event_id)`. Another benefit of scoping `event_ids` to the room is
Copy link
Contributor

@DMRobertson DMRobertson Sep 12, 2022

Choose a reason for hiding this comment

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

I'm not sure I follow this reasoning. Is the point that we can do this with a single DELETE FROM ... WHERE ... rather than having to use a subselect or similar which joins to the events table?

Copy link
Contributor Author

@MadLittleMods MadLittleMods Sep 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@DMRobertson Seems to be the case so we don't have to do this:

synapse/synapse/storage/databases/main/purge_events.py

Lines 388 to 412 in fa2f3d8

# Now we delete tables which lack an index on room_id but have one on event_id
for table in (
"event_auth",
"event_edges",
"event_json",
"event_push_actions_staging",
"event_relations",
"event_to_state_groups",
"event_auth_chains",
"event_auth_chain_to_calculate",
"redactions",
"rejections",
"state_events",
):
logger.info("[purge] removing %s from %s", room_id, table)
txn.execute(
"""
DELETE FROM %s WHERE event_id IN (
SELECT event_id FROM events WHERE room_id=?
)
"""
% (table,),
(room_id,),
)

From the chapter sync, it was one of the useful benefits people liked from pairing up (room_id, event_id).

Added a note on this part of why it's easier ⏩

@MadLittleMods MadLittleMods mentioned this pull request Sep 14, 2022
Merged
4 tasks
MadLittleMods added a commit that referenced this pull request Sep 26, 2022
@MadLittleMods
Emphasize the right reasons to use (room_id, event_id)
563cb35 
Follow-up to:
 - #13701
 - #13771
@MadLittleMods MadLittleMods mentioned this pull request Sep 26, 2022
Merged
4 tasks
@MadLittleMods
Copy link
Contributor Author

MadLittleMods commented Sep 26, 2022

Closing in favor of #13915

richvdh pushed a commit that referenced this pull request Sep 27, 2022
@MadLittleMods
Emphasize the right reasons to use (room_id, event_id) in a schema (#…
35e9d6a 
…13915)

* Emphasize the right reasons to use (room_id, event_id)

Follow-up to:
 - #13701
 - #13771
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@DMRobertson DMRobertson Awaiting requested review from DMRobertson

Assignees

No one assigned

Labels

A-Docs things relating to the documentation T-Enhancement New features, changes in functionality, improvements in performance, or user-facing enhancements.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@MadLittleMods @DMRobertson