Skip to content
This repository was archived by the owner on Apr 26, 2024. It is now read-only.

fix bug #2969 (loading all state for a given type from the DB if the state_key is None) #2990

Merged
ara4n merged 5 commits into from
Mar 13, 2018
Merged

fix bug #2969 (loading all state for a given type from the DB if the state_key is None) #2990

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1b1c137
fix bug #2926
ara4n Mar 13, 2018
52f7e23
PR feedbackz
ara4n Mar 13, 2018
b2aba9e
build where_clause sanely
ara4n Mar 13, 2018
865377a
disable optimisation for searching for state groups
ara4n Mar 13, 2018
afbf4d3
typoe
ara4n Mar 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 27 additions & 7 deletions synapse/storage/state.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,9 @@ 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 (
"AND type = ?",
(etype,)
)
for etype, state_key in types
]
Expand All @@ -259,10 +262,19 @@ 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 = []
Copy link
Copy Markdown
Member

@richvdh richvdh Mar 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it'd be nice to define this inside the if clause where it is used

Copy link
Copy Markdown
Member Author

@ara4n ara4n Mar 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

Copy link
Copy Markdown
Member

@richvdh richvdh Mar 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may be going insane, but I can't see it being used outside the if clause?

Copy link
Copy Markdown
Member Author

@ara4n ara4n Mar 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member

@richvdh richvdh Mar 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if types is not None:
for typ in types:
if typ[1] is None:
where_clauses.append("(type = ?)")
where_args.extend(typ[0])
else:
where_clauses.append("(type = ? AND state_key = ?)")
where_args.extend([typ[0], typ[1]])
where_clause = "AND (%s)" % (" OR ".join(where_clauses))

wildcard_types = False
if types is not None:
where_clause = "AND (%s)" % (
" OR ".join(["(type = ? AND state_key = ?)"] * len(types)),
)
for typ in types:
if typ[1] is None:
where_clauses.append("(type = ?)")
where_args.extend(typ[0])
wildcard_types = True
else:
where_clauses.append("(type = ? AND state_key = ?)")
where_args.extend([typ[0], typ[1]])
where_clause = "AND (%s)" % (" OR ".join(where_clauses))
else:
where_clause = ""

Expand All @@ -279,7 +291,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:
Copy link
Copy Markdown
Member

@richvdh richvdh Mar 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be unconditional now:

args = [next_group] + where_args

Copy link
Copy Markdown
Member Author

@ara4n ara4n Mar 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, although unclear what that buys us? (plus in my lazyloading PR we do have other if's at that point)

Copy link
Copy Markdown
Member

@richvdh richvdh Mar 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

simplicity, mostly. but I agree it's not a big deal.

args.extend(i for typ in types for i in typ)
args.extend(where_args)

txn.execute(
"SELECT type, state_key, event_id FROM state_groups_state"
Expand All @@ -292,9 +304,17 @@ def _get_state_groups_from_groups_txn(self, txn, groups, types=None):
if (typ, state_key) not in results[group]
)

# If the lengths match then we must have all the types,
# so no need to go walk further down the tree.
if types is not None and len(results[group]) == len(types):
# If the number of entries in the (type,state_key)->event_id dict
# matches the number of (type,state_keys) types we were searching
# for, then we must have found them all, so no need to go walk
# further down the tree... UNLESS our types filter contained
# wildcards (i.e. Nones) in which case we have to do an exhaustive
# search
if (
types is not None and
not wildcard_types and
len(results[group]) == len(types)
):
break

next_group = self._simple_select_one_onecol_txn(
Expand Down