From 73b17b60c0dc66e30b5aff70286bfe228437ff22 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Wed, 3 May 2023 15:19:37 -0700 Subject: [PATCH 01/16] add not null check to column full_user_id in tables profiles and user_filters --- .../01_add_profiles_not_valid_check.sql.postgres | 16 ++++++++++++++++ ...add_user_filters_not_valid_check.sql.postgres | 16 ++++++++++++++++ 2 files changed, 32 insertions(+) create mode 100644 synapse/storage/schema/main/delta/77/01_add_profiles_not_valid_check.sql.postgres create mode 100644 synapse/storage/schema/main/delta/77/02_add_user_filters_not_valid_check.sql.postgres diff --git a/synapse/storage/schema/main/delta/77/01_add_profiles_not_valid_check.sql.postgres b/synapse/storage/schema/main/delta/77/01_add_profiles_not_valid_check.sql.postgres new file mode 100644 index 000000000000..51372b693319 --- /dev/null +++ b/synapse/storage/schema/main/delta/77/01_add_profiles_not_valid_check.sql.postgres @@ -0,0 +1,16 @@ +/* Copyright 2023 The Matrix.org Foundation C.I.C + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +ALTER TABLE profiles ADD CHECK (full_user_id IS NOT NULL) NOT VALID; \ No newline at end of file diff --git a/synapse/storage/schema/main/delta/77/02_add_user_filters_not_valid_check.sql.postgres b/synapse/storage/schema/main/delta/77/02_add_user_filters_not_valid_check.sql.postgres new file mode 100644 index 000000000000..a8e5d3c91a82 --- /dev/null +++ b/synapse/storage/schema/main/delta/77/02_add_user_filters_not_valid_check.sql.postgres @@ -0,0 +1,16 @@ +/* Copyright 2023 The Matrix.org Foundation C.I.C + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +ALTER TABLE user_filters ADD CHECK (full_user_id IS NOT NULL) NOT VALID; \ No newline at end of file From 46ef8020e6ce904569fe755f47f39583063aa717 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Wed, 3 May 2023 15:20:42 -0700 Subject: [PATCH 02/16] add background update to copy entries from column `user_id` in table profiles to column `full_user_id` --- synapse/storage/databases/main/profile.py | 88 ++++++++++++++++++- .../03bg_populate_full_user_id_profiles.sql | 16 ++++ 2 files changed, 102 insertions(+), 2 deletions(-) create mode 100644 synapse/storage/schema/main/delta/77/03bg_populate_full_user_id_profiles.sql diff --git a/synapse/storage/databases/main/profile.py b/synapse/storage/databases/main/profile.py index b109f8c07f1e..a6b87a8701bc 100644 --- a/synapse/storage/databases/main/profile.py +++ b/synapse/storage/databases/main/profile.py @@ -15,9 +15,13 @@ from synapse.api.errors import StoreError from synapse.storage._base import SQLBaseStore -from synapse.storage.database import DatabasePool, LoggingDatabaseConnection +from synapse.storage.database import ( + DatabasePool, + LoggingDatabaseConnection, + LoggingTransaction, +) from synapse.storage.databases.main.roommember import ProfileInfo -from synapse.types import UserID +from synapse.types import JsonDict, UserID if TYPE_CHECKING: from synapse.server import HomeServer @@ -31,6 +35,7 @@ def __init__( hs: "HomeServer", ): super().__init__(database, db_conn, hs) + self.server_name: str = hs.hostname self.db_pool.updates.register_background_index_update( "profiles_full_user_id_key_idx", index_name="profiles_full_user_id_key", @@ -39,6 +44,85 @@ def __init__( unique=True, ) + self.db_pool.updates.register_background_update_handler( + "populate_full_user_id_profiles", self.populate_full_user_id_profiles + ) + + async def populate_full_user_id_profiles( + self, progress: JsonDict, batch_size: int + ) -> int: + """ + Background update to populate the column `full_user_id` of the table + profiles from entries in the column `user_local_part` of the same table + """ + + lower_bound_id = progress.get("lower_bound_id", "") + + def _get_last_id(txn: LoggingTransaction) -> Optional[str]: + sql = """ + SELECT user_id FROM profiles + WHERE user_id > ? + ORDER BY user_id + LIMIT 1 OFFSET 50 + """ + txn.execute(sql, (lower_bound_id,)) + res = txn.fetchone() + if res: + upper_bound_id = res[0] + return upper_bound_id + else: + return None + + def _process_batch( + txn: LoggingTransaction, lower_bound_id: str, upper_bound_id: str + ) -> None: + sql = f""" + UPDATE profiles + SET full_user_id = user_id || ':{self.server_name}' + WHERE ? < user_id AND user_id <= ? AND full_user_id IS NULL + """ + txn.execute(sql, (lower_bound_id, upper_bound_id)) + + def _final_batch(txn: LoggingTransaction, lower_bound_id: str) -> None: + sql = f""" + UPDATE profiles + SET full_user_id = user_id || ':{self.server_name}' + WHERE ? < user_id AND full_user_id IS NULL + """ + txn.execute(sql, (lower_bound_id,)) + + upper_bound_id = await self.db_pool.runInteraction( + "populate_full_user_id_profiles", _get_last_id + ) + + if not upper_bound_id: + await self.db_pool.runInteraction( + "populate_full_user_id_profiles", _final_batch, lower_bound_id + ) + + await self.db_pool.updates._end_background_update( + "populate_full_user_id_profiles" + ) + return 1 + + await self.db_pool.runInteraction( + "populate_full_user_id_profiles", + _process_batch, + lower_bound_id, + upper_bound_id, + ) + + progress["lower_bound_id"] = upper_bound_id + + await self.db_pool.runInteraction( + "populate_full_user_id_profiles", + self.db_pool.updates._background_update_progress_txn, + "populate_full_user_id_profiles", + progress, + ) + + return 50 + async def get_profileinfo(self, user_localpart: str) -> ProfileInfo: try: profile = await self.db_pool.simple_select_one( diff --git a/synapse/storage/schema/main/delta/77/03bg_populate_full_user_id_profiles.sql b/synapse/storage/schema/main/delta/77/03bg_populate_full_user_id_profiles.sql new file mode 100644 index 000000000000..12101ab914d7 --- /dev/null +++ b/synapse/storage/schema/main/delta/77/03bg_populate_full_user_id_profiles.sql @@ -0,0 +1,16 @@ +/* Copyright 2023 The Matrix.org Foundation C.I.C + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +INSERT INTO background_updates (ordering, update_name, progress_json) VALUES (7703, 'populate_full_user_id_profiles', '{}'); \ No newline at end of file From de7d08e4134c123badab1951e2a0cefc90416fd1 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Wed, 3 May 2023 20:03:52 -0700 Subject: [PATCH 03/16] add background update to copy column `user_id` of the table user_filters to `full_user_id` of same table --- synapse/storage/databases/main/filtering.py | 81 +++++++++++++++++++ ...4bg_populate_full_user_id_user_filters.sql | 16 ++++ 2 files changed, 97 insertions(+) create mode 100644 synapse/storage/schema/main/delta/77/04bg_populate_full_user_id_user_filters.sql diff --git a/synapse/storage/databases/main/filtering.py b/synapse/storage/databases/main/filtering.py index 50516402f96f..4e41c08dcd1d 100644 --- a/synapse/storage/databases/main/filtering.py +++ b/synapse/storage/databases/main/filtering.py @@ -40,6 +40,7 @@ def __init__( hs: "HomeServer", ): super().__init__(database, db_conn, hs) + self.server_name: str = hs.hostname self.db_pool.updates.register_background_index_update( "full_users_filters_unique_idx", index_name="full_users_unique_idx", @@ -48,6 +49,86 @@ def __init__( unique=True, ) + self.db_pool.updates.register_background_update_handler( + "populate_full_user_id_user_filters", + self.populate_full_user_id_user_filters, + ) + + async def populate_full_user_id_user_filters( + self, progress: JsonDict, batch_size: int + ) -> int: + """ + Background update to populate the column `full_user_id` of the table + user_filters from entries in the column `user_local_part` of the same table + """ + + lower_bound_id = progress.get("lower_bound_id", "") + + def _get_last_id(txn: LoggingTransaction) -> Optional[str]: + sql = """ + SELECT user_id FROM user_filters + WHERE user_id > ? + ORDER BY user_id + LIMIT 1 OFFSET 50 + """ + txn.execute(sql, (lower_bound_id,)) + res = txn.fetchone() + if res: + upper_bound_id = res[0] + return upper_bound_id + else: + return None + + def _process_batch( + txn: LoggingTransaction, lower_bound_id: str, upper_bound_id: str + ) -> None: + sql = f""" + UPDATE user_filters + SET full_user_id = user_id || ':{self.server_name}' + WHERE ? < user_id AND user_id <= ? AND full_user_id IS NULL + """ + txn.execute(sql, (lower_bound_id, upper_bound_id)) + + def _final_batch(txn: LoggingTransaction, lower_bound_id: str) -> None: + sql = f""" + UPDATE user_filters + SET full_user_id = user_id || ':{self.server_name}' + WHERE ? < user_id AND full_user_id IS NULL + """ + txn.execute(sql, (lower_bound_id,)) + + upper_bound_id = await self.db_pool.runInteraction( + "populate_full_user_id_user_filters", _get_last_id + ) + + if not upper_bound_id: + await self.db_pool.runInteraction( + "populate_full_user_id_user_filters", _final_batch, lower_bound_id + ) + + await self.db_pool.updates._end_background_update( + "populate_full_user_id_user_filters" + ) + return 1 + + await self.db_pool.runInteraction( + "populate_full_user_id_user_filters", + _process_batch, + lower_bound_id, + upper_bound_id, + ) + + progress["lower_bound_id"] = upper_bound_id + + await self.db_pool.runInteraction( + "populate_full_user_id_user_filters", + self.db_pool.updates._background_update_progress_txn, + "populate_full_user_id_user_filters", + progress, + ) + + return 50 + @cached(num_args=2) async def get_user_filter( self, user_localpart: str, filter_id: Union[int, str] diff --git a/synapse/storage/schema/main/delta/77/04bg_populate_full_user_id_user_filters.sql b/synapse/storage/schema/main/delta/77/04bg_populate_full_user_id_user_filters.sql new file mode 100644 index 000000000000..1f4d683cac3c --- /dev/null +++ b/synapse/storage/schema/main/delta/77/04bg_populate_full_user_id_user_filters.sql @@ -0,0 +1,16 @@ +/* Copyright 2023 The Matrix.org Foundation C.I.C + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +INSERT INTO background_updates (ordering, update_name, progress_json) VALUES (7704, 'populate_full_user_id_user_filters', '{}'); \ No newline at end of file From 0cb14d83bda2b29a87fffe12eb34eea99b798105 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Wed, 3 May 2023 20:07:29 -0700 Subject: [PATCH 04/16] bump schema version --- synapse/storage/schema/__init__.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/synapse/storage/schema/__init__.py b/synapse/storage/schema/__init__.py index 1672976209d6..1a8f1c0c123b 100644 --- a/synapse/storage/schema/__init__.py +++ b/synapse/storage/schema/__init__.py @@ -12,7 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. -SCHEMA_VERSION = 76 # remember to update the list below when updating +SCHEMA_VERSION = 77 # remember to update the list below when updating """Represents the expectations made by the codebase about the database schema This should be incremented whenever the codebase changes its requirements on the @@ -100,13 +100,16 @@ Changes in SCHEMA_VERSION = 76: - Adds a full_user_id column to tables profiles and user_filters. + +Changes in SCHEMA_VERSION = 77 + - Add CHECK (full_user_id IS NOT NULL) to tables profiles and user_filters """ SCHEMA_COMPAT_VERSION = ( - # Queries against `event_stream_ordering` columns in membership tables must - # be disambiguated. - 74 + # insertions to the column `full_user_id` of tables profiles and user_filters can no + # longer be null + 75 ) """Limit on how far the synapse codebase can be rolled back without breaking db compat From b82c0c8b7aaf675244a23b28ca01655192750e55 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Wed, 3 May 2023 20:16:36 -0700 Subject: [PATCH 05/16] newsfragment --- changelog.d/15537.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/15537.misc diff --git a/changelog.d/15537.misc b/changelog.d/15537.misc new file mode 100644 index 000000000000..979e0ba97719 --- /dev/null +++ b/changelog.d/15537.misc @@ -0,0 +1 @@ +Add not null constraint to column full_user_id of tables profiles and user_filters. From a735a01daa3b2efd2f1f7d8c787bfc5140859218 Mon Sep 17 00:00:00 2001 From: Shay Date: Wed, 10 May 2023 11:25:36 -0700 Subject: [PATCH 06/16] Update synapse/storage/schema/__init__.py Co-authored-by: reivilibre --- synapse/storage/schema/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/schema/__init__.py b/synapse/storage/schema/__init__.py index ed068e646d24..e2499b03b784 100644 --- a/synapse/storage/schema/__init__.py +++ b/synapse/storage/schema/__init__.py @@ -102,7 +102,7 @@ - Adds a full_user_id column to tables profiles and user_filters. Changes in SCHEMA_VERSION = 77 - - Add CHECK (full_user_id IS NOT NULL) to tables profiles and user_filters + - (Postgres) Add NOT VALID CHECK (full_user_id IS NOT NULL) to tables profiles and user_filters """ From 92b03caf4be4d767216177d72252606317a4ece3 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Wed, 10 May 2023 14:57:34 -0700 Subject: [PATCH 07/16] name constraint --- .../main/delta/77/01_add_profiles_not_valid_check.sql.postgres | 2 +- .../delta/77/02_add_user_filters_not_valid_check.sql.postgres | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/storage/schema/main/delta/77/01_add_profiles_not_valid_check.sql.postgres b/synapse/storage/schema/main/delta/77/01_add_profiles_not_valid_check.sql.postgres index 51372b693319..3eb226c64869 100644 --- a/synapse/storage/schema/main/delta/77/01_add_profiles_not_valid_check.sql.postgres +++ b/synapse/storage/schema/main/delta/77/01_add_profiles_not_valid_check.sql.postgres @@ -13,4 +13,4 @@ * limitations under the License. */ -ALTER TABLE profiles ADD CHECK (full_user_id IS NOT NULL) NOT VALID; \ No newline at end of file +ALTER TABLE profiles ADD CONSTRAINT full_user_id_not_null CHECK (full_user_id IS NOT NULL) NOT VALID; \ No newline at end of file diff --git a/synapse/storage/schema/main/delta/77/02_add_user_filters_not_valid_check.sql.postgres b/synapse/storage/schema/main/delta/77/02_add_user_filters_not_valid_check.sql.postgres index a8e5d3c91a82..ba037daf471d 100644 --- a/synapse/storage/schema/main/delta/77/02_add_user_filters_not_valid_check.sql.postgres +++ b/synapse/storage/schema/main/delta/77/02_add_user_filters_not_valid_check.sql.postgres @@ -13,4 +13,4 @@ * limitations under the License. */ -ALTER TABLE user_filters ADD CHECK (full_user_id IS NOT NULL) NOT VALID; \ No newline at end of file +ALTER TABLE user_filters ADD CONSTRAINT full_user_id_not_null CHECK (full_user_id IS NOT NULL) NOT VALID; \ No newline at end of file From 0252c1085c95cd2a1b73231ba5aedf25940bf7d2 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Wed, 10 May 2023 14:57:52 -0700 Subject: [PATCH 08/16] requested changes --- synapse/storage/databases/main/filtering.py | 28 +++++++++++++++------ synapse/storage/databases/main/profile.py | 28 +++++++++++++++------ 2 files changed, 42 insertions(+), 14 deletions(-) diff --git a/synapse/storage/databases/main/filtering.py b/synapse/storage/databases/main/filtering.py index 4e41c08dcd1d..da31eb44dc96 100644 --- a/synapse/storage/databases/main/filtering.py +++ b/synapse/storage/databases/main/filtering.py @@ -25,6 +25,7 @@ LoggingDatabaseConnection, LoggingTransaction, ) +from synapse.storage.engines import PostgresEngine from synapse.types import JsonDict, UserID from synapse.util.caches.descriptors import cached @@ -41,6 +42,7 @@ def __init__( ): super().__init__(database, db_conn, hs) self.server_name: str = hs.hostname + self.database_engine = database.engine self.db_pool.updates.register_background_index_update( "full_users_filters_unique_idx", index_name="full_users_unique_idx", @@ -82,26 +84,38 @@ def _get_last_id(txn: LoggingTransaction) -> Optional[str]: def _process_batch( txn: LoggingTransaction, lower_bound_id: str, upper_bound_id: str ) -> None: - sql = f""" + sql = """ UPDATE user_filters - SET full_user_id = user_id || ':{self.server_name}' + SET full_user_id = '@' || user_id || ? WHERE ? < user_id AND user_id <= ? AND full_user_id IS NULL """ - txn.execute(sql, (lower_bound_id, upper_bound_id)) + txn.execute(sql, (f":{self.server_name}", lower_bound_id, upper_bound_id)) def _final_batch(txn: LoggingTransaction, lower_bound_id: str) -> None: - sql = f""" + sql = """ UPDATE user_filters - SET full_user_id = user_id || ':{self.server_name}' + SET full_user_id = '@' || user_id || ? WHERE ? < user_id AND full_user_id IS NULL """ - txn.execute(sql, (lower_bound_id,)) + txn.execute( + sql, + ( + f":{self.server_name}", + lower_bound_id, + ), + ) + + if isinstance(self.database_engine, PostgresEngine): + sql = """ + ALTER TABLE user_filters VALIDATE CONSTRAINT full_user_id_not_null + """ + txn.execute(sql) upper_bound_id = await self.db_pool.runInteraction( "populate_full_user_id_user_filters", _get_last_id ) - if not upper_bound_id: + if upper_bound_id is None: await self.db_pool.runInteraction( "populate_full_user_id_user_filters", _final_batch, lower_bound_id ) diff --git a/synapse/storage/databases/main/profile.py b/synapse/storage/databases/main/profile.py index a6b87a8701bc..f3b3d1f01d3f 100644 --- a/synapse/storage/databases/main/profile.py +++ b/synapse/storage/databases/main/profile.py @@ -21,6 +21,7 @@ LoggingTransaction, ) from synapse.storage.databases.main.roommember import ProfileInfo +from synapse.storage.engines import PostgresEngine from synapse.types import JsonDict, UserID if TYPE_CHECKING: @@ -36,6 +37,7 @@ def __init__( ): super().__init__(database, db_conn, hs) self.server_name: str = hs.hostname + self.database_engine = database.engine self.db_pool.updates.register_background_index_update( "profiles_full_user_id_key_idx", index_name="profiles_full_user_id_key", @@ -76,26 +78,38 @@ def _get_last_id(txn: LoggingTransaction) -> Optional[str]: def _process_batch( txn: LoggingTransaction, lower_bound_id: str, upper_bound_id: str ) -> None: - sql = f""" + sql = """ UPDATE profiles - SET full_user_id = user_id || ':{self.server_name}' + SET full_user_id = '@' || user_id || ? WHERE ? < user_id AND user_id <= ? AND full_user_id IS NULL """ - txn.execute(sql, (lower_bound_id, upper_bound_id)) + txn.execute(sql, (f":{self.server_name}", lower_bound_id, upper_bound_id)) def _final_batch(txn: LoggingTransaction, lower_bound_id: str) -> None: - sql = f""" + sql = """ UPDATE profiles - SET full_user_id = user_id || ':{self.server_name}' + SET full_user_id = '@' || user_id || ? WHERE ? < user_id AND full_user_id IS NULL """ - txn.execute(sql, (lower_bound_id,)) + txn.execute( + sql, + ( + f":{self.server_name}", + lower_bound_id, + ), + ) + + if isinstance(self.database_engine, PostgresEngine): + sql = """ + ALTER TABLE profiles VALIDATE CONSTRAINT full_user_id_not_null + """ + txn.execute(sql) upper_bound_id = await self.db_pool.runInteraction( "populate_full_user_id_profiles", _get_last_id ) - if not upper_bound_id: + if upper_bound_id is None: await self.db_pool.runInteraction( "populate_full_user_id_profiles", _final_batch, lower_bound_id ) From 2465b9d75ac8d91dd324aa909a0f8f29a0520963 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Wed, 10 May 2023 15:01:17 -0700 Subject: [PATCH 09/16] add tests --- tests/storage/test_profile.py | 64 ++++++++++++++++++++ tests/storage/test_user_filters.py | 96 ++++++++++++++++++++++++++++++ 2 files changed, 160 insertions(+) create mode 100644 tests/storage/test_user_filters.py diff --git a/tests/storage/test_profile.py b/tests/storage/test_profile.py index 6ec34997ea53..d5afcb714012 100644 --- a/tests/storage/test_profile.py +++ b/tests/storage/test_profile.py @@ -14,6 +14,8 @@ from twisted.test.proto_helpers import MemoryReactor from synapse.server import HomeServer +from synapse.storage.database import LoggingTransaction +from synapse.storage.engines import PostgresEngine from synapse.types import UserID from synapse.util import Clock @@ -69,3 +71,65 @@ def test_avatar_url(self) -> None: self.assertIsNone( self.get_success(self.store.get_profile_avatar_url(self.u_frank.localpart)) ) + + def test_profiles_bg_migration(self) -> None: + """ + Test background job that copies entries from column user_id to full_user_id, adding + the hostname in the process. + """ + updater = self.hs.get_datastores().main.db_pool.updates + + # drop the constraint so we can insert nulls in full_user_id to populate the test + if isinstance(self.store.database_engine, PostgresEngine): + + def f(txn: LoggingTransaction) -> None: + txn.execute( + "ALTER TABLE profiles DROP CONSTRAINT full_user_id_not_null" + ) + + self.get_success(self.store.db_pool.runInteraction("", f)) + + for i in range(0, 70): + self.get_success( + self.store.db_pool.simple_insert( + "profiles", + {"user_id": f"hello{i}"}, + ) + ) + + # re-add the constraint so that when it's validated it actually exists + if isinstance(self.store.database_engine, PostgresEngine): + + def f(txn: LoggingTransaction) -> None: + txn.execute( + "ALTER TABLE profiles ADD CONSTRAINT full_user_id_not_null CHECK (full_user_id IS NOT NULL) NOT VALID" + ) + + self.get_success(self.store.db_pool.runInteraction("", f)) + + self.get_success( + self.store.db_pool.simple_insert( + "background_updates", + values={ + "update_name": "populate_full_user_id_profiles", + "progress_json": "{}", + }, + ) + ) + + self.get_success( + updater.run_background_updates(False), + ) + + expected_values = [] + for i in range(0, 70): + expected_values.append((f"@hello{i}:{self.hs.hostname}",)) + + res = self.get_success( + self.store.db_pool.execute("", None, "SELECT full_user_id from profiles") + ) + self.assertEqual(len(res), len(expected_values)) + for value in res: + self.assertEqual(True, value in expected_values) + for value in expected_values: + self.assertEqual(True, value in res) diff --git a/tests/storage/test_user_filters.py b/tests/storage/test_user_filters.py new file mode 100644 index 000000000000..35f98ab923e8 --- /dev/null +++ b/tests/storage/test_user_filters.py @@ -0,0 +1,96 @@ +# Copyright 2023 The Matrix.org Foundation C.I.C +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + + +from twisted.internet.testing import MemoryReactor + +from synapse.server import HomeServer +from synapse.storage.database import LoggingTransaction +from synapse.storage.engines import PostgresEngine +from synapse.util import Clock + +from tests import unittest + + +class ProfileStoreTestCase(unittest.HomeserverTestCase): + """ + Test background migration that copies entries from column user_id to full_user_id, adding + the hostname in the process. + """ + def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: + self.store = hs.get_datastores().main + + def test_bg_migration2(self) -> None: + updater = self.hs.get_datastores().main.db_pool.updates + + # drop the constraint so we can insert nulls in full_user_id to populate the test + if isinstance(self.store.database_engine, PostgresEngine): + + def f(txn: LoggingTransaction) -> None: + txn.execute( + "ALTER TABLE user_filters DROP CONSTRAINT full_user_id_not_null" + ) + + self.get_success(self.store.db_pool.runInteraction("", f)) + + for i in range(0, 70): + self.get_success( + self.store.db_pool.simple_insert( + "user_filters", + { + "user_id": f"hello{i}", + "filter_id": i, + "filter_json": bytearray(i), + }, + ) + ) + + # re-add the constraint so that when it's validated it actually exists + if isinstance(self.store.database_engine, PostgresEngine): + + def f(txn: LoggingTransaction) -> None: + txn.execute( + "ALTER TABLE user_filters ADD CONSTRAINT full_user_id_not_null CHECK (full_user_id IS NOT NULL) NOT VALID" + ) + + self.get_success(self.store.db_pool.runInteraction("", f)) + + self.get_success( + self.store.db_pool.simple_insert( + "background_updates", + values={ + "update_name": "populate_full_user_id_user_filters", + "progress_json": "{}", + }, + ) + ) + + self.get_success( + updater.run_background_updates(False), + ) + + expected_values = [] + for i in range(0, 70): + expected_values.append((f"@hello{i}:{self.hs.hostname}",)) + + res = self.get_success( + self.store.db_pool.execute( + "", None, "SELECT full_user_id from user_filters ORDER BY full_user_id" + ) + ) + self.assertEqual(len(res), len(expected_values)) + for value in res: + self.assertEqual(True, value in expected_values) + for value in expected_values: + self.assertEqual(True, value in res) From 869c609c788e48da105b28e6ed8af1a591cc2412 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Wed, 10 May 2023 15:06:06 -0700 Subject: [PATCH 10/16] bump schema compat --- synapse/storage/schema/__init__.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/storage/schema/__init__.py b/synapse/storage/schema/__init__.py index ed068e646d24..5cc786f0303d 100644 --- a/synapse/storage/schema/__init__.py +++ b/synapse/storage/schema/__init__.py @@ -102,7 +102,7 @@ - Adds a full_user_id column to tables profiles and user_filters. Changes in SCHEMA_VERSION = 77 - - Add CHECK (full_user_id IS NOT NULL) to tables profiles and user_filters + - (Postgres) Add NOT VALID CHECK (full_user_id IS NOT NULL) to tables profiles and user_filters """ @@ -115,7 +115,7 @@ # # insertions to the column `full_user_id` of tables profiles and user_filters can no # longer be null - 75 + 76 ) """Limit on how far the synapse codebase can be rolled back without breaking db compat From 6fd76c0082d3ef1b69496058b5e1886f187b5dde Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Wed, 10 May 2023 15:10:06 -0700 Subject: [PATCH 11/16] lint --- tests/storage/test_user_filters.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/storage/test_user_filters.py b/tests/storage/test_user_filters.py index 35f98ab923e8..457a8113105d 100644 --- a/tests/storage/test_user_filters.py +++ b/tests/storage/test_user_filters.py @@ -28,6 +28,7 @@ class ProfileStoreTestCase(unittest.HomeserverTestCase): Test background migration that copies entries from column user_id to full_user_id, adding the hostname in the process. """ + def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: self.store = hs.get_datastores().main From 6075047e93352b6c84997aa9e77aebc19aa851c4 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Wed, 10 May 2023 15:26:07 -0700 Subject: [PATCH 12/16] apparently I had an anyuerism --- tests/storage/test_user_filters.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/storage/test_user_filters.py b/tests/storage/test_user_filters.py index 457a8113105d..b8e7eb621bd5 100644 --- a/tests/storage/test_user_filters.py +++ b/tests/storage/test_user_filters.py @@ -23,7 +23,7 @@ from tests import unittest -class ProfileStoreTestCase(unittest.HomeserverTestCase): +class UserFiltersStoreTestCase(unittest.HomeserverTestCase): """ Test background migration that copies entries from column user_id to full_user_id, adding the hostname in the process. From b8c3cf268b56e191a8a5d07d240d12c016423e37 Mon Sep 17 00:00:00 2001 From: Shay Date: Thu, 11 May 2023 18:44:33 -0700 Subject: [PATCH 13/16] Update tests/storage/test_user_filters.py Co-authored-by: David Robertson --- tests/storage/test_user_filters.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/storage/test_user_filters.py b/tests/storage/test_user_filters.py index b8e7eb621bd5..89c0f82211e8 100644 --- a/tests/storage/test_user_filters.py +++ b/tests/storage/test_user_filters.py @@ -13,7 +13,7 @@ # limitations under the License. -from twisted.internet.testing import MemoryReactor +from twisted.test.proto_helpers import MemoryReactor from synapse.server import HomeServer from synapse.storage.database import LoggingTransaction From 1f3bac8ad01a44068a1ad7ed50c14dd366069795 Mon Sep 17 00:00:00 2001 From: Shay Date: Thu, 11 May 2023 20:05:48 -0700 Subject: [PATCH 14/16] Update test_user_filters.py --- tests/storage/test_user_filters.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/storage/test_user_filters.py b/tests/storage/test_user_filters.py index 89c0f82211e8..8f3d8fe4e897 100644 --- a/tests/storage/test_user_filters.py +++ b/tests/storage/test_user_filters.py @@ -32,7 +32,7 @@ class UserFiltersStoreTestCase(unittest.HomeserverTestCase): def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: self.store = hs.get_datastores().main - def test_bg_migration2(self) -> None: + def test_bg_migration(self) -> None: updater = self.hs.get_datastores().main.db_pool.updates # drop the constraint so we can insert nulls in full_user_id to populate the test From d75462eda21fc91a5b80156819dfcf716f1b0501 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Tue, 16 May 2023 09:47:27 -0700 Subject: [PATCH 15/16] simplify tests --- tests/storage/test_profile.py | 11 ++++------- tests/storage/test_user_filters.py | 9 +++------ 2 files changed, 7 insertions(+), 13 deletions(-) diff --git a/tests/storage/test_profile.py b/tests/storage/test_profile.py index d5afcb714012..a924e2ab7e3c 100644 --- a/tests/storage/test_profile.py +++ b/tests/storage/test_profile.py @@ -93,7 +93,7 @@ def f(txn: LoggingTransaction) -> None: self.get_success( self.store.db_pool.simple_insert( "profiles", - {"user_id": f"hello{i}"}, + {"user_id": f"hello{i:02}"}, ) ) @@ -123,13 +123,10 @@ def f(txn: LoggingTransaction) -> None: expected_values = [] for i in range(0, 70): - expected_values.append((f"@hello{i}:{self.hs.hostname}",)) + expected_values.append((f"@hello{i:02}:{self.hs.hostname}",)) res = self.get_success( - self.store.db_pool.execute("", None, "SELECT full_user_id from profiles") + self.store.db_pool.execute("", None, "SELECT full_user_id from profiles ORDER BY full_user_id") ) self.assertEqual(len(res), len(expected_values)) - for value in res: - self.assertEqual(True, value in expected_values) - for value in expected_values: - self.assertEqual(True, value in res) + self.assertEqual(res, expected_values) diff --git a/tests/storage/test_user_filters.py b/tests/storage/test_user_filters.py index 8f3d8fe4e897..bab802f56ec6 100644 --- a/tests/storage/test_user_filters.py +++ b/tests/storage/test_user_filters.py @@ -50,7 +50,7 @@ def f(txn: LoggingTransaction) -> None: self.store.db_pool.simple_insert( "user_filters", { - "user_id": f"hello{i}", + "user_id": f"hello{i:02}", "filter_id": i, "filter_json": bytearray(i), }, @@ -83,7 +83,7 @@ def f(txn: LoggingTransaction) -> None: expected_values = [] for i in range(0, 70): - expected_values.append((f"@hello{i}:{self.hs.hostname}",)) + expected_values.append((f"@hello{i:02}:{self.hs.hostname}",)) res = self.get_success( self.store.db_pool.execute( @@ -91,7 +91,4 @@ def f(txn: LoggingTransaction) -> None: ) ) self.assertEqual(len(res), len(expected_values)) - for value in res: - self.assertEqual(True, value in expected_values) - for value in expected_values: - self.assertEqual(True, value in res) + self.assertEqual(res, expected_values) From 0c1d08513239bd4ec00a058e4396d91a43895ed7 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Tue, 16 May 2023 10:08:14 -0700 Subject: [PATCH 16/16] lint --- tests/storage/test_profile.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/storage/test_profile.py b/tests/storage/test_profile.py index a924e2ab7e3c..f9cf0fcb82ed 100644 --- a/tests/storage/test_profile.py +++ b/tests/storage/test_profile.py @@ -126,7 +126,9 @@ def f(txn: LoggingTransaction) -> None: expected_values.append((f"@hello{i:02}:{self.hs.hostname}",)) res = self.get_success( - self.store.db_pool.execute("", None, "SELECT full_user_id from profiles ORDER BY full_user_id") + self.store.db_pool.execute( + "", None, "SELECT full_user_id from profiles ORDER BY full_user_id" + ) ) self.assertEqual(len(res), len(expected_values)) self.assertEqual(res, expected_values)