Make purge_history operate on tokens #3221
Conversation
As we're soon going to change how topological_ordering works
synapse/storage/events.py
Outdated
| @@ -1856,6 +1855,8 @@ def _purge_history_txn( | |||
| # furthermore, we might already have the table from a previous (failed) | |||
| # purge attempt, so let's drop the table first. | |||
|
|
|||
| token = RoomStreamToken.parse(token) | |||
There was a problem hiding this comment.
can you put this somewhere that isn't between the "let's build a temporary table" comment and the building of said temp table?
There was a problem hiding this comment.
could you also use different names for the stringy token and the tokeny token?
synapse/storage/events.py
Outdated
| @@ -1871,6 +1872,10 @@ def _purge_history_txn( | |||
| "CREATE INDEX events_to_purge_should_delete" | |||
| " ON events_to_purge(should_delete)", | |||
| ) | |||
| txn.execute( | |||
There was a problem hiding this comment.
might be worth commenting on why we need this index
| @@ -1927,9 +1932,8 @@ def _purge_history_txn( | |||
| txn.execute( | |||
There was a problem hiding this comment.
the comment on this doesn't really seem to be true any more.
There was a problem hiding this comment.
# We calculate the new entries for the backward extremeties by finding
# all events that point to events that are to be purged
we now look for events we are about to purge, which point to events we are not going to purge. it's completely different.
There was a problem hiding this comment.
OK, its different in terms that we're not returning the event that points to purged events but the purged events that we get pointed at, yes. (And this isn't anything to do with the change in the sql, and was wrong before)
| SELECT COALESCE(MIN(depth), 0) | ||
| FROM event_backward_extremities AS eb | ||
| INNER JOIN event_edges AS eg ON eg.prev_event_id = eb.event_id | ||
| INNER JOIN events AS e ON e.event_id = eg.event_id |
There was a problem hiding this comment.
is this looking for the prev_event before the extremity? which we have likely just purged?
There was a problem hiding this comment.
This is looking at the events that point to the extremity, as we don't actually have the extremity events.
There was a problem hiding this comment.
(And I think the check is the right way round?)
There was a problem hiding this comment.
oh right. ehm. I'm not convinced this is going to calculate the right thing in the presence of disconnected bits of dag.
There was a problem hiding this comment.
Each disconnected part of the DAG will have their backwards extremities in the event_backward_extremities table, so the event with the minimum depth must be one of them, no?
No description provided.