Convert simple_select_list and simple_select_list_txn to return lists of tuples #16505
Conversation
|
(I probably need to double check the return types of these.) |
clokep
force-pushed
the
clokep/axe-cursor-to-dict-5
branch
from
965e465
to
a91e34d
Compare
| "thumbnail_type", | ||
| "thumbnail_length", | ||
| rows = cast( | ||
| List[Tuple[int, int, str, str, int]], |
There was a problem hiding this comment.
These seem to weirdly be all nullable?
| "thumbnail_type", | ||
| "thumbnail_length", | ||
| rows = cast( | ||
| List[Tuple[int, int, str, str, int]], |
There was a problem hiding this comment.
These seem to be all nullable.
| retcols=("room_id", "joined_via"), | ||
| desc="get_server_which_served_partial_join", | ||
| rows = cast( | ||
| List[Tuple[str, str]], |
There was a problem hiding this comment.
joined_via is nullable?
There was a problem hiding this comment.
I think I added that column after the fact.
ISTR that recent postgresses have made it so that adding a new column with a non-null default is much faster than it used to be. Maybe we don't need to add nullable columns when extending a table these days?
There was a problem hiding this comment.
This is as far as I reviewed up to
There was a problem hiding this comment.
ISTR that recent postgresses have made it so that adding a new column with a non-null default is much faster than it used to be. Maybe we don't need to add nullable columns when extending a table these days?
PG 11's changelog says
Many other useful performance improvements, including the ability to avoid a table rewrite for ALTER TABLE ADD COLUMN with a non-null column default
There was a problem hiding this comment.
I think this table is usually small so we can also probably just change it on the fly? I've filed an issue for now: #16547
| "is_verified", | ||
| "session_data", | ||
| rows = cast( | ||
| List[Tuple[str, str, int, int, int, str]], |
There was a problem hiding this comment.
first_message_index and forwarded_count and is_verified is nullable. (The last doesn't matter we cast to a bool below.)
|
@DMRobertson Any thoughts on this? Should I just file issues for the above? |
| retcols=("room_id", "joined_via"), | ||
| desc="get_server_which_served_partial_join", | ||
| rows = cast( | ||
| List[Tuple[str, str]], |
There was a problem hiding this comment.
I think I added that column after the fact.
ISTR that recent postgresses have made it so that adding a new column with a non-null default is much faster than it used to be. Maybe we don't need to add nullable columns when extending a table these days?
| retcols=("room_id", "joined_via"), | ||
| desc="get_server_which_served_partial_join", | ||
| rows = cast( | ||
| List[Tuple[str, str]], |
There was a problem hiding this comment.
This is as far as I reviewed up to
My preference would be to update the type hints that doesn't cause new type errors. If there are new type errors, then yeah let's file an issue. (Also happy for you to punt it and just file an issue :)) |
clokep
force-pushed
the
clokep/axe-cursor-to-dict-5
branch
from
9c880ad
to
5153b9a
Compare
| self.mock_txn.__iter__ = Mock(return_value=iter([(1,), (2,), (3,)])) | ||
| self.mock_txn.fetchall.return_value = [(1,), (2,), (3,)] |
There was a problem hiding this comment.
To check: I think that this is needed because we've changed the implementation to use fetchall (rather than a dictionary comprehension that loops over the psycopg cursor). Do I understand correctly?
More of #16431, this is again fairly large since these are used everywhere.