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

Purge chain cover tables when purging events. #9498

Merged
merged 5 commits into from
Mar 3, 2021
Merged

Purge chain cover tables when purging events. #9498

merged 5 commits into from
Mar 3, 2021

Conversation

clokep
Copy link
Member

@clokep clokep commented Feb 25, 2021 •
edited
Loading

When purging a room we should be purging the corresponding entries on the chain cover index.

Two of the tables (event_auth_chains and event_auth_chains_to_calculate) are easy since they reference event_id, but event_auth_chain_links is a bit harder since it does not.

I'm not really sure this is the correct approach, but would like some feedback.

This seems to fix #9481.

I'd like to include a fix for people who historically left a room as well, but I'm not sure there's any good way to do that. (Trying to find all events which no longer exist seems like it would be expensive.)

I do not think the same code should be applied to purge_history (as opposed to purge_room) in this file since that doesn't seem to remove state events.

@waclaw66 waclaw66 mentioned this pull request Feb 25, 2021
Closed
@clokep clokep force-pushed the clokep/purge-event-auth-chain branch from 1414c06 to 80c6667 Compare February 25, 2021 19:09
@clokep clokep force-pushed the clokep/purge-event-auth-chain branch from 80c6667 to fb83f9a Compare February 25, 2021 19:21
@clokep clokep requested a review from a team February 25, 2021 19:30
@richvdh
Copy link
Member

richvdh commented Feb 25, 2021

shouldn't we have had some foreign key references which would have caught this problem earlier? :/

@clokep
Copy link
Member Author

clokep commented Feb 26, 2021

shouldn't we have had some foreign key references which would have caught this problem earlier? :/

I think most of our tables don't have foreign keys. I'd like to at least add a test that joins a room, leaves that room, then re-joins it. 😢

@richvdh
Copy link
Member

richvdh commented Feb 26, 2021

I think most of our tables don't have foreign keys

this is sort of my point. These are recently added tables; why are we still not using foreign keys? And sorry, this isn't really the place to debate it, I'm just a bit sad that we keep repeating historical mistakes.

@richvdh
Copy link
Member

richvdh commented Feb 26, 2021

btw the approach here looks correct; but I'm not that familiar with the auth chain stuff, so I can't speak to whether the details are correct.

I do not think the same code should be applied to purge_history (as opposed to purge_room) in this file since that doesn't seem to remove state events.

I would still think we should at least delete the relevant rows in event_auth_chains ?

@clokep
Copy link
Member Author

clokep commented Mar 2, 2021

@erikjohnston Any thoughts on if it would make sense to do a background update to try to fix previously purged rooms?

@erikjohnston
Copy link
Member

@erikjohnston Any thoughts on if it would make sense to do a background update to try to fix previously purged rooms?

Ugh, we probably should do something to fix this I guess, as currently there's no way of getting out of this mess when you discover it. I think its just a case of iterating over all the rows in event_auth_chains and removing them if we don't have an associated event in the events table

@waclaw66
Copy link

waclaw66 commented Mar 3, 2021 •
edited
Loading

Is there any particular reason why Synapse doesn't use foreign keys? They would make all the maintenance much easier and keep database consistent.

@richvdh
Copy link
Member

richvdh commented Mar 3, 2021

Is there any particular reason why Synapse doesn't use foreign keys? They would make all the maintenance much easier and keep database consistent.

I feel like I raised this point earlier: #9498 (comment)

@clokep
Copy link
Member Author

clokep commented Mar 3, 2021

From our meeting today we discussed:

  • Merging this as is.
  • Following-up with a background update to fix for people who have already purged rooms.

