Fix federation stall on concurrent access errors

[ShadowJonathan]

This PR fixes #9635 assertively enough by ensuring that _store_destination_rooms_entries_txn (now just store_destination_rooms_entries) doesn't raise psycopg2.errors.SerializationFailure as much to get the process_event_queue_for_federation loop stuck.

It achieves this by replacing the txn.execute_batch call (which does many INSERTs) with one simple_upsert_many call (which effectively does "one" INSERT)

This decreases the load on the database enough to ensure SerializationFailure errors dont happen (as often) again.

What i thought this PR would do changed with some conversation, under here is the previous content of the PR, it is faulty, but here for historical reference.

Previous content

This PR fixes #9635 enough by ensuring that the _store_destination_rooms_entries_txn db subroutines gets retried a few times when it fails with psycopg2.errors.SerializationFailure.

This is fine, as this failure is simply a rollback of the transaction if another transaction had changed the view of the table concurrently, which means the "other" transaction has succeeded, and this one has failed, thus, this is infinitely retry-able until this transaction "succeeds" while the other "loses". In the very worst case, this blocks until the destinations table is less congested.

This PR will change the methods involved to use SerializationFailure-tolerant ones, such as simple_upsert_many, that will retry up to about 5 times until it succeeds. This means that every db interaction has sufficiently higher chance to succeed compared to previous behaviour. (probably only resulting in one or two "retries" before it succeeds in most scenarious)

And to relieve some of that congestion, the second fix this PR gives is making sure every handle_room_events background task only instructs it's (eventual) _send_pdu calls to only call self.store.store_destination_rooms_entries when it is handling the final event in the (per-room) event list.

Again, this is fine, as destinations_rooms is used to calculate which events need to be caught-up with, and in the worst case (when the federation sender shuts down or aborts in the middle of a handle_room_events task), it'll cause the catch-up to notify more events (likely 1 more event) some servers, but it wont drop any to send.

This'll also cause federation to deliver to large rooms such as #matrix:matrix.org more efficiently, as the store_destination_rooms_entries call is only done once on the last event that is being caught up with, meaning that if that call is expensive, and federation events start "backing up", it'll be able to keep pace by calling this only once per room in the batch of 100 events that're submitted to per-destination queues. This'll increase performance substantially on smaller servers.

Edit: Some of the principles this PR was made on (for destination_rooms) were likely to be false, this PR will only be about the fix, and the optimisation will be tried in another PR.

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)

Signed-off-by: Jonathan de Jong <jonathan@automatia.nl>

[ShadowJonathan]

The way it increases performance is due to _send_pdu previously calling store_destination_rooms_entries once per event, this can be costly for large rooms.

Now, it calls it only once per room per process_event_queue_for_federation batch (max 100), which means once pressure is applied onto large rooms by local users, process_event_queue_for_federation will start to process larger batches, which then call store_destination_rooms_entries less, which means that it is able to "catch up" more efficiently per batch, until an equilibrium is reached.

[erikjohnston]

Oh, this is very interesting. I'm surprised that its so contested and is introducing delays. My only concern with your approach is that it means not keeping that table up to date, and I'm not sure what the consequences of that are.

We recently introduced a change which handles the SerializationFailure by doing upserts outside of transactions, so I wonder if just splitting store_destination_rooms_entries_txn into two would magically help here, e.g. something like:

    async def store_destination_rooms_entries(
        self,
        destinations: Iterable[str],
        room_id: str,
        stream_ordering: int,
    ) -> None:
        """
        Updates or creates `destination_rooms` entries in batch for a single event.

        Args:
            destinations: list of destinations
            room_id: the room_id of the event
            stream_ordering: the stream_ordering of the event
        """

        await self.db_pool.simple_upsert_many(
            table="destinations",
            key_names=("destination",),
            key_values=[(d,) for d in destinations],
            value_names=[],
            value_values=[],
            desc="store_destination_rooms_entries_dests",
        )

        rows = [(destination, room_id) for destination in destinations]
        await self.db_pool.simple_upsert_many(
            table="destination_rooms",
            key_names=("destination", "room_id"),
            key_values=rows,
            value_names=["stream_ordering"],
            value_values=[(stream_ordering,)] * len(rows),
            desc="store_destination_rooms_entries_rooms",
        )

If that works, then yay we can avoid some of the complexity of this PR.

[ShadowJonathan]

That fixes #9635, but that doesn't fix the performance improvement I've noticed (which I still kinda wanna fix), of which I've hypothesised the following;

Again, this is fine, as destinations_rooms is used to calculate which events need to be caught-up with, and in the worst case (when the federation sender shuts down or aborts in the middle of a handle_room_events task), it'll cause the catch-up to notify more events (likely 1 more event) some servers, but it wont drop any to send.

Which is only in the case of which handle_room_events gets an exception thrown, which is then thrown into store_destination_rooms_entries, which is caught and effectively transaction sending is reset (or stays) at the current stream_id in the database.

