From 5ce9b79257d078717c1864e86e9aff833122369c Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 31 Oct 2023 11:03:44 -0400 Subject: [PATCH 1/6] Avoid executing no-op queries. --- synapse/storage/database.py | 12 ++++++++++-- synapse/storage/databases/main/devices.py | 2 +- synapse/storage/databases/main/events.py | 12 ++++++------ synapse/storage/databases/main/room.py | 2 +- synapse/storage/databases/main/search.py | 4 ++-- 5 files changed, 20 insertions(+), 12 deletions(-) diff --git a/synapse/storage/database.py b/synapse/storage/database.py index a4e7048368a3..18d9853c0423 100644 --- a/synapse/storage/database.py +++ b/synapse/storage/database.py @@ -1132,7 +1132,7 @@ def simple_insert_many_txn( txn: LoggingTransaction, table: str, keys: Collection[str], - values: Iterable[Iterable[Any]], + values: Collection[Iterable[Any]], ) -> None: """Executes an INSERT query on the named table. @@ -1145,6 +1145,9 @@ def simple_insert_many_txn( keys: list of column names values: for each row, a list of values in the same order as `keys` """ + # IF there's nothing to insert, don't send the query. + if not values: + return if isinstance(txn.database_engine, PostgresEngine): # We use `execute_values` as it can be a lot faster than `execute_batch`, @@ -1470,7 +1473,7 @@ def simple_upsert_many_txn( key_names: Collection[str], key_values: Collection[Iterable[Any]], value_names: Collection[str], - value_values: Iterable[Iterable[Any]], + value_values: Collection[Iterable[Any]], ) -> None: """ Upsert, many times. @@ -1483,6 +1486,9 @@ def simple_upsert_many_txn( value_values: A list of each row's value column values. Ignored if value_names is empty. """ + if not value_values: + return + if table not in self._unsafe_to_upsert_tables: return self.simple_upsert_many_txn_native_upsert( txn, table, key_names, key_values, value_names, value_values @@ -2059,6 +2065,8 @@ def simple_update_many_txn( raise ValueError( f"{len(key_values)} key rows and {len(value_values)} value rows: should be the same number." ) + if not value_values: + return # List of tuples of (value values, then key values) # (This matches the order needed for the query) diff --git a/synapse/storage/databases/main/devices.py b/synapse/storage/databases/main/devices.py index 49edbb9e060e..f796323b8fae 100644 --- a/synapse/storage/databases/main/devices.py +++ b/synapse/storage/databases/main/devices.py @@ -703,7 +703,7 @@ def _mark_as_sent_devices_by_remote_txn( key_names=("destination", "user_id"), key_values=[(destination, user_id) for user_id, _ in rows], value_names=("stream_id",), - value_values=((stream_id,) for _, stream_id in rows), + value_values=[(stream_id,) for _, stream_id in rows], ) # Delete all sent outbound pokes diff --git a/synapse/storage/databases/main/events.py b/synapse/storage/databases/main/events.py index 3c1492e3ad28..a578a671a4dd 100644 --- a/synapse/storage/databases/main/events.py +++ b/synapse/storage/databases/main/events.py @@ -1454,7 +1454,7 @@ def event_dict(event: EventBase) -> JsonDict: txn, table="event_json", keys=("event_id", "room_id", "internal_metadata", "json", "format_version"), - values=( + values=[ ( event.event_id, event.room_id, @@ -1463,7 +1463,7 @@ def event_dict(event: EventBase) -> JsonDict: event.format_version, ) for event, _ in events_and_contexts - ), + ], ) self.db_pool.simple_insert_many_txn( @@ -1486,7 +1486,7 @@ def event_dict(event: EventBase) -> JsonDict: "state_key", "rejection_reason", ), - values=( + values=[ ( self._instance_name, event.internal_metadata.stream_ordering, @@ -1505,7 +1505,7 @@ def event_dict(event: EventBase) -> JsonDict: context.rejected, ) for event, context in events_and_contexts - ), + ], ) # If we're persisting an unredacted event we go and ensure @@ -1528,11 +1528,11 @@ def event_dict(event: EventBase) -> JsonDict: txn, table="state_events", keys=("event_id", "room_id", "type", "state_key"), - values=( + values=[ (event.event_id, event.room_id, event.type, event.state_key) for event, _ in events_and_contexts if event.is_state() - ), + ], ) def _store_rejected_events_txn( diff --git a/synapse/storage/databases/main/room.py b/synapse/storage/databases/main/room.py index 3e8fcf197511..84af7f0de821 100644 --- a/synapse/storage/databases/main/room.py +++ b/synapse/storage/databases/main/room.py @@ -2216,7 +2216,7 @@ def _store_partial_state_room_txn( txn, table="partial_state_rooms_servers", keys=("room_id", "server_name"), - values=((room_id, s) for s in servers), + values=[(room_id, s) for s in servers], ) self._invalidate_cache_and_stream(txn, self.is_partial_state_room, (room_id,)) self._invalidate_cache_and_stream( diff --git a/synapse/storage/databases/main/search.py b/synapse/storage/databases/main/search.py index dbde9130c618..f4bef4c99b35 100644 --- a/synapse/storage/databases/main/search.py +++ b/synapse/storage/databases/main/search.py @@ -106,7 +106,7 @@ def store_search_entries_txn( txn, table="event_search", keys=("event_id", "room_id", "key", "value"), - values=( + values=[ ( entry.event_id, entry.room_id, @@ -114,7 +114,7 @@ def store_search_entries_txn( _clean_value_for_search(entry.value), ) for entry in entries - ), + ], ) else: From e6010d10bcef773c441720f9cabf380ee598f995 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 31 Oct 2023 11:09:35 -0400 Subject: [PATCH 2/6] Newsfragment --- changelog.d/16583.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/16583.misc diff --git a/changelog.d/16583.misc b/changelog.d/16583.misc new file mode 100644 index 000000000000..df5b27b11257 --- /dev/null +++ b/changelog.d/16583.misc @@ -0,0 +1 @@ +Avoid executing no-op queries. From da04633d590772a7492e09e41fed95807c681414 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 31 Oct 2023 13:08:12 -0400 Subject: [PATCH 3/6] Fix --- synapse/storage/database.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/storage/database.py b/synapse/storage/database.py index 18d9853c0423..fb0be52ffc77 100644 --- a/synapse/storage/database.py +++ b/synapse/storage/database.py @@ -1486,7 +1486,7 @@ def simple_upsert_many_txn( value_values: A list of each row's value column values. Ignored if value_names is empty. """ - if not value_values: + if not key_values: return if table not in self._unsafe_to_upsert_tables: @@ -2065,7 +2065,7 @@ def simple_update_many_txn( raise ValueError( f"{len(key_values)} key rows and {len(value_values)} value rows: should be the same number." ) - if not value_values: + if not key_values: return # List of tuples of (value values, then key values) From 44228ace9c0f767fee8b49d3705c9d51b3a0ea3e Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 7 Nov 2023 09:57:21 -0500 Subject: [PATCH 4/6] Update tests. --- tests/storage/test_base.py | 25 +++++-------------------- 1 file changed, 5 insertions(+), 20 deletions(-) diff --git a/tests/storage/test_base.py b/tests/storage/test_base.py index b4c490b568f2..de4fcfe02613 100644 --- a/tests/storage/test_base.py +++ b/tests/storage/test_base.py @@ -189,17 +189,9 @@ def test_insert_many_no_iterable( ) if USE_POSTGRES_FOR_TESTS: - self.mock_execute_values.assert_called_once_with( - self.mock_txn, - "INSERT INTO tablename (col1, col2) VALUES ?", - [], - template=None, - fetch=False, - ) + self.mock_execute_values.assert_not_called() else: - self.mock_txn.executemany.assert_called_once_with( - "INSERT INTO tablename (col1, col2) VALUES(?, ?)", [] - ) + self.mock_txn.executemany.assert_not_called() @defer.inlineCallbacks def test_select_one_1col(self) -> Generator["defer.Deferred[object]", object, None]: @@ -393,7 +385,7 @@ def test_update_many(self) -> Generator["defer.Deferred[object]", object, None]: ) @defer.inlineCallbacks - def test_update_many_no_values( + def test_update_many_no_iterable( self, ) -> Generator["defer.Deferred[object]", object, None]: yield defer.ensureDeferred( @@ -408,16 +400,9 @@ def test_update_many_no_values( ) if USE_POSTGRES_FOR_TESTS: - self.mock_execute_batch.assert_called_once_with( - self.mock_txn, - "UPDATE tablename SET col3 = ? WHERE col1 = ? AND col2 = ?", - [], - ) + self.mock_execute_batch.assert_not_called() else: - self.mock_txn.executemany.assert_called_once_with( - "UPDATE tablename SET col3 = ? WHERE col1 = ? AND col2 = ?", - [], - ) + self.mock_txn.executemany.assert_not_called() @defer.inlineCallbacks def test_delete_one(self) -> Generator["defer.Deferred[object]", object, None]: From 6aff5acc4701aad2b3b7c42efb2366c3e388378a Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 7 Nov 2023 10:03:21 -0500 Subject: [PATCH 5/6] Add comments. --- synapse/storage/database.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/synapse/storage/database.py b/synapse/storage/database.py index 6089225d9fb9..8e7195863517 100644 --- a/synapse/storage/database.py +++ b/synapse/storage/database.py @@ -1130,7 +1130,7 @@ def simple_insert_many_txn( keys: list of column names values: for each row, a list of values in the same order as `keys` """ - # IF there's nothing to insert, don't send the query. + # If there's nothing to insert, then skip executing the query. if not values: return @@ -1916,6 +1916,7 @@ def simple_select_many_txn( Returns: The results as a list of tuples. """ + # If there's nothing to select, then skip executing the query. if not iterable: return [] @@ -2050,6 +2051,7 @@ def simple_update_many_txn( raise ValueError( f"{len(key_values)} key rows and {len(value_values)} value rows: should be the same number." ) + # If there is nothing to update, then skip executing the query. if not key_values: return @@ -2286,6 +2288,7 @@ def simple_delete_many_txn( Returns: Number rows deleted """ + # If there's nothing to delete, then skip executing the query. if not values: return 0 From 14b4086157fe81d1b9bf5e40816c801a0c14b80f Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 7 Nov 2023 12:26:49 -0500 Subject: [PATCH 6/6] Clarify how value_names is handled for upsert_many. --- synapse/storage/database.py | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/synapse/storage/database.py b/synapse/storage/database.py index 8e7195863517..792f2e7cdf92 100644 --- a/synapse/storage/database.py +++ b/synapse/storage/database.py @@ -1471,9 +1471,19 @@ def simple_upsert_many_txn( value_values: A list of each row's value column values. Ignored if value_names is empty. """ + # If there's nothing to upsert, then skip executing the query. if not key_values: return + # No value columns, therefore make a blank list so that the following + # zip() works correctly. + if not value_names: + value_values = [() for x in range(len(key_values))] + elif len(value_values) != len(key_values): + raise ValueError( + f"{len(key_values)} key rows and {len(value_values)} value rows: should be the same number." + ) + if table not in self._unsafe_to_upsert_tables: return self.simple_upsert_many_txn_native_upsert( txn, table, key_names, key_values, value_names, value_values @@ -1508,10 +1518,6 @@ def simple_upsert_many_txn_emulated( value_values: A list of each row's value column values. Ignored if value_names is empty. """ - # No value columns, therefore make a blank list so that the following - # zip() works correctly. - if not value_names: - value_values = [() for x in range(len(key_values))] # Lock the table just once, to prevent it being done once per row. # Note that, according to Postgres' documentation, once obtained, @@ -1549,10 +1555,7 @@ def simple_upsert_many_txn_native_upsert( allnames.extend(value_names) if not value_names: - # No value columns, therefore make a blank list so that the - # following zip() works correctly. latter = "NOTHING" - value_values = [() for x in range(len(key_values))] else: latter = "UPDATE SET " + ", ".join( k + "=EXCLUDED." + k for k in value_names