fix bug #2969 (loading all state for a given type from the DB if the state_key is None) #2990
Conversation
synapse/storage/state.py
Outdated
| else: | ||
| where_clause += "(type = ? AND state_key = ?)" | ||
| where_args.extend([typ[0], typ[1]]) | ||
| if typ != types[-1]: |
There was a problem hiding this comment.
is this supposed to be checking if it's the last element? it looks fragile.
Why not do " OR ".join(where_clauses), or something
There was a problem hiding this comment.
yes, it is. guess it is fragile if you have dups in the list. stupid brainfart on .join(where_clauses)
synapse/storage/state.py
Outdated
| ) if state_key is not None else | ||
| ( | ||
| "AND type = ?", | ||
| (etype) |
synapse/storage/state.py
Outdated
| @@ -240,6 +240,10 @@ def _get_state_groups_from_groups_txn(self, txn, groups, types=None): | |||
| ( | |||
| "AND type = ? AND state_key = ?", | |||
| (etype, state_key) | |||
| ) if state_key is not None else | |||
| ( | |||
There was a problem hiding this comment.
probably put this on the previous line after the else
|
@richvdh ptal |
| @@ -259,10 +262,17 @@ def _get_state_groups_from_groups_txn(self, txn, groups, types=None): | |||
| key = (typ, state_key) | |||
| results[group][key] = event_id | |||
| else: | |||
| where_args = [] | |||
| where_clauses = [] | |||
There was a problem hiding this comment.
it'd be nice to define this inside the if clause where it is used
There was a problem hiding this comment.
i may be going insane, but i'm failing to see why? it's used outside the if clause, so isn't it nicer to define it at the 'right' scope level, rather than give the impression that it's scoped to the if block (even though python scoping extends to the whole def?)
There was a problem hiding this comment.
I may be going insane, but I can't see it being used outside the if clause?
There was a problem hiding this comment.
surely where_args & where_clauses are used much later on to actually execute the query. hence defining them at the scope common to both. or are we talking about different if clauses?
There was a problem hiding this comment.
I'm talking about where_clauses, which is used at lines 270, 273 and 275. They are all inside the next if clause.
synapse/synapse/storage/state.py
Lines 267 to 275 in b2aba9e
| @@ -279,7 +289,7 @@ def _get_state_groups_from_groups_txn(self, txn, groups, types=None): | |||
| # after we finish deduping state, which requires this func) | |||
| args = [next_group] | |||
| if types: | |||
There was a problem hiding this comment.
this can be unconditional now:
args = [next_group] + where_args
There was a problem hiding this comment.
yes, although unclear what that buys us? (plus in my lazyloading PR we do have other if's at that point)
There was a problem hiding this comment.
simplicity, mostly. but I agree it's not a big deal.
|
I am totally failing to understand the optimisation at line 307 - the level of golf here is impenetrable :| |
when type filter includes wildcards on state_key
|
okay, it finally clicked; have made the comments more idiot-proof and hopefully disabled the optimisation correctly; good catch. @richvdh ptal |
No description provided.