Fix MultiWriteIdGenerator's handling of restarts.

[erikjohnston]

On startup MultiWriteIdGenerator fetches the maximum stream ID for each instance from the table and uses that as its initial "current position" for each writer. This is problematic as a) it involves either a scan of events table or adding a new index (neither of which is ideal), and b) if rows are being persisted out of order elsewhere while the process restarts then using the maximum stream ID is not correct. This could theoretically lead to race conditions where e.g. events that are persisted out of order are not sent down sync streams.

We fix this by creating a new table that tracks the current positions of each writer to the stream, and update it each time we finish persisting a new entry. This is a relatively small overhead when persisting events. However for the cache invalidation stream this is a much bigger relative overhead, so instead we note that for invalidation we don't actually care about reliability over restarts (as there's no caches to invalidate) and simply don't bother reading and writing to the new table in that particular case.

---

This has the side effect of fixing start up times on develop.

Probably best to review commits separately. If it helps I can move the first two commits to a dedicated PR.

[clokep]

If it helps I can move the first two commits to a dedicated PR.

I suspect this would be helpful to shrink the size a bit, but isn't required.

[erikjohnston]

If it helps I can move the first two commits to a dedicated PR.

I suspect this would be helpful to shrink the size a bit, but isn't required.

Done: #8383

[clokep]

I think it looks pretty reasonable, but I'm not sure I understand the impact of different failures modes.

[clokep]

Are there any issues with this being a different transaction? (What if this transaction fails?) I suspect it is OK since we always update to the largest position anyway.

[erikjohnston]

We want to do it after we've marked the associated ID as "persisted", which happens after the transaction has already finished.

Though a follow up question of: can we mark the ID as persisted before we finish the transaction? The answer to that is probably yes if we faff around a bit.

One thing to note is that the only stream that actually uses get_next_txn is the cache invalidation stream, which we explicitly dont' record the stream positions for. The only reason I've added updating the stream positions table here is for consistency and to avoid future foot guns.

[clokep]

It looks like _current_positions is initiated to an empty dict. We probably don't need this.

[erikjohnston]

I think it looks pretty reasonable, but I'm not sure I understand the impact of different failures modes.

Yeah, its a bit of a tricky thing to reason about? Would a call to walk through it help? Or do you want a second opinion on this?

[clokep]

By excluding only the known writers here could there be a dataloss situation when removing a writer, if that writer is the minimum position?

If you have writers:

• A at pos 15
• B at pos 17
• C at pos 12

And then remove "C", _current_positions will be {A: 15, B: 17} and min_stream_id will be 15. I'm not sure if this is OK or not?

It seems you also don't want to include writers that you no longer care about or else you can end up in a situation where you have an old writer always saying it is far behind, which I'm guessing this if-statement checks against?

[erikjohnston]

As per discussion elsewhere: this is OK because if C has been removed from the list it means its been turned off, and so we know its up to date.

It's worth noting that these positions aren't really "where C has gotten up to in the rooms", but instead is more "where C is currently persisting events". I.e. we're not worried that if we remove C from the deployment then A or B will start persisting at position 12, as either C finished persisting a row at 12 or the request gets retried and A or B will persist it with a new position.

[clokep]

Isn't this true of any instance in _current_positions? I'm having trouble following why this is only true of the current instance.

[erikjohnston]

We know that if the current instance has just been restarted then we don't have any rows that are currently being persisted, so its safe to set the current position of itself to the max stream ID. Other instances may not have been restarted so may still be persisting things.

(We don't just set the current position of the instance to the max stream ID as in future we want every entry in current_positions to have matching instance in the DB, i.e. if we have {A: 5, B: 6} then we want the row 5 in the DB to have an instance of A and row 6 to have an instance of B. This will allow us to serialise the current position to (5, 6), as we can then just look the rows up in the DB to get back to {A: 5, B: 6})

[clokep]

Doesn't really matter, but this is checked in the callers too. I'm assuming this was just double checking?

[erikjohnston]

Yes, just in case somewhere forgets to do the check.

[clokep]

I think it looks pretty reasonable, but I'm not sure I understand the impact of different failures modes.

Yeah, its a bit of a tricky thing to reason about? Would a call to walk through it help? Or do you want a second opinion on this?

I took another look (and went back to more of the code this calls into) and I think I have a decent handle on it. I asked a couple more questions -- if they're easier to answer over a call that sounds fine with me!