From a2c74d05bce4c40b4b18b69365ba6cd80474ef0f Mon Sep 17 00:00:00 2001 From: Christoph Settgast Date: Sat, 4 May 2024 18:04:30 +0200 Subject: [PATCH 1/3] Partial revert #17044 to fix sql performance regression For details see #17129. Compared to a full revert, we keep the class method instroduced in #17129, as its used elsewhere by now Fixes: #17129 Signed-off-by: Christoph Settgast --- .../databases/main/event_federation.py | 99 ++++++++++++++++++- 1 file changed, 96 insertions(+), 3 deletions(-) diff --git a/synapse/storage/databases/main/event_federation.py b/synapse/storage/databases/main/event_federation.py index fb132ef090..97ac07c3a3 100644 --- a/synapse/storage/databases/main/event_federation.py +++ b/synapse/storage/databases/main/event_federation.py @@ -280,16 +280,64 @@ class EventFederationWorkerStore(SignatureWorkerStore, EventsWorkerStore, SQLBas # Now we look up all links for the chains we have, adding chains that # are reachable from any event. + # + # This query is structured to first get all chain IDs reachable, and + # then pull out all links from those chains. This does pull out more + # rows than is strictly necessary, however there isn't a way of + # structuring the recursive part of query to pull out the links without + # also returning large quantities of redundant data (which can make it a + # lot slower). + sql = """ + WITH RECURSIVE links(chain_id) AS ( + SELECT + DISTINCT origin_chain_id + FROM event_auth_chain_links WHERE %s + UNION + SELECT + target_chain_id + FROM event_auth_chain_links + INNER JOIN links ON (chain_id = origin_chain_id) + ) + SELECT + origin_chain_id, origin_sequence_number, + target_chain_id, target_sequence_number + FROM links + INNER JOIN event_auth_chain_links ON (chain_id = origin_chain_id) + """ # A map from chain ID to max sequence number *reachable* from any event ID. chains: Dict[int, int] = {} - for links in self._get_chain_links(txn, set(event_chains.keys())): + + # Add all linked chains reachable from initial set of chains. + chains_to_fetch = set(event_chains.keys()) + while chains_to_fetch: + batch2 = tuple(itertools.islice(chains_to_fetch, 1000)) + chains_to_fetch.difference_update(batch2) + clause, args = make_in_list_sql_clause( + txn.database_engine, "origin_chain_id", batch2 + ) + txn.execute(sql % (clause,), args) + + links: Dict[int, List[Tuple[int, int, int]]] = {} + + for ( + origin_chain_id, + origin_sequence_number, + target_chain_id, + target_sequence_number, + ) in txn: + links.setdefault(origin_chain_id, []).append( + (origin_sequence_number, target_chain_id, target_sequence_number) + ) + for chain_id in links: if chain_id not in event_chains: continue _materialize(chain_id, event_chains[chain_id], links, chains) + chains_to_fetch.difference_update(chains) + # Add the initial set of chains, excluding the sequence corresponding to # initial event. for chain_id, seq_no in event_chains.items(): @@ -579,9 +627,53 @@ class EventFederationWorkerStore(SignatureWorkerStore, EventsWorkerStore, SQLBas # Now we look up all links for the chains we have, adding chains that # are reachable from any event. + # + # This query is structured to first get all chain IDs reachable, and + # then pull out all links from those chains. This does pull out more + # rows than is strictly necessary, however there isn't a way of + # structuring the recursive part of query to pull out the links without + # also returning large quantities of redundant data (which can make it a + # lot slower). + sql = """ + WITH RECURSIVE links(chain_id) AS ( + SELECT + DISTINCT origin_chain_id + FROM event_auth_chain_links WHERE %s + UNION + SELECT + target_chain_id + FROM event_auth_chain_links + INNER JOIN links ON (chain_id = origin_chain_id) + ) + SELECT + origin_chain_id, origin_sequence_number, + target_chain_id, target_sequence_number + FROM links + INNER JOIN event_auth_chain_links ON (chain_id = origin_chain_id) + """ + + # (We need to take a copy of `seen_chains` as we want to mutate it in + # the loop) + chains_to_fetch = set(seen_chains) + while chains_to_fetch: + batch2 = tuple(itertools.islice(chains_to_fetch, 1000)) + clause, args = make_in_list_sql_clause( + txn.database_engine, "origin_chain_id", batch2 + ) + txn.execute(sql % (clause,), args) + + links: Dict[int, List[Tuple[int, int, int]]] = {} + + for ( + origin_chain_id, + origin_sequence_number, + target_chain_id, + target_sequence_number, + ) in txn: + links.setdefault(origin_chain_id, []).append( + (origin_sequence_number, target_chain_id, target_sequence_number) + ) - # (We need to take a copy of `seen_chains` as the function mutates it) - for links in self._get_chain_links(txn, set(seen_chains)): for chains in set_to_chain: for chain_id in links: if chain_id not in chains: @@ -589,6 +681,7 @@ class EventFederationWorkerStore(SignatureWorkerStore, EventsWorkerStore, SQLBas _materialize(chain_id, chains[chain_id], links, chains) + chains_to_fetch.difference_update(chains) seen_chains.update(chains) # Now for each chain we figure out the maximum sequence number reachable From 3fff4bde147734eeeb0ae70a0a3cf893976ec47f Mon Sep 17 00:00:00 2001 From: Christoph Settgast Date: Sat, 4 May 2024 18:04:47 +0200 Subject: [PATCH 2/3] changelog --- changelog.d/17154.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/17154.bugfix diff --git a/changelog.d/17154.bugfix b/changelog.d/17154.bugfix new file mode 100644 index 0000000000..e6cd5234a8 --- /dev/null +++ b/changelog.d/17154.bugfix @@ -0,0 +1 @@ +Fixed SQL performance regression introduced in #17044 From c01300710e555aa5dae78198f2be6fa5c9a6bfe2 Mon Sep 17 00:00:00 2001 From: Christoph Settgast Date: Sat, 4 May 2024 18:12:32 +0200 Subject: [PATCH 3/3] fixup changelog --- changelog.d/17154.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/17154.bugfix b/changelog.d/17154.bugfix index e6cd5234a8..0f3f7a9e05 100644 --- a/changelog.d/17154.bugfix +++ b/changelog.d/17154.bugfix @@ -1 +1 @@ -Fixed SQL performance regression introduced in #17044 +Fixed SQL performance regression introduced in #17044.