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

delete messages from device_inbox table when deleting device#10098

Closed
JohannesKleine wants to merge 7 commits intomatrix-org:developfrom
JohannesKleine:fix-#3599
Closed

delete messages from device_inbox table when deleting device#10098
JohannesKleine wants to merge 7 commits intomatrix-org:developfrom
JohannesKleine:fix-#3599

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

JohannesKleine commented May 30, 2021

#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
desc="delete_device",
)

await self.db_pool.simple_delete_one(
Copy link
Member

@erikjohnston erikjohnston Jun 4, 2021

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

@JohannesKleine JohannesKleine Oct 16, 2021

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

anoadragon453 commented Sep 14, 2021

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

JohannesKleine commented Oct 16, 2021

Just removed the background update related things.

@dklimpel
Copy link
Contributor

dklimpel commented Oct 16, 2021

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

@JohannesKleine
Copy link
Contributor Author

JohannesKleine commented Oct 16, 2021

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

dklimpel commented Oct 16, 2021

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

erikjohnston commented Oct 29, 2021

Oh, does #10969 make this PR redundant?

@dklimpel
Copy link
Contributor

dklimpel commented Oct 29, 2021

Oh, does #10969 make this PR redundant?

Yes. IMO this is done already.

@erikjohnston
Copy link
Member

erikjohnston commented Oct 29, 2021

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

1 more reviewer

@erikjohnston erikjohnston erikjohnston left review comments

Reviewers whose approvals may not affect merge requirements

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