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

Run database updates in a transaction #8265

Merged
merged 2 commits into from
Sep 7, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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/8265.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix logstanding bug which could lead to incomplete database upgrades on SQLite.
27 changes: 22 additions & 5 deletions synapse/storage/prepare_database.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,15 @@
import os
import re
from collections import Counter
from typing import TextIO
from typing import Optional, TextIO

import attr

from synapse.config.homeserver import HomeServerConfig
from synapse.storage.engines import BaseDatabaseEngine
from synapse.storage.engines.postgres import PostgresEngine
from synapse.storage.types import Cursor
from synapse.storage.types import Connection, Cursor
from synapse.types import Collection

logger = logging.getLogger(__name__)

Expand All @@ -47,7 +50,12 @@ class UpgradeDatabaseException(PrepareDatabaseException):
pass


def prepare_database(db_conn, database_engine, config, databases=["main", "state"]):
def prepare_database(
db_conn: Connection,
database_engine: BaseDatabaseEngine,
config: Optional[HomeServerConfig],
databases: Collection[str] = ["main", "state"],
):
"""Prepares a physical database for usage. Will either create all necessary tables
or upgrade from an older schema version.

Expand All @@ -57,15 +65,24 @@ def prepare_database(db_conn, database_engine, config, databases=["main", "state
Args:
db_conn:
database_engine:
config (synapse.config.homeserver.HomeServerConfig|None):
config :
application config, or None if we are connecting to an existing
database which we expect to be configured already
databases (list[str]): The name of the databases that will be used
databases: The name of the databases that will be used
with this physical database. Defaults to all databases.
"""

try:
cur = db_conn.cursor()

# sqlite does not automatically start transactions for DDL / SELECT statements,
# 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")
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this correctly sets the isolation level that we set on the connection? I suppose it doesn't really matter too much

Copy link
Member Author

Choose a reason for hiding this comment

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

well, the isolation level is set when we open the connection. I don't think there's a need to do it again here?


version_info = _get_or_create_schema_state(cur, database_engine)

if version_info:
Expand Down