(I've looked at where destination_rooms is referenced in the codebase, it is only used in _get_catch_up_room_event_ids_txn and _get_catch_up_outstanding_destinations_txn, which results ultimately go into the following (snippets):

_get_catch_up_room_event_ids_txn:

synapse/synapse/federation/sender/per_destination_queue.py

Lines 380 to 385 in 026503f

	# get at most 50 catchup room/PDUs
	while True:
	event_ids = await self._store.get_catch_up_room_event_ids(
	self._destination,
	self._last_successful_stream_ordering,
	)

_get_catch_up_outstanding_destinations_txn:

synapse/synapse/federation/sender/__init__.py

Lines 621 to 634 in 844c9b0

	async def _wake_destinations_needing_catchup(self):
	"""
	Wakes up destinations that need catch-up and are not currently being
	backed off from.
	In order to reduce load spikes, adds a delay between each destination.
	"""
	last_processed = None # type: Optional[str]
	while True:
	destinations_to_wake = (
	await self.store.get_catch_up_outstanding_destinations(last_processed)
	)

This missing an update (by crashing) isn't a huge deal, as it would only result in the federation sender sending a few more events the next time 'round)

[erikjohnston]

I'm sort of wondering whether that will speed things up. The DB hits should be fast, and we'll be doing quite a lot of other DB hits during processing, so I'm slightly surprised if the fixed version would actually cause a slow down. I can completely believe that when things were getting contested, and retried, etc that would cause it to be a source of slowness.

[ShadowJonathan]

I'm sort of wondering whether that will speed things up.

The federation senders now do an insert to destinations (for certainty that the foreign key exists), and an update to destination_rooms, this needs to be done concurrently, for every event, which is likely to result in many contentions for a slow postgres database with many federated events to multiple rooms concurrently.

The DB hits should be fast [...]

In my experience, with #matrix:matrix.org, they were not (for a small homeserver), as destinations and destination_rooms accesses/updates (on every event, for all 2k homeservers) are slow (and resulted in that concurrent access bug), i think this optimization (at least) will offer a lot to smaller and/or slower homeservers (as those will get the best performance bonus from this).

[erikjohnston]

The federation senders now do an insert to destinations (for certainty that the foreign key exists), and an update to destination_rooms, this needs to be done concurrently, for every event, which is likely to result in many contentions for a slow postgres database with many federated events to multiple rooms concurrently.

The insert into destinations shouldn't be bad in the contended case, as its just a DO NOTHING. My understanding is that destination_rooms shouldn't have concurrent writes to the same room, so also shouldn't be contended.

The thing is that while we do do this for every event, we also also do a lot of reads and writes to the DB for each transaction that takes place, which is often per event per destination, so even if we're doing a bunch of batching up my gut instinct would be they'd be dwarfed by all the other stuff.

Though given this is an issue occurring while your federation sender is busy catching up its entirely possible that all the performance characteristic are different than the "normal" case.

[ShadowJonathan]

My understanding is that destination_rooms shouldn't have concurrent writes to the same room, so also shouldn't be contended.

It is, because for a batch from process_event_queue_for_federation is separated into the rooms that each events belong to, and so handle_room_events gets called for each of these batches, concurrently.

When the homeservers in each room overlap, then it gets contested, and so a room like #matrix:matrix.org causes a holdup on the database, and any other small room that shares a room with #matrix:matrix.org (very likely) gets no serialised access, no concurrent update.

And even for large rooms like #matrix:matrix.org, a call to update destination_rooms is very expensive (for slow databases, 2k destination_rooms entries updated on every event), it would be useful to minimise this as much as possible regardless of the concurrency problem.

[ShadowJonathan]

...wait, I'm realising I might have it wrong about destination_rooms, I'll need to think about this for a moment.

Edit: I think i've misunderstood destination_rooms, and ive noted some other bugs that could happen (in regards to what happens if the last handle_event call wouldn't call _send_pdu for whatever reason), i'll probably need to think about how i'll rework this, i'll do that after this weekend.

[ShadowJonathan]

Yeah, i think I'll remove the optimization and split that into another PR, as it currently has some problems that needs to be hashed out individually, I'll make this PR just about the fix, then

[ShadowJonathan]

(I'm sorry for all the drama, this is still one of the first times I've looked at this code, gaining more understanding the longer i looked at it 😅)

[erikjohnston]

Yeah, i think I'll remove the optimization and split that into another PR, as it currently has some problems that needs to be hashed out individually, I'll make this PR just about the fix, then

No worries! Your initial stab was perfectly reasonable, and we do similar things elsewhere. It's just one of those things that's best to avoid if possible, and I happen to have been staring at a lot of this sort of stuff recently 😅

[clokep]

Can we get an improved title on this PR? I find it best to put the issue that it fixes in the description and not in the title and have the title be a short bit on what the change actually is.

[richvdh]

It might be helpful to update the description of the PR too. AFAICT it ended up being quite different to what was written originally.