Avoid executing no-op queries. (#16583)

If simple_{insert,upsert,update}_many_txn is called without any data
to modify then return instead of executing the query.

This matches the behavior of simple_{select,delete}_many_txn.

Author: clokep

changelog.d/16583.misc

@@ -0,0 +1 @@
+Avoid executing no-op queries.

synapse/storage/database.py

@@ -1117,7 +1117,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.
 
@@ -1130,6 +1130,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, then skip executing 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`,
@@ -1455,7 +1458,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.
@@ -1468,6 +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
@@ -1502,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,
@@ -1543,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
@@ -1910,6 +1919,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 []
 
@@ -2044,6 +2054,9 @@ 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
 
         # List of tuples of (value values, then key values)
         # (This matches the order needed for the query)
@@ -2278,6 +2291,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
 

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

synapse/storage/databases/main/events.py

@@ -1476,7 +1476,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,
@@ -1485,7 +1485,7 @@ def event_dict(event: EventBase) -> JsonDict:
                     event.format_version,
                 )
                 for event, _ in events_and_contexts
-            ),
+            ],
         )
 
         self.db_pool.simple_insert_many_txn(
@@ -1508,7 +1508,7 @@ def event_dict(event: EventBase) -> JsonDict:
                 "state_key",
                 "rejection_reason",
             ),
-            values=(
+            values=[
                 (
                     self._instance_name,
                     event.internal_metadata.stream_ordering,
@@ -1527,7 +1527,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
@@ -1550,11 +1550,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(

synapse/storage/databases/main/room.py

@@ -2268,7 +2268,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(

synapse/storage/databases/main/search.py

@@ -106,15 +106,15 @@ def store_search_entries_txn(
                 txn,
                 table="event_search",
                 keys=("event_id", "room_id", "key", "value"),
-                values=(
+                values=[
                     (
                         entry.event_id,
                         entry.room_id,
                         entry.key,
                         _clean_value_for_search(entry.value),
                     )
                     for entry in entries
-                ),
+                ],
             )
 
         else:

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]: