Do not always start a db txn on Postgres

[anoadragon453]

While running the unit tests locally and watching my postgres logs, I saw the following log line start to be emitted for every DB query that was being run:

2023-01-13 18:05:15.379 GMT [493791] WARNING:  there is already a transaction in progress

Which resulted in quite a lot of log lines, generating noise...

This warning is harmless, and is merely telling us that we've started a transaction (with BEGIN) and then started another one (with another BEGIN). Now this warning will only appear when we actually prepare a database for the first time (which we do all the time when running the unit tests, as each tests uses a separate database) or apply database migrations.

So while on your actual homeserver you may see this warning pop up a couple times when you update Synapse, it won't be very frequent.

In any case, it should be a harmless change and reduces some noise, mostly for devs :)

[clokep]

Is db_conn.autocmmit always false for sqlite? Can we simplify this to:

Suggested change

	if isinstance(database_engine, Sqlite3Engine) or (
	isinstance(database_engine, PostgresEngine)
	and db_conn.autocommit
	):
	if db_conn.autocommit:

Note that attempting to set auto-commit for sqlite seems to be a no-op:

synapse/synapse/storage/engines/sqlite.py

Lines 114 to 119 in 8c94dd3

	def attempt_to_set_autocommit(
	self, conn: sqlite3.Connection, autocommit: bool
	) -> None:
	# Twisted doesn't let us set attributes on the connections, so we can't
	# set the connection to autocommit mode.
	pass

[anoadragon453]

From playing around in the manhole, it sadly appears that autocommit isn't a valid attribute in the case of SQLite:

>>> hs.get_datastores().main.db_pool.runWithConnection(lambda conn: print("ac:", conn.autocommit))
<Deferred #0>
Deferred #0 failed: "'sqlite3.Connection' object has no attribute 'autocommit'"

[DMRobertson]

https://docs.python.org/3/library/sqlite3.html?highlight=sqlite3#transaction-control

The sqlite3 module does not adhere to the transaction handling recommended by PEP 249.

🫠🫠

[clokep]

That's annoying. I secretly want to burn all isinstance(foo, FooEngine)....but not a big deal. 👍

[DMRobertson]

It looks like sqlite can be in an autocommit mode or an implicit transaction mode, depending on the value of isolation_level.

It'd be nice if we could abstract these details away into the DB engines somehow. The ideal would be something like

if db_engine.will_autocommit(db_conn):
    cur.execute("BEGIN TRANSACTION")

but I don't know if that's going to open up an even bigger can of worms. Maybe see what the rest of the team think?

[squahtx]

The abstraction sounds like a good idea. I'd not block the PR on it though.

[squahtx]

lgtm