Remove Generator in _quarantine_media_txn()

[realtyem]

when updating the remote_media_cache table

Context: matrix-org/synapse#15439 (#15439)

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file.
• Code style is correct
  (run the linters)

[CLAassistant]

All committers have signed the CLA.

[anoadragon453]

@realtyem do you have a reference for why providing generators to txn.executemany or txn.execute_batch may be a bad idea?

[realtyem]

@realtyem do you have a reference for why providing generators to txn.executemany or txn.execute_batch may be a bad idea?

matrix-org/synapse#15439

Is a lot of the basis. From that, I distill the TLDR as "Generators may cause inadvertent data loss during a db query retry by exhausting an iterable"

[realtyem]

The comments at spot seem to be relevant. Although this set of PR's may not necessarily be (part of|used by) this infrastructure, still seems prudent to not use generators as a good habit/safety precaution. It may be they end up used in the future, so don't let our tomorrow selves find the foot gun. (And if they do get used in this infrastructure, then is the 'inspection' infrastructure in this function working?) (edit: yes it's working as intended)

synapse/synapse/storage/database.py

Lines 683 to 695 in 11bc9a1

	"""Start a new database transaction with the given connection.
	Note: The given func may be called multiple times under certain
	failure modes. This is normally fine when in a standard transaction,
	but care must be taken if the connection is in `autocommit` mode that
	the function will correctly handle being aborted and retried half way
	through its execution.
	Similarly, the arguments to `func` (`args`, `kwargs`) should not be generators,
	since they could be evaluated multiple times (which would produce an empty
	result on the second or subsequent evaluation). Likewise, the closure of `func`
	must not reference any generators. This method attempts to detect such usage
	and will log an error.

[realtyem]

I believe I misunderstood your question, my apologizes.

In and of itself, there is no reason I know of why you can not use a Generator to produce the arguments needed for a call to executemany(), for either the sqlite3 library or psycopg2.

However, coming shortly in the future is a bit of a rework for a fair chunk of the database interfacing code and some tightening up of the Typing around it would be a really really good idea. I propose a continuation of work done previously(linked in the issue above, but several links deep into it, I believe). It would make the arguments used for executemany() and execute_batch() need to be a Sequence as it's outer container instead of an Iterable. There is some thoughts around using a Collection instead, but I believe that using a grouping of arguments that enforces 'deterministic order' to be the correct way forward.

Since that is as simple as a List or, preferably, a Tuple, I believe it is not an especially hard hill to climb.

[MadLittleMods]

This seems fine to do given Cursor.executemany(..., parameters: Sequence[SQLQueryParameters]), a type further down the line of LoggingTransaction.executemany(...)

synapse/synapse/storage/types.py

Lines 47 to 49 in 3ae80b0

	def executemany(
	self, sql: str, parameters: Sequence[SQLQueryParameters]
	) -> Any: ...

And the goal is to add proper type hints to LoggingTransaction.executemany(...). We can see these generator lints when trying to add the types, see matrix-org/synapse#15439 (comment)