Message Retention Purging Strategy

[mredig]

This is a focused sub topic of #6713.

For message retention, we need to purge expired message events somehow. My original intention was to migrate the dbs to index the message's timestamp and run a DELETE WHERE timestamp < expiration style query, but when discussed with @stefanceriu he raised some concerns. I can't recall all of them, but I believe it revolved around there being multiple different stores used for different purposes.

I'm now proposing that I'd approach this instead by iterating over events from the oldest to the newest, stopping once we encounter a message that is not expired. The added benefit here is that we can inspect each message as we encounter it to also clear up local media caches.

I'd intend to implement by modifying the EventCacheStore to gain load_first_chunk and load_next_chunk methods, such that we can start from the oldest events and work towards the newest. The biggest catch, however, is that the IndexedDB for web is missing the previous_id column in its table to be able to find the first chunk with a simple WHERE previous_id is NULL, and instead need to scan the whole table to find it. So this plan would also entail a migration to add the previous_id column (and a table wipe to reset).

So, I'm soliciting feedback on this plan, but would be open to other suggestions as well.

[poljar]

I'd intend to implement by modifying the EventCacheStore to gain load_first_chunk and load_next_chunk methods, such that we can start from the oldest events and work towards the newest. The biggest catch, however, is that the IndexedDB for web is missing the previous_id column in its table to be able to find the first chunk with a simple WHERE previous_id is NULL, and instead need to scan the whole table to find it. So this plan would also entail a migration to add the previous_id column (and a table wipe to reset).

This sounds sensible to me.

Another thing, which you might have discussed with @stefanceriu as well, is that using the SDK's redaction mechanism might be the best way forward when it comes to the deletion of events.

This will ensure that everything gets updated and notified that an event is no more. Additionally it won't mess with any sort of assumptions the SDK might in regards to events not disappearing from the DB.

If needs be, additional metadata could be attached to the redacted items so we can distinguish between real redactions and our retention-related redactions.

[poljar]

I'd intend to implement by modifying the EventCacheStore to gain load_first_chunk and load_next_chunk methods, such that we can start from the oldest events and work towards the newest.

Oh, actually, you might want to consider some different mechanism. These methods will put the chunks into memory and keep them there.

Your use-case doesn't require this, you only want to find event IDs for events you want to put through the redaction mechanism.

This would avoid the need to fill up the event cache, and you'll have a much smaller memory footprint.

[mredig]

using the SDK's redaction mechanism might be the best way forward when it comes to the deletion of events.

Does this allow complete removal of the events? The purpose of retention is that it completely deletes the event, not just redacts it.

If it doesn't, are you perhaps suggesting that we modify to allow either behavior, so it follows a common path and follows through on any follow up actions consistently?

Oh, actually, you might want to consider some different mechanism. These methods will put the chunks into memory and keep them there.

is this suggesting my original idea of

to migrate the dbs to index the message's timestamp and run a DELETE WHERE timestamp < expiration style query

if so, I agree that it would be far more memory efficient, but apparently would need chunk cleanup afterwards? Not to mention that linked local cached media wouldn't get cleared.

[mredig]

These methods will put the chunks into memory and keep them there.

Even if the entire chunk is deleted from retention?

Alternatively, do we need some chunk unload function?

[poljar]

Does this allow complete removal of the events? The purpose of retention is that it completely deletes the event, not just redacts it.

No, the metadata would have been left around. If that's undesirable then the solution will have to be more complex.

If it doesn't, are you perhaps suggesting that we modify to allow either behavior, so it follows a common path and follows through on any follow up actions consistently?

While this might not have been my initial suggestion, indeed this would be a good approach. Mimick the redaction code path somewhat to not forget other places where events might be stored, i.e. the message search feature listens to the event cache to remove events from the index once they get redacted.

to migrate the dbs to index the message's timestamp and run a DELETE WHERE timestamp < expiration style query

No, just deleting things won't work, as you have might noticed or was previously discussed.

Even if the entire chunk is deleted from retention?

Hmm, I might have been wrong here. Hard to say how it would behave if you load the chunks in reverse order. @Hywan, would that mess with the operation of the event cache? If older chunks are loaded but the newest one isn't?

If it does mess with the operation, then I would say you'd want to:

1. Get the event IDs you'd want to remove.
2. Find them if they are in the memory, remove with remove_events_by_position()
3. Only now you delete them from the DB as well.

Alternatively, do we need some chunk unload function?

I guess no, as empty chunks get removed automatically, so you would end up at most with a single chunk which you only partially deleted.

[mredig]

It sounds to me like it would be rational to start with my plan of

