Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Do not always start a db txn on Postgres #14840

Merged
merged 5 commits into from
Feb 9, 2023
Merged

Do not always start a db txn on Postgres #14840

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
744a25e
Don't always start a db txn on Postgres
anoadragon453 Jan 13, 2023
a4aa150
changelog
anoadragon453 Jan 13, 2023
acc4a5c
Postgres: Only begin a txn when autocommit mode is enabled
anoadragon453 Jan 18, 2023
ddd7ecb
words
anoadragon453 Jan 18, 2023
54d5958
lint
anoadragon453 Feb 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/14840.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Prevent "WARNING: there is already a transaction in progress" lines appearing in PostgreSQL's logs on some occasions.
14 changes: 10 additions & 4 deletions synapse/storage/prepare_database.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@

from synapse.config.homeserver import HomeServerConfig
from synapse.storage.database import LoggingDatabaseConnection
from synapse.storage.engines import BaseDatabaseEngine, PostgresEngine
from synapse.storage.engines import BaseDatabaseEngine, PostgresEngine, Sqlite3Engine
from synapse.storage.schema import SCHEMA_COMPAT_VERSION, SCHEMA_VERSION
from synapse.storage.types import Cursor

Expand Down Expand Up @@ -108,9 +108,15 @@ def prepare_database(
# so we start one before running anything. This ensures that any upgrades
# are either applied completely, or not at all.
#
# (psycopg2 automatically starts a transaction as soon as we run any statements
# at all, so this is redundant but harmless there.)
cur.execute("BEGIN TRANSACTION")
# psycopg2 does not automatically start transactions when in autocommit mode.
# While it is technically harmless to nest transactions in postgres, doing so
# results in a warning in Postgres' logs per query. And we'd rather like to
# avoid doing that.
if isinstance(database_engine, Sqlite3Engine) or (
isinstance(database_engine, PostgresEngine)
and db_conn.autocommit
):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

🫠🫠

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

cur.execute("BEGIN TRANSACTION")

logger.info("%r: Checking existing schema version", databases)
version_info = _get_or_create_schema_state(cur, database_engine)
Expand Down