Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use receipts event_stream_ordering instead of joins #17032

Merged
merged 3 commits into from
Apr 12, 2024

Conversation

Fizzadar
Copy link
Contributor

@Fizzadar Fizzadar commented Mar 26, 2024

Resurrecting matrix-org/synapse#13918.

This should reduce IOPs incurred by joining to the events table to lookup stream ordering, which happens in many receipt handling code paths. Like the previous PR I believe sufficient time has passed between the original migration in DB schema 72 and now to merge this as-is. It's highly unlikely that both the migration is still ongoing AND (active) users still have any receipts prior to that date.

In the unlikely event there is a receipt without a populated event_stream_ordering synapse will behave just as it does now when receipts exist for events that don't (yet): for push action calculation the receipts are just ignored.

I've removed the validation on event IDs as this is already covered here:

# Ensure the room/event exists, this will raise an error if the user
# cannot view the event.
if not await self.event_handler.get_event(user_id, room_id, event_id):
return

Pull Request Checklist

This should reduce IOPs incurred by joining to the events table to lookup
stream ordering, which happens in many receipt handling code paths.
@CLAassistant
Copy link

CLAassistant commented Mar 26, 2024

CLA assistant check
All committers have signed the CLA.

@Fizzadar Fizzadar marked this pull request as ready for review March 27, 2024 10:14
@Fizzadar Fizzadar requested a review from a team as a code owner March 27, 2024 10:14
@erikjohnston
Copy link
Member

FTR the row was added in v1.68.0 in Sep 2022

Copy link
Member

@erikjohnston erikjohnston left a comment

Choose a reason for hiding this comment

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

Sorry for the delay in review, let's give this a go!

@erikjohnston erikjohnston merged commit fe4719a into element-hq:develop Apr 12, 2024
38 checks passed
@Fizzadar Fizzadar deleted the use-receipts-stream-ordering branch April 12, 2024 08:59
@@ -736,8 +735,7 @@ def _insert_linearized_receipt_txn(
thread_args = (thread_id,)

sql = f"""
SELECT stream_ordering, event_id FROM events
INNER JOIN receipts_linearized AS r USING (event_id, room_id)
SELECT r.event_stream_ordering, r.event_id FROM receipts_linearized AS r
WHERE r.room_id = ? AND r.receipt_type = ? AND r.user_id = ? AND {thread_clause}
Copy link
Member

Choose a reason for hiding this comment

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

I actually had some empty stream orderings in this for remote users, and the code below assumes stream_ordering is not None. I added a WHERE event_stream_ordering IS NOT NULL, and that seemed to have fixed it. @Fizzadar I think that patch makes sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep that makes sense, thanks for catching that!

erikjohnston added a commit that referenced this pull request Apr 15, 2024
erikjohnston added a commit that referenced this pull request Apr 17, 2024
yingziwu added a commit to yingziwu/synapse that referenced this pull request May 3, 2024
No significant changes since 1.106.0rc1.

- Send an email if the address is already bound to an user account. ([\#16819](element-hq/synapse#16819))
- Implement the rendezvous mechanism described by [MSC4108](matrix-org/matrix-spec-proposals#4108). ([\#17056](element-hq/synapse#17056))
- Support delegating the rendezvous mechanism described [MSC4108](matrix-org/matrix-spec-proposals#4108) to an external implementation. ([\#17086](element-hq/synapse#17086))

- Add validation to ensure that the `limit` parameter on `/publicRooms` is non-negative. ([\#16920](element-hq/synapse#16920))
- Return `400 M_NOT_JSON` upon receiving invalid JSON in query parameters across various client and admin endpoints, rather than an internal server error. ([\#16923](element-hq/synapse#16923))
- Make the CSAPI endpoint `/keys/device_signing/upload` idempotent. ([\#16943](element-hq/synapse#16943))
- Redact membership events if the user requested erasure upon deactivating. ([\#17076](element-hq/synapse#17076))

- Add a prompt in the contributing guide to manually configure icu4c. ([\#17069](element-hq/synapse#17069))
- Clarify what part of message retention is still experimental. ([\#17099](element-hq/synapse#17099))

- Use new receipts column to optimise receipt and push action SQL queries. Contributed by Nick @ Beeper (@Fizzadar). ([\#17032](element-hq/synapse#17032), [\#17096](element-hq/synapse#17096))
- Fix mypy with latest Twisted release. ([\#17036](element-hq/synapse#17036))
- Bump minimum supported Rust version to 1.66.0. ([\#17079](element-hq/synapse#17079))
- Add helpers to transform Twisted requests to Rust http Requests/Responses. ([\#17081](element-hq/synapse#17081))
- Fix type annotation for `visited_chains` after `mypy` upgrade. ([\#17125](element-hq/synapse#17125))

* Bump anyhow from 1.0.81 to 1.0.82. ([\#17095](element-hq/synapse#17095))
* Bump peaceiris/actions-gh-pages from 3.9.3 to 4.0.0. ([\#17087](element-hq/synapse#17087))
* Bump peaceiris/actions-mdbook from 1.2.0 to 2.0.0. ([\#17089](element-hq/synapse#17089))
* Bump pyasn1-modules from 0.3.0 to 0.4.0. ([\#17093](element-hq/synapse#17093))
* Bump pygithub from 2.2.0 to 2.3.0. ([\#17092](element-hq/synapse#17092))
* Bump ruff from 0.3.5 to 0.3.7. ([\#17094](element-hq/synapse#17094))
* Bump sigstore/cosign-installer from 3.4.0 to 3.5.0. ([\#17088](element-hq/synapse#17088))
* Bump twine from 4.0.2 to 5.0.0. ([\#17091](element-hq/synapse#17091))
* Bump types-pillow from 10.2.0.20240406 to 10.2.0.20240415. ([\#17090](element-hq/synapse#17090))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants