Remove old rows from the cache_invalidation_stream_by_instance table automatically. (This table is not used when Synapse is configured to use SQLite.)

[reivilibre]

I propose a mechanism to clean up old (> 7d old, to be very generous) rows from the cache_invalidation_stream_by_instance table.

On LPnet, this table is 700 MB (4e6 rows) and has never been cleaned before. On Morg, this table is 191 GB (8e8 rows) but seems to have been manually cleaned in 2020.

These rows are only needed for mere instants whilst other workers catch up with the stream, so there is no utility in keeping them around for longer than a few instants.
However, to allow for clock drift, debugging and just generally out of caution, I set the cut-off to 7 days.

Fixes: #3665

Base: develop

Original commit schedule, with full messages:

1. Add a cache invalidation clean-up task

2. Run the cache invalidation stream clean-up on the background worker

3. Tune down

4. call_later is in millis!

[MadLittleMods]

Minor things

[MadLittleMods]

It would be nice if we could easily determine the outcome of the deletion.

There is this to see how many rows were deleted if we're interested in that boilerplate.

(not saying we have to use it as the current way is simple, just cumbersome).

[reivilibre]

I considered checking the count of deleted rows but these streams can technically have gaps so you don't actually know how many rows are to be deleted, only the upper bound.

DELETE ... RETURNING ... was an option too but it felt less obvious to me and I wanted it to be 'obviously correct', whereas I wouldn't have as much confidence writing something that way

[DMRobertson]

SGTM. Have you had a chance to test this on your homeserver?

Only two other thoughts spring to mind is:

• is it worth adding logging for this, so you can see the cleanup is making forward progress? I guess not; you can always SELECT min(stream_id) FROM ... the table.
• Is this a generic mechanism/are there other streams whose data is safe to clean out periodically? I suggest you ignore this question so we can land this improvement as-is.

[DMRobertson]

Would this be slightly clearer if we just had one function that was decorated with @wrap_as_background_process? (Is that even equivalent? 🤷)

[reivilibre]

I only learnt about this wrapper very recently, I will give it a go :)

[DMRobertson]

I think this makes sense, but I probably would have queried for something like

                SELECT stream_id FROM cache_invalidation_stream_by_instance
                WHERE stream_id > ? OR invalidation_ts > ?
                ORDER BY stream_id ASC
                LIMIT 1 

i.e. to see if the complement of rows covered by the previous query is nonempty.

[reivilibre]

I think I genuinely do only want to look at the first row after our current marker: adding more conditions like you suggest is either going to cause more work for the DB as it'd have to scan all the remaining rows until finding one (I think?) or in the case of the OR condition, it's going to not be able to use any index for that part of the condition.

Have to be honest I don't quite follow your suggestion's query, at first I was thinking you wanted WHERE stream_id > ? AND invalidation_ts <= ? but altogether not sure. Ignoring that I don't fully grok what you're saying, I think OR invalidation_ts > ? is potentially problematic as we don't have an index on invalidation_ts — I can't see how the query planner would make a generally efficient plan for the query :).

[DMRobertson]

At first I thought you were deleting things based on both stream_id and invalidation_ts. If so, it seemed odd to not take both into account when working out if had deleted everything that you wanted to.

I see now that you're only deleting things based on stream_id, using the invalidation_ts only to select a sensible upper limit on stream_id.

Paranoid note: it is not necessarily the case that x.stream_id > y.stream_id implies x.invalidation_ts > y.invalidation_ts (consider e.g. one worker is slow to complete transactions or has a laggy clock). I wouldn't expect this to have any meaingful effect in practice because any drifts should be small.

[reivilibre]

Have you had a chance to test this on your homeserver?

yup!

[DMRobertson]

Happy iff CI is.