Fetch edits for multiple events in a single query

[clokep]

This fetches edits for multiple events in a single query, which should reduce load when trying to calculate bundled aggregations for many events.

I did some very unscientific testing this seems to result in ~10 - 20% speedup when doing a /sync with a large account. I'm not sure that's accurate to real workloads, but it seems to truly help.

My hope would be to do similar queries for the other bundled aggregations, although I'm only confident this is possible for thread summaries.

Part of #11825.

Based on #11612

Should be reviewable commit-by-commit. Sorry, we're way past that...I can rebase if people would like. 😄

[clokep]

Ideally we would be only pull out the latest edit event per original event, but I haven't found a reasonable way to do that (maybe a lateral join, but that's not supported on sqlite). Any thoughts on how to improve this would be appreciated!

[DMRobertson]

Does latest edit mean "edit with largest depth"?

Might we be able to use a trick like https://stackoverflow.com/a/27802817/5252017 ? (Sorry, not an expert here)

[clokep]

It means the largest origin_server_ts (which is why we're ordering by that descending.)

[reivilibre]

maybe a lateral join, but that's not supported on sqlite

N.B. SQLite does support lateral joins as long as you don't write the word LATERAL — if that's the only thing blocking you, you should be able to work around that by only inserting that word for Postgres.

[clokep]

@reivilibre Do you have any reference for that? I've been unable to get it to work (and my searching online has yielded "you can't do this on SQLite").

[erikjohnston]

On Postgres you can do SELECT DISTINCT ON (original.event_id) ... which will choose the first row (as defined by the ORDER BY clause). I don't think SQLite has that, and so you would need to use a different query, at which point you may as well fall back to the old behaviour.

Potentially you could use window functions and first_value , which I think both postgres and sqlite support, but those sorts of queries really are voodoo magic.

[clokep]

❤️ @erikjohnston Thank you! That gave me some breadcrumbs to realize we do something very similar elsewhere:

synapse/synapse/storage/databases/main/end_to_end_keys.py

Lines 616 to 639 in 3e0536c

	if isinstance(self.database_engine, PostgresEngine):
	# The `DISTINCT ON` clause will pick the *first* row it
	# encounters, so ordering by stream ID desc will ensure we get
	# the latest key.
	sql = """
	SELECT DISTINCT ON (user_id, keytype) user_id, keytype, keydata, stream_id
	FROM e2e_cross_signing_keys
	WHERE %(clause)s
	ORDER BY user_id, keytype, stream_id DESC
	""" % {
	"clause": clause
	}
	else:
	# SQLite has special handling for bare columns when using
	# MIN/MAX with a `GROUP BY` clause where it picks the value from
	# a row that matches the MIN/MAX.
	sql = """
	SELECT user_id, keytype, keydata, MAX(stream_id)
	FROM e2e_cross_signing_keys
	WHERE %(clause)s
	GROUP BY user_id, keytype
	""" % {
	"clause": clause
	}

I think we can abstract out the changes in 8400c20, but that is proving a bit tedious / invasive so I'd like to do it separately.

[clokep]

I'm going to request review on this while leaving it in draft. I suspect there's a bit more to do here, but would like to get some thoughts from the team! I know there's at least a couple of TODO comments to handle.

[clokep]

I rebased this on top of #11659 as I don't see any downside to doing that, see #11659 (comment) for some performance testing of this.

[DMRobertson]

Some of this was a little over my head and I think I might be clutching at straws in places. Hopefully the feedback is vaguely useful? Happy to talk it through.

[DMRobertson]

Does latest edit mean "edit with largest depth"?

Might we be able to use a trick like https://stackoverflow.com/a/27802817/5252017 ? (Sorry, not an expert here)

[DMRobertson]

I've put this back to "draft" because I didn't realise there were some questions/work outstanding at first.

[clokep]

This query seems to not return the expected value. It seems to be unstable -- in the tests the origin_server_ts should never be duplicated, but sometimes this query is returning the max edit.event_id instead.

I suspect we should only ever use a single MAX() (or MIN() to get this behavior from SQLite, trying to see if there's a way to workaround this...

[clokep]

I ended up switching to the older, less efficient version for SQLite (where all edits are pulled) since the MAX/MIN trick doesn't seem to work well when you have multiple columns you want to order over.

[clokep]

I retargeted @matrix-org/synapse-core with my review since this has undergone quite a bit of changes, but it might make sense for @DMRobertson or @reivilibre to look at it again with some previous context?

[DMRobertson]

LGTM, but would appreciate another set of eyes looking over this.

Do we have any tests which make use of the new behaviour (i.e. that would fetch edits for N events in 1 query rather than N?)

[clokep]

Do we have any tests which make use of the new behaviour (i.e. that would fetch edits for N events in 1 query rather than N?)

Yes, tests.rest.client.test_relations.RelationsTestCase.test_multi_edit. I fought with this test being flaky for a long time. 👍

[erikjohnston]

Can we make this class depend on EventsWorkerStore instead of ignoring the error?

[clokep]

I can attempt to do that! Note that we have similar ignores all over, see #11165.

[clokep]

This gives other errors (similar to #11165) about inconsistent MROs. I'm going to leave this to be solved in #11165.

[erikjohnston]

Fair!