diff --git a/.ci/scripts/calculate_jobs.py b/.ci/scripts/calculate_jobs.py index 5249acdc5d6..f3b1bb1503c 100755 --- a/.ci/scripts/calculate_jobs.py +++ b/.ci/scripts/calculate_jobs.py @@ -99,24 +99,24 @@ def set_output(key: str, value: str): # First calculate the various sytest jobs. # -# For each type of test we only run on bullseye on PRs +# For each type of test we only run on bookworm on PRs sytest_tests = [ { - "sytest-tag": "bullseye", + "sytest-tag": "bookworm", }, { - "sytest-tag": "bullseye", + "sytest-tag": "bookworm", "postgres": "postgres", }, { - "sytest-tag": "bullseye", + "sytest-tag": "bookworm", "postgres": "multi-postgres", "workers": "workers", }, { - "sytest-tag": "bullseye", + "sytest-tag": "bookworm", "postgres": "multi-postgres", "workers": "workers", "reactor": "asyncio", @@ -127,11 +127,11 @@ def set_output(key: str, value: str): sytest_tests.extend( [ { - "sytest-tag": "bullseye", + "sytest-tag": "bookworm", "reactor": "asyncio", }, { - "sytest-tag": "bullseye", + "sytest-tag": "bookworm", "postgres": "postgres", "reactor": "asyncio", }, diff --git a/.github/workflows/latest_deps.yml b/.github/workflows/latest_deps.yml index a00c52fcb21..526546531a7 100644 --- a/.github/workflows/latest_deps.yml +++ b/.github/workflows/latest_deps.yml @@ -139,9 +139,9 @@ jobs: fail-fast: false matrix: include: - - sytest-tag: bullseye + - sytest-tag: bookworm - - sytest-tag: bullseye + - sytest-tag: bookworm postgres: postgres workers: workers redis: redis diff --git a/.github/workflows/twisted_trunk.yml b/.github/workflows/twisted_trunk.yml index 04a8db2cc77..3c3e237b909 100644 --- a/.github/workflows/twisted_trunk.yml +++ b/.github/workflows/twisted_trunk.yml @@ -108,11 +108,13 @@ jobs: if: needs.check_repo.outputs.should_run_workflow == 'true' runs-on: ubuntu-latest container: - # We're using debian:bullseye because it uses Python 3.9 which is our minimum supported Python version. + # We're using bookworm because that's what Debian oldstable is at the time of writing. + # It uses Python 3.11, but sytest-synapse:bookworm is built with Python 3.9, + # which is our minimum supported Python version. # This job is a canary to warn us about unreleased twisted changes that would cause problems for us if # they were to be released immediately. For simplicity's sake (and to save CI runners) we use the oldest # version, assuming that any incompatibilities on newer versions would also be present on the oldest. - image: matrixdotorg/sytest-synapse:bullseye + image: matrixdotorg/sytest-synapse:bookworm volumes: - ${{ github.workspace }}:/src diff --git a/changelog.d/19047.doc b/changelog.d/19047.doc new file mode 100644 index 00000000000..fee241f2a57 --- /dev/null +++ b/changelog.d/19047.doc @@ -0,0 +1 @@ +Update the link to the Debian oldstable package for SQLite. diff --git a/changelog.d/19047.feature b/changelog.d/19047.feature new file mode 100644 index 00000000000..5a4cae67e7a --- /dev/null +++ b/changelog.d/19047.feature @@ -0,0 +1 @@ +Always treat `RETURNING` as supported by SQL engines, now that the minimum-supported versions of both SQLite and Postgres support it. diff --git a/docs/deprecation_policy.md b/docs/deprecation_policy.md index 2f3a09723e5..06c724d3489 100644 --- a/docs/deprecation_policy.md +++ b/docs/deprecation_policy.md @@ -21,7 +21,7 @@ people building from source should ensure they can fetch recent versions of Rust (e.g. by using [rustup](https://rustup.rs/)). The oldest supported version of SQLite is the version -[provided](https://packages.debian.org/bullseye/libsqlite3-0) by +[provided](https://packages.debian.org/oldstable/libsqlite3-0) by [Debian oldstable](https://wiki.debian.org/DebianOldStable). diff --git a/docs/development/contributing_guide.md b/docs/development/contributing_guide.md index eb6f04e301c..41fff1d6a37 100644 --- a/docs/development/contributing_guide.md +++ b/docs/development/contributing_guide.md @@ -320,7 +320,7 @@ The following command will let you run the integration test with the most common configuration: ```sh -$ docker run --rm -it -v /path/where/you/have/cloned/the/repository\:/src:ro -v /path/to/where/you/want/logs\:/logs matrixdotorg/sytest-synapse:bullseye +$ docker run --rm -it -v /path/where/you/have/cloned/the/repository\:/src:ro -v /path/to/where/you/want/logs\:/logs matrixdotorg/sytest-synapse:bookworm ``` (Note that the paths must be full paths! You could also write `$(realpath relative/path)` if needed.) diff --git a/synapse/storage/database.py b/synapse/storage/database.py index a4b2b26795c..3e404143cb9 100644 --- a/synapse/storage/database.py +++ b/synapse/storage/database.py @@ -1165,36 +1165,17 @@ def simple_insert_returning_txn( SQLite versions that don't support it). """ - if txn.database_engine.supports_returning: - sql = "INSERT INTO %s (%s) VALUES(%s) RETURNING %s" % ( - table, - ", ".join(k for k in values.keys()), - ", ".join("?" for _ in values.keys()), - ", ".join(k for k in returning), - ) - - txn.execute(sql, list(values.values())) - row = txn.fetchone() - assert row is not None - return row - else: - # For old versions of SQLite we do a standard insert and then can - # use `last_insert_rowid` to get at the row we just inserted - DatabasePool.simple_insert_txn( - txn, - table=table, - values=values, - ) - txn.execute("SELECT last_insert_rowid()") - row = txn.fetchone() - assert row is not None - (rowid,) = row + sql = "INSERT INTO %s (%s) VALUES(%s) RETURNING %s" % ( + table, + ", ".join(k for k in values.keys()), + ", ".join("?" for _ in values.keys()), + ", ".join(k for k in returning), + ) - row = DatabasePool.simple_select_one_txn( - txn, table=table, keyvalues={"rowid": rowid}, retcols=returning - ) - assert row is not None - return row + txn.execute(sql, list(values.values())) + row = txn.fetchone() + assert row is not None + return row async def simple_insert_many( self, diff --git a/synapse/storage/databases/main/delayed_events.py b/synapse/storage/databases/main/delayed_events.py index 78f55b983f0..2d8f9619c09 100644 --- a/synapse/storage/databases/main/delayed_events.py +++ b/synapse/storage/databases/main/delayed_events.py @@ -347,33 +347,28 @@ def process_target_delayed_event_txn( EventDetails, Optional[Timestamp], ]: - sql_cols = ", ".join( - ( - "room_id", - "event_type", - "state_key", - "origin_server_ts", - "content", - "device_id", - ) - ) - sql_update = "UPDATE delayed_events SET is_processed = TRUE" - sql_where = "WHERE delay_id = ? AND user_localpart = ? AND NOT is_processed" - sql_args = (delay_id, user_localpart) txn.execute( + """ + UPDATE delayed_events + SET is_processed = TRUE + WHERE delay_id = ? AND user_localpart = ? + AND NOT is_processed + RETURNING + room_id, + event_type, + state_key, + origin_server_ts, + content, + device_id + """, ( - f"{sql_update} {sql_where} RETURNING {sql_cols}" - if self.database_engine.supports_returning - else f"SELECT {sql_cols} FROM delayed_events {sql_where}" + delay_id, + user_localpart, ), - sql_args, ) row = txn.fetchone() if row is None: raise NotFoundError("Delayed event not found") - elif not self.database_engine.supports_returning: - txn.execute(f"{sql_update} {sql_where}", sql_args) - assert txn.rowcount == 1 event = EventDetails( RoomID.from_string(row[0]), diff --git a/synapse/storage/databases/main/event_federation.py b/synapse/storage/databases/main/event_federation.py index d77420ff475..77e77b4ff01 100644 --- a/synapse/storage/databases/main/event_federation.py +++ b/synapse/storage/databases/main/event_federation.py @@ -2045,61 +2045,29 @@ async def remove_received_event_from_staging( Returns: The received_ts of the row that was deleted, if any. """ - if self.db_pool.engine.supports_returning: - def _remove_received_event_from_staging_txn( - txn: LoggingTransaction, - ) -> Optional[int]: - sql = """ - DELETE FROM federation_inbound_events_staging - WHERE origin = ? AND event_id = ? - RETURNING received_ts - """ - - txn.execute(sql, (origin, event_id)) - row = cast(Optional[Tuple[int]], txn.fetchone()) - - if row is None: - return None - - return row[0] - - return await self.db_pool.runInteraction( - "remove_received_event_from_staging", - _remove_received_event_from_staging_txn, - db_autocommit=True, - ) + def _remove_received_event_from_staging_txn( + txn: LoggingTransaction, + ) -> Optional[int]: + sql = """ + DELETE FROM federation_inbound_events_staging + WHERE origin = ? AND event_id = ? + RETURNING received_ts + """ - else: + txn.execute(sql, (origin, event_id)) + row = cast(Optional[Tuple[int]], txn.fetchone()) - def _remove_received_event_from_staging_txn( - txn: LoggingTransaction, - ) -> Optional[int]: - received_ts = self.db_pool.simple_select_one_onecol_txn( - txn, - table="federation_inbound_events_staging", - keyvalues={ - "origin": origin, - "event_id": event_id, - }, - retcol="received_ts", - allow_none=True, - ) - self.db_pool.simple_delete_txn( - txn, - table="federation_inbound_events_staging", - keyvalues={ - "origin": origin, - "event_id": event_id, - }, - ) + if row is None: + return None - return received_ts + return row[0] - return await self.db_pool.runInteraction( - "remove_received_event_from_staging", - _remove_received_event_from_staging_txn, - ) + return await self.db_pool.runInteraction( + "remove_received_event_from_staging", + _remove_received_event_from_staging_txn, + db_autocommit=True, + ) async def get_next_staged_event_id_for_room( self, diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py index 906d1a91f68..55bc2e7ef67 100644 --- a/synapse/storage/databases/main/registration.py +++ b/synapse/storage/databases/main/registration.py @@ -2544,31 +2544,13 @@ def user_delete_access_tokens_for_devices_txn( ) args.append(user_id) - if self.database_engine.supports_returning: - sql = f""" - DELETE FROM access_tokens - WHERE {clause} AND user_id = ? - RETURNING token, id, device_id - """ - txn.execute(sql, args) - tokens_and_devices = txn.fetchall() - else: - tokens_and_devices = self.db_pool.simple_select_many_txn( - txn, - table="access_tokens", - column="device_id", - iterable=batch_device_ids, - keyvalues={"user_id": user_id}, - retcols=("token", "id", "device_id"), - ) - - self.db_pool.simple_delete_many_txn( - txn, - table="access_tokens", - keyvalues={"user_id": user_id}, - column="device_id", - values=batch_device_ids, - ) + sql = f""" + DELETE FROM access_tokens + WHERE {clause} AND user_id = ? + RETURNING token, id, device_id + """ + txn.execute(sql, args) + tokens_and_devices = txn.fetchall() self._invalidate_cache_and_stream_bulk( txn, diff --git a/synapse/storage/databases/main/user_directory.py b/synapse/storage/databases/main/user_directory.py index 9deb9ab73c2..03ee7cee185 100644 --- a/synapse/storage/databases/main/user_directory.py +++ b/synapse/storage/databases/main/user_directory.py @@ -356,27 +356,19 @@ async def _populate_user_directory_process_users( def _populate_user_directory_process_users_txn( txn: LoggingTransaction, ) -> Optional[int]: - if self.database_engine.supports_returning: - # Note: we use an ORDER BY in the SELECT to force usage of an - # index. Otherwise, postgres does a sequential scan that is - # surprisingly slow (I think due to the fact it will read/skip - # over lots of already deleted rows). - sql = f""" - DELETE FROM {TEMP_TABLE + "_users"} - WHERE user_id IN ( - SELECT user_id FROM {TEMP_TABLE + "_users"} ORDER BY user_id LIMIT ? - ) - RETURNING user_id - """ - txn.execute(sql, (batch_size,)) - user_result = cast(List[Tuple[str]], txn.fetchall()) - else: - sql = "SELECT user_id FROM %s ORDER BY user_id LIMIT %s" % ( - TEMP_TABLE + "_users", - str(batch_size), + # Note: we use an ORDER BY in the SELECT to force usage of an + # index. Otherwise, postgres does a sequential scan that is + # surprisingly slow (I think due to the fact it will read/skip + # over lots of already deleted rows). + sql = f""" + DELETE FROM {TEMP_TABLE + "_users"} + WHERE user_id IN ( + SELECT user_id FROM {TEMP_TABLE + "_users"} ORDER BY user_id LIMIT ? ) - txn.execute(sql) - user_result = cast(List[Tuple[str]], txn.fetchall()) + RETURNING user_id + """ + txn.execute(sql, (batch_size,)) + user_result = cast(List[Tuple[str]], txn.fetchall()) if not user_result: return None @@ -435,17 +427,6 @@ def _populate_user_directory_process_users_txn( # Actually insert the users with their profiles into the directory. self._update_profiles_in_user_dir_txn(txn, profiles_to_insert) - # We've finished processing the users. Delete it from the table, if - # we haven't already. - if not self.database_engine.supports_returning: - self.db_pool.simple_delete_many_txn( - txn, - table=TEMP_TABLE + "_users", - column="user_id", - values=users_to_work_on, - keyvalues={}, - ) - # Update the remaining counter. progress["remaining"] -= len(users_to_work_on) self.db_pool.updates._background_update_progress_txn( diff --git a/synapse/storage/engines/_base.py b/synapse/storage/engines/_base.py index 9fec42c2e0e..be6981f77ce 100644 --- a/synapse/storage/engines/_base.py +++ b/synapse/storage/engines/_base.py @@ -63,12 +63,6 @@ def supports_using_any_list(self) -> bool: """ ... - @property - @abc.abstractmethod - def supports_returning(self) -> bool: - """Do we support the `RETURNING` clause in insert/update/delete?""" - ... - @abc.abstractmethod def check_database( self, db_conn: ConnectionType, allow_outdated_version: bool = False diff --git a/synapse/storage/engines/postgres.py b/synapse/storage/engines/postgres.py index e4cd359201b..06bd1e5b603 100644 --- a/synapse/storage/engines/postgres.py +++ b/synapse/storage/engines/postgres.py @@ -193,11 +193,6 @@ def supports_using_any_list(self) -> bool: """Do we support using `a = ANY(?)` and passing a list""" return True - @property - def supports_returning(self) -> bool: - """Do we support the `RETURNING` clause in insert/update/delete?""" - return True - def is_deadlock(self, error: Exception) -> bool: if isinstance(error, psycopg2.DatabaseError): # https://www.postgresql.org/docs/current/static/errcodes-appendix.html diff --git a/synapse/storage/engines/sqlite.py b/synapse/storage/engines/sqlite.py index 9d1795ebe59..6de718fe491 100644 --- a/synapse/storage/engines/sqlite.py +++ b/synapse/storage/engines/sqlite.py @@ -68,11 +68,6 @@ def supports_using_any_list(self) -> bool: """Do we support using `a = ANY(?)` and passing a list""" return False - @property - def supports_returning(self) -> bool: - """Do we support the `RETURNING` clause in insert/update/delete?""" - return sqlite3.sqlite_version_info >= (3, 35, 0) - def check_database( self, db_conn: sqlite3.Connection, allow_outdated_version: bool = False ) -> None: @@ -80,8 +75,8 @@ def check_database( # Synapse is untested against older SQLite versions, and we don't want # to let users upgrade to a version of Synapse with broken support for their # sqlite version, because it risks leaving them with a half-upgraded db. - if sqlite3.sqlite_version_info < (3, 27, 0): - raise RuntimeError("Synapse requires sqlite 3.27 or above.") + if sqlite3.sqlite_version_info < (3, 40, 0): + raise RuntimeError("Synapse requires sqlite 3.40 or above.") def check_new_database(self, txn: Cursor) -> None: """Gets called when setting up a brand new database. This allows us to