Prevent multiple device list updates from breaking a batch send

[anoadragon453]

Fixes #5153 and towards fixing #5095.

This PR:

• Ensures we take device list updates from the DB in order of ascending stream_id
• Raises the limit of updates from 20 to 100 (presumably this must be kept under 100 EDU transaction limit)
• Removes one more update from the DB and checks to see if its stream_id matches that of the update before it. If so, an update is running over the bounds of the batch, and we try to send everything but updates with that device ID.
• If the above results in us not sending any updates, then we simply ignore this update as it's probably not correct behavior anyways.

cc @richvdh

[anoadragon453]

Hm, this is currently failing as:

            SELECT user_id, device_id, max(stream_id) FROM device_lists_outbound_pokes
            WHERE destination = ? AND ? < stream_id AND stream_id <= ? AND sent = ?
            GROUP BY user_id, device_id
            ORDER BY stream_id
            LIMIT %d

is not valid SQL, failing with:

GroupingError: column "device_lists_outbound_pokes.stream_id" must appear in the GROUP BY clause or be used in an aggregate function
LINE 1: ...sent = false GROUP BY user_id, device_id ORDER BY stream_id ...

Searching tells me we need to use an ORDER BY ... OVER to combine both functions, but does this work on both Postgres and SQLite?

. o ( Perhaps this was the reason we didn't have an ORDER BY in the first place )

[codecov]

Codecov Report

Merging #5156 into develop will increase coverage by 0.08%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop    #5156      +/-   ##
===========================================
+ Coverage    62.99%   63.08%   +0.08%     
===========================================
  Files          341      341              
  Lines        35607    35624      +17     
  Branches      5827     5828       +1     
===========================================
+ Hits         22432    22474      +42     
+ Misses       11605    11584      -21     
+ Partials      1570     1566       -4

[richvdh]

I think you've ended up overcomplicating this; could you have another go?

Also: we try to minimise the work we do in a _txn function, since you're hanging onto a database connection which could otherwise be doing useful work. Suggest returning the raw list and doing the deduping magic in the parent?

(this will probably require you to split _get_devices_by_remote_txn in two, but afaict there is no need for both parts of it to be done on the same transaction, so that should be fine)

[richvdh]

this should probably be passed in as a limit param, so that it can be derived from https://github.com/matrix-org/synapse/blob/develop/synapse/federation/sender/per_destination_queue.py#L37.

[richvdh]

Suggested change

	duplicate_updates = [r for r in txn]
	duplicate_updates = list(txn)

[richvdh]

I'm not quite following what's going on here, but I think you're assuming that duplicates can only occur on adjacent rows, which is not the case.

I suggest you just build a dict which maps from (user_id, device_id) to stream_id, and then you can iterate over the results and check the dict for each row.

[richvdh]

getting there, but I think it could do with a bit of cleaning up to be clear and elegant. I've made a few suggestions, but please don't feel constrained by them: have a look at the code yourself and ask yourself if there are things that could be simplified.

[Incidentally, some of this stuff is getting a bit gnarly. Some UTs for get_devices_by_remote wouldn't go amiss.]

[richvdh]

this doesn't look right any more

[richvdh]

why - 1?

[richvdh]

RuntimeError is probably more appropriate here. It's not really a failure at the storage layer.

[richvdh]

It's not really getting the max_stream_id. Indeed it's now just returning the now_stream_id which gets passed in, so we could simplify that and just return results.

What it's really doing is building the device EDUs, including the e2e keys. So it could do with a better name too.

[richvdh]

At the moment there's a bit of a funny split where the fancy logic for clipping the list is in the parent, but we've got a +1 and the long comment in this function. I think it would be more intuitive to make _get_devices_by_remote_txn dumb, and move the +1 and the comment to the parent too.

[richvdh]

if not query_map

[richvdh]

this comment could do with an explanation as to what it means if we ended up in this situation.

[richvdh]

I'd be inclined to write this:

query_map[key] = max(query_map.get(key, 0), update[2])

[richvdh]

query_map is deduplicated, so it might have fewer than limit entries even if we hit the limit. As above, I think you can combine the clip of now_stream_id with this.

old

[richvdh]

the default for limit is redundant, and probably just confusing?

[richvdh]

Suggested change

	# Check if the last and second-to-last row's stream_id's are the same
	# Check if the last and second-to-last rows' stream_ids are the same

[richvdh]

I think this condition is redundant. We may as well set the stream_id_cutoff to the stream_id of the last row whenever we exceed the limit. Note that this also makes the len(updates) > 1 condition redundant.

[richvdh]

note that the token we return here is used as the from_token next time, which is exclusive, so this should be now_stream_id rather than now_stream_id + 1.

Also: there is no need to skip everything between the stream_id_cutoff and now_stream_id: we should return stream_id_cutoff - 1 instead.

[richvdh]

I think this could be phrased more clearly.

If we didn't find any updates with a stream_id lower than the cutoff, it means that there are more than limit
updates all of which have the same steam_id.

That should only happen if a client is spamming the server with new devices, in which case E2E isn't going to work well anyway. We'll just skip that stream_id and return an empty list, and continue with the next stream_id next time.

[richvdh]

do we not want stream_id_cutoff - 1 here too?

[richvdh]

actually, _get_device_update_edus_by_remote_txn doesn't seem to use this param. let's kill it!

[richvdh]

and here.

maybe we can get rid of stream_id_cutoff altogether, and just use now_stream_id ?

[richvdh]

a bit of docstring on the params and results (here and _get_device_update_edus_by_remote_txn) wouldn't go amiss.

[richvdh]

limit is unused as well.

[richvdh]

does this need to be a txn function? if you make it do runInteraction itself, we can free up the db connection for the second half of the function.

old