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

Convert simple_select_many_batch, simple_select_many_txn to tuples. #16444

Merged
merged 10 commits into from Oct 11, 2023

Conversation

clokep
Copy link
Contributor

@clokep clokep commented Oct 6, 2023

🪓 cursor_to_dict, more of #16431.

This one got a bit big, but I couldn't really figure out a good way to separate them. It is fairly tedious, but I'll try to mark noticeable spots.

synapse/storage/databases/main/devices.py Show resolved Hide resolved
synapse/storage/databases/main/devices.py Show resolved Hide resolved
synapse/storage/databases/main/keys.py Outdated Show resolved Hide resolved
synapse/storage/databases/main/push_rule.py Show resolved Hide resolved
synapse/storage/databases/main/push_rule.py Show resolved Hide resolved
synapse/storage/databases/main/roommember.py Show resolved Hide resolved
synapse/storage/databases/main/state.py Show resolved Hide resolved
synapse/storage/databases/main/state.py Show resolved Hide resolved
synapse/storage/databases/state/store.py Show resolved Hide resolved
@clokep clokep marked this pull request as ready for review October 6, 2023 17:41
@clokep clokep requested a review from a team as a code owner October 6, 2023 17:41
Copy link
Contributor

@DMRobertson DMRobertson left a comment

Choose a reason for hiding this comment

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

I think this looks sane and would like to see it land. I mostly ended up double-checking the casts. I think in a few places we don't say Optional[] where the corresponding DB column is nullable---would you mind fixing those up?

synapse/storage/databases/main/events.py Outdated Show resolved Hide resolved
synapse/storage/databases/main/events.py Show resolved Hide resolved
synapse/storage/databases/main/keys.py Show resolved Hide resolved
synapse/storage/databases/main/keys.py Outdated Show resolved Hide resolved
synapse/storage/databases/main/push_rule.py Outdated Show resolved Hide resolved
synapse/storage/databases/main/push_rule.py Outdated Show resolved Hide resolved
synapse/storage/databases/main/transactions.py Outdated Show resolved Hide resolved
synapse/storage/databases/main/user_directory.py Outdated Show resolved Hide resolved
synapse/storage/databases/main/user_directory.py Outdated Show resolved Hide resolved
Comment on lines -226 to +231
# We sort the rows so that the most recently added entry is picked up.
rows.sort(key=lambda r: r["ts_added_ms"])
# We sort the rows by ts_added_ms so that the most recently added entry
# will stomp over older entries in the dictionary.
rows.sort(key=lambda r: r[2])
Copy link
Contributor

Choose a reason for hiding this comment

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

Ohhhhh I see: key ids are not unique in this table.

We could probably get the DB to do that for us (e.g. https://stackoverflow.com/questions/13325583/postgresql-max-and-group-by) , but I doubt it makes much difference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is annoying to do for both sqlite & postgres, I think. And I suspect Erik was attempting to not change too much code? Not 100% sure.

Copy link
Contributor

@DMRobertson DMRobertson left a comment

Choose a reason for hiding this comment

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

Thanks for working through this!

@clokep clokep merged commit a4904dc into develop Oct 11, 2023
32 of 38 checks passed
@clokep clokep deleted the clokep/axe-cursor-to-dict-3 branch October 11, 2023 17:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants