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

delete messages from device_inbox table when deleting device #10098

Closed
wants to merge 7 commits into from
Closed

delete messages from device_inbox table when deleting device #10098

wants to merge 7 commits into from

Conversation

JohannesKleine
Copy link
Contributor

@JohannesKleine JohannesKleine commented May 30, 2021 •
edited
Loading

Fixes #3599

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.
  • Pull request includes a sign off
  • Code style is correct (run the linters)

@JohannesKleine
Copy link
Contributor Author

#9346 is also related

@JohannesKleine JohannesKleine mentioned this pull request Jun 1, 2021
Merged
4 tasks
@richvdh richvdh requested a review from a team June 3, 2021 15:17
erikjohnston
erikjohnston reviewed Jun 4, 2021
View reviewed changes
synapse/storage/databases/main/devices.py Outdated
@@ -1130,6 +1130,12 @@ async def delete_device(self, user_id: str, device_id: str) -> None:
desc="delete_device",
)

await self.db_pool.simple_delete_one(
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
await self.db_pool.simple_delete_one(
await self.db_pool.simple_delete_many(

Since the table may have many rows (or no rows)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed it

@erikjohnston erikjohnston added the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Jun 16, 2021
@anoadragon453
Copy link
Member

Hey @JohannesKleine, are you able to implement the remaining work on this feature?

@dklimpel
Copy link
Contributor

dklimpel commented Oct 1, 2021

Fixes #3599

@dklimpel dklimpel mentioned this pull request Oct 1, 2021
Merged
4 tasks
@JohannesKleine JohannesKleine changed the title fix #3599 delete messages from device_inbox table when deleting device fixes #3599 delete messages from device_inbox table when deleting device Oct 16, 2021
@JohannesKleine
Copy link
Contributor Author

Just removed the background update related things.

@dklimpel
Copy link
Contributor

I had continued to work on it and also added unit tests. #10969

@JohannesKleine
Copy link
Contributor Author

Saw it (still working on understand your things), left you some input, but this here was unfinished business. If this small change gets merged and my other pull request it could help to keep the db clean until the background job is finished.

@dklimpel
Copy link
Contributor

The queries are in one transaction: #10969 (comment)
And the functions were deduplicated #10969 (comment)

@richvdh richvdh changed the title fixes #3599 delete messages from device_inbox table when deleting device delete messages from device_inbox table when deleting device Oct 18, 2021
@erikjohnston erikjohnston removed the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Oct 29, 2021
@erikjohnston erikjohnston requested a review from a team October 29, 2021 13:54
@erikjohnston
Copy link
Member

Oh, does #10969 make this PR redundant?

@dklimpel
Copy link
Contributor

Oh, does #10969 make this PR redundant?

Yes. IMO this is done already.

@erikjohnston
Copy link
Member

Then I'll close it. Thanks both!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@erikjohnston erikjohnston erikjohnston left review comments

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

Successfully merging this pull request may close these issues.

device_inbox never gets emptied
4 participants
@JohannesKleine @anoadragon453 @dklimpel @erikjohnston