modifying the EventCacheStore to gain load_first_chunk and load_next_chunk methods, such that we can start from the oldest events and work towards the newest. The biggest catch, however, is that the IndexedDB for web is missing the previous_id column in its table to be able to find the first chunk with a simple WHERE previous_id is NULL, and instead need to scan the whole table to find it. So this plan would also entail a migration to add the previous_id column (and a table wipe to reset).

and go from there?

Obvs we can adjust if necessary, but that this would most likely be the right path and would allow me to get started?

[poljar]

Obvs we can adjust if necessary, but that this would most likely be the right path and would allow me to get started?

Yeah, I think that's going to work, not sure if we'll want to merge the loading functions later on, but that can be a separate discussion and refactor on top of the new functions.

[Hywan]

@poljar wrote:

I guess no, as empty chunks get removed automatically, so you would end up at most with a single chunk which you only partially deleted.

Hmm, that's true that LinkedChunk::remove_item_at will remove the chunk if it becomes empty after the deletion of the item. However, you need the LinkedChunk to be loaded in memory to get the updates (that will be broadcasted to the database, via Update::RemoveChunk).

remove_events_by_position() only exists in EventLinkedChunk (which is a thin wrapper around LinkedChunk tailored for the Event Cache). It means it implies the chunk to be loaded in memory too.

However, one can use RoomEventCacheState::remove_events to do a proper removal. But it only emits Update::RemoveItem, as it is unable to know when a chunk is empty without more complex calculation. We also don't expect to database to clean the chunks.

Note

remove_events already sort positions by descending order to preserve the positions when doing multiple removals.

We can ignore empty chunks in the database for now. Consequently, taking the plan proposed by @poljar, I would change the following steps:

2. (updated) Find them if they are in the memory, remove with RoomEventCacheState::remove_events (same for all caches…!)

and

4. (new) Later, add a “gc-phase” to clean up/compact/rebuilt the LinkedChunks. This was on our todo list because removing all the gaps everywhere all the time can increase the chunk identifier greatly for no particular reason. We can also have consecutive gaps in some cases. This clean up/compact/rebuilt step can also remove the empty chunks if any.

Thoughts?

[mredig]

so it sounds like this is the plan?

1. Get expired event IDs (via chunk iteration, oldest to newest, stop at first non-expired)
2. deserialize events to extract any locally cached linked media and delete
3. Remove via RoomEventCacheState::remove_events (which should also delete from the DB?)
4. Delete from DB (covered by 3?)
5. (Future) GC pass to compact LinkedChunks and clean up any empty chunks left behind

And the first step for this would be to load the oldest chunks so we can iterate with a load_first_chunk and load_next_chunk, right?

[poljar]

Number 4. Is indeed covered by 3.

The rest looks fine, step 2 might need discussion, i.e. media can be linked by multiple events.

And the first step for this would be to load the oldest chunks so we can iterate with a load_first_chunk and load_next_chunk, right?

Yes.

[Hywan]

For the GC, we need a whole discussion because it's very not trivial.

1. Get expired event IDs (via chunk iteration, oldest to newest, stop at first non-expired)

You can scan the events table directly, without having to interact with a LinkedChunk at all.

[mredig]

@Hywan

You can scan the events table directly, without having to interact with a LinkedChunk at all.

oooh so it would probably be a better strategy to change step 1 to "Query events table for ids older than the retention cutoff, getting the event ids (and chunk positions)." (but this would also necessitate a db migration for both sql and indexeddb to include origin_server_ts). From there, the remaining steps would stay as is? And it seems like this strategy would supercede #7022, right?

[Hywan]

It's better to store the timestamp, which is derived from the origin_server_ts; see TimelineEvent::timestamp.

I'm also not sure if using https://sqlite.org/json1.html could be a better idea in that specific case 🤔.

[poljar]

oooh so it would probably be a better strategy to change step 1 to "Query events table for ids older than the retention cutoff, getting the event ids (and chunk positions)."

Yes, as mentioned in #6965 (comment), you'd only need event IDs, positions.

Fishing them out of the linked chunks would probably be fine as well, as long as we don't attempt to cache them longer term.

But more specialized methods to directly get only what we need out of the store sound like a sensible strategy that would let us operate on bigger chunks of data.

[mredig]

k so updated steps:

1. Query the events table for (event_id, position) pairs where timestamp < cutoff -- requires db migration to add timestamp to both SQLite and IndexedDB (wrapped in a convenience like find_events_before_timestamp(room_id, cutoff) -> Vec<(OwnedEventId, Position)>)
2. For each, call state.find_event(event_id) to get EventLocation and event content
   2.5 delete any linked media
3. Sort into in_memory_events / in_store_events by EventLocation
4. Call remove_events with both lists
5. (Future) GC pass to compact LinkedChunks and clean up empty chunks