Fix assertions being thrown by the EventsStream update function

[richvdh]

The problem here was that the EventsStream update function could return more rows than was requested of it, which was then upsetting the db_query_to_update_function wrapper which didn't know how to handle this. The confusion over whether the limit was a limit or a target was introduced in #7024, but I think there are much longer-standing bugs here in the limit logic.

So, part of the fix here is to make it clear that the limit passed into the update_function is a target rather than a hard limit, and to avoid using db_query_to_update_function which is too simple for this usecase. But to get it right, we need to pull get_ex_outlier_stream_rows out to a separate query (to make it easier to see if we have hit the limit), and apply more intelligence about how we truncate the results from the three queries.

Tests and an answer to the FIXME to follow.

[richvdh]

This is intended to fix #7308.

[erikjohnston]

There is quite a lot going on here, but I think I now understand what's happening: we're specifically fixing a bug where EventStream.update_function was basically truncating the individual sources before merging rather than after merging, leading to potentially missing updates when the stream is limited.

It feels like this is only really fixing #7308 because we just so happen to now be avoiding the call path that includes the assertion, which is fine but...

This doesn't fix db_query_to_update_function to work with DB functions that may returns more than the given limit (which may be a case of converting the len == check to a len >= one, if callers are indeed happy with update functions returning more rows than expected). I think there are probably more DB functions that work like this than just the event stream? If not we should document that db_query_to_update_function expects the given function to treat the passed in limit as a hard limit.

Separately, as per comment we need to figure out how to better handle the limiting of get_all_updated_current_state_deltas.

---

The above is more me trying to get my head around the changes here. I think these changes make the situation strictly better and so I don't propose you fix the above bugs in this PR, but are things that need to be fixed.

[erikjohnston]

I think its worth adding a comment that its safe to do this because get_all_new_forward_event_rows will honour the limit. TBH its not clear to me that these assertions help any and could be replaced the limited checks with len(event_rows) >= target_row_count

[richvdh]

TBH its not clear to me that these assertions help any and could be replaced the limited checks with len(event_rows) >= target_row_count

I disagree. The only reason that it's safe to do upper_limit = event_rows[-1][0] is because we understand exactly how get_all_new_forward_event_rows behaves at the limit. If it starts returning more rows than the limit, then clearly we're not understanding its behaviour.

I think it's exactly that not-understanding that has allowed the bugs in this code to creep in and linger for so long, and I think exactly the same applies to the assertion in db_query_to_update_function: I wouldn't want to remove the assertion there.

I'll add a comment to explain this a bit.

[erikjohnston]

Why has this been removed?

[richvdh]

it's redundant: there is another check for the same thing up in get_updates_since, and if in the future we do manage to call get_all_new_forward_event_rows with last_id == current_id, it still behaves correctly.

I'm not a fan of shortcuts like this - they make me suspicious that the algorithm they are short-cutting is incorrect somehow. Obviously if there's a significant efficiency gain from the shortcut, there's an argument for it, but since in practice the shortcut is there in get_updates_since, I don't think it has any merit here.

[erikjohnston]

Three thoughts here:

1. I'd rather functions did easy wins in terms of performance to avoid foot guns when changes are made 10 layers up the stack, especially when pretty much all other functions that fetch streams from the DB do this and other optimisations.
2. Avoiding a DB query is a big win for functions that get called in a loop.
3. Personally I don't really feel that the presence of a short circuit makes me any more or less inclined to trust the algorithm is right.

[erikjohnston]

I don't think this is safe, either:

1. get_all_updated_current_state_deltas takes a hard limit and may not return all rows with the last stream_id, or
2. get_all_updated_current_state_deltas always returns all rows for the last stream_id, and so may return more than limit items.

Currently, it looks like we're in the first case, and so we need to discard all rows with the stream_id of state_rows[-1][0].

[richvdh]

Right. I think this is the same thing as I was worrying about for the FIXME, below.

[erikjohnston]

No it isn't a given.

[erikjohnston]

i.e. they can have multiple rows per stream ID afaict

[richvdh]

Yeah, I suspected as much. I was rather hoping that this wouldn't be any worse than it has been for years, and that it would therefore be safe to deploy while I thought about it a bit harder.

[erikjohnston]

It took me a while to realise the crux here is that: "for each source we must return all rows up to the same token."

[richvdh]

Fair. Will add another comment.

[richvdh]

This doesn't fix db_query_to_update_function to work with DB functions that may returns more than the given limit

As above: I'm not convinced that just lifting the restriction is the right solution - I think the fact that we were lax on these checks is what allowed us to mess up here previously.

And yes, I haven't gone through the other db_query functions to check whether they honour the limit or not. I assumed that the others were simpler and therefore more likely to honour the limit, but I agree it's worth checking. Again though, I'd like to defer it for now.

[erikjohnston]

Given this is breaking stuff and the functional changes are fine, lets merge it. I do think we need to figure out where we're going with these changes as right now I think its turning into a bit of a mess with assertions everywhere, and that feels wrong and we should either be consistent with our API contracts (and maybe assert those) or move the logic based on asserted assumptions down into the DB functions.

[erikjohnston]

(I should also say that this is a net improvement, thank you 🙂 )