@clokep clokep marked this pull request as ready for review March 3, 2021 15:19
@clokep clokep merged commit 922788c into develop Mar 3, 2021
@clokep clokep deleted the clokep/purge-event-auth-chain branch March 3, 2021 16:04
@clokep clokep mentioned this pull request Mar 3, 2021
Merged
clokep added a commit that referenced this pull request Mar 9, 2021
@clokep
Add a background task to purge unused chain IDs. (#9542)
dc51d8f 
This is a companion change to apply the fix in #9498 /
922788c to previously
purged rooms.
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Mar 14, 2021
@Midar
Update chat/matrix-synapse to 1.29.0
d5b1cc5 
Synapse 1.29.0 (2021-03-08)
===========================

Note that synapse now expects an `X-Forwarded-Proto` header when used with a reverse proxy. Please see [UPGRADE.rst](UPGRADE.rst#upgrading-to-v1290) for more details on this change.


No significant changes.


Synapse 1.29.0rc1 (2021-03-04)
==============================

Features
--------

- Add rate limiters to cross-user key sharing requests. ([\#8957](matrix-org/synapse#8957))
- Add `order_by` to the admin API `GET /_synapse/admin/v1/users/<user_id>/media`. Contributed by @dklimpel. ([\#8978](matrix-org/synapse#8978))
- Add some configuration settings to make users' profile data more private. ([\#9203](matrix-org/synapse#9203))
- The `no_proxy` and `NO_PROXY` environment variables are now respected in proxied HTTP clients with the lowercase form taking precedence if both are present. Additionally, the lowercase `https_proxy` environment variable is now respected in proxied HTTP clients on top of existing support for the uppercase `HTTPS_PROXY` form and takes precedence if both are present. Contributed by Timothy Leung. ([\#9372](matrix-org/synapse#9372))
- Add a configuration option, `user_directory.prefer_local_users`, which when enabled will make it more likely for users on the same server as you to appear above other users. ([\#9383](matrix-org/synapse#9383), [\#9385](matrix-org/synapse#9385))
- Add support for regenerating thumbnails if they have been deleted but the original image is still stored. ([\#9438](matrix-org/synapse#9438))
- Add support for `X-Forwarded-Proto` header when using a reverse proxy. ([\#9472](matrix-org/synapse#9472), [\#9501](matrix-org/synapse#9501), [\#9512](matrix-org/synapse#9512), [\#9539](matrix-org/synapse#9539))


Bugfixes
--------

- Fix a bug where users' pushers were not all deleted when they deactivated their account. ([\#9285](matrix-org/synapse#9285), [\#9516](matrix-org/synapse#9516))
- Fix a bug where a lot of unnecessary presence updates were sent when joining a room. ([\#9402](matrix-org/synapse#9402))
- Fix a bug that caused multiple calls to the experimental `shared_rooms` endpoint to return stale results. ([\#9416](matrix-org/synapse#9416))
- Fix a bug in single sign-on which could cause a "No session cookie found" error. ([\#9436](matrix-org/synapse#9436))
- Fix bug introduced in v1.27.0 where allowing a user to choose their own username when logging in via single sign-on did not work unless an `idp_icon` was defined. ([\#9440](matrix-org/synapse#9440))
- Fix a bug introduced in v1.26.0 where some sequences were not properly configured when running `synapse_port_db`. ([\#9449](matrix-org/synapse#9449))
- Fix deleting pushers when using sharded pushers. ([\#9465](matrix-org/synapse#9465), [\#9466](matrix-org/synapse#9466), [\#9479](matrix-org/synapse#9479), [\#9536](matrix-org/synapse#9536))
- Fix missing startup checks for the consistency of certain PostgreSQL sequences. ([\#9470](matrix-org/synapse#9470))
- Fix a long-standing bug where the media repository could leak file descriptors while previewing media. ([\#9497](matrix-org/synapse#9497))
- Properly purge the event chain cover index when purging history. ([\#9498](matrix-org/synapse#9498))
- Fix missing chain cover index due to a schema delta not being applied correctly. Only affected servers that ran development versions. ([\#9503](matrix-org/synapse#9503))
- Fix a bug introduced in v1.25.0 where `/_synapse/admin/join/` would fail when given a room alias. ([\#9506](matrix-org/synapse#9506))
- Prevent presence background jobs from running when presence is disabled. ([\#9530](matrix-org/synapse#9530))
- Fix rare edge case that caused a background update to fail if the server had rejected an event that had duplicate auth events. ([\#9537](matrix-org/synapse#9537))


Improved Documentation
----------------------

- Update the example systemd config to propagate reloads to individual units. ([\#9463](matrix-org/synapse#9463))


Internal Changes
----------------

- Add documentation and type hints to `parse_duration`. ([\#9432](matrix-org/synapse#9432))
- Remove vestiges of `uploads_path` configuration setting. ([\#9462](matrix-org/synapse#9462))
- Add a comment about systemd-python. ([\#9464](matrix-org/synapse#9464))
- Test that we require validated email for email pushers. ([\#9496](matrix-org/synapse#9496))
- Allow python to generate bytecode for synapse. ([\#9502](matrix-org/synapse#9502))
- Fix incorrect type hints. ([\#9515](matrix-org/synapse#9515), [\#9518](matrix-org/synapse#9518))
- Add type hints to device and event report admin API. ([\#9519](matrix-org/synapse#9519))
- Add type hints to user admin API. ([\#9521](matrix-org/synapse#9521))
- Bump the versions of mypy and mypy-zope used for static type checking. ([\#9529](matrix-org/synapse#9529))
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@erikjohnston erikjohnston erikjohnston approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Federated room cannot be joined again when left and deleted before
4 participants
@clokep @richvdh @erikjohnston @waclaw66