-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Remove option to skip locking of tables during emulated upserts #14469
Conversation
To perform an emulated upsert into a table safely, we must either: * lock the table, * be the only writer upserting into the table * or rely on another unique index being present. When the 2nd or 3rd cases were applicable, we previously avoided locking the table as an optimization. However, as seen in #14406, it is easy to slip up when adding new schema deltas and corrupt the database. Since #13760, Synapse has required SQLite >= 3.27.0, which has support for native upserts. This means that we now only perform emulated upserts while waiting for background updates to add unique indexes. Since emulated upserts are far less frequent now, let's remove the option to skip locking tables, so that we don't shoot ourselves in the foot again. Signed-off-by: Sean Quah <seanq@matrix.org>
Signed-off-by: Sean Quah <seanq@matrix.org>
9c14ad6
to
2836401
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went through the cases affected to see if any of them might use an emulated upsert. (I.e. would any of these upserts start locking tables after this change, when they wouldn't have before?)
AFAICS I don't think that's true of any of these cases. (I do find all of this quite confusing, so I wouldn't mind a sanity check of my reasoning.)
synapse/storage/database.py
Outdated
If there is no such index yet[*], we can "emulate" an upsert with a SELECT | ||
followed by either an INSERT or an UPDATE. This is unsafe: we cannot make the | ||
same atomicity guarantees that a native upsert can and are very vulnerable to | ||
races and crashes. Therefore if we wish to upsert without an appropriate unique | ||
index, we must acquire a table-level lock before the emulated upsert. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we cannot make the same atomicity guarantees that a native upsert can
I wonder if my phrasing was poor here. Maybe we can make those guarantees if we're using a sufficiently high isolation level?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to reword it a little. Suggestions welcome.
table="user_directory_search", | ||
keyvalues={"user_id": user_id}, | ||
values={"value": value}, | ||
lock=False, # We're only inserter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unique index exists here for postgres:
CREATE UNIQUE INDEX user_directory_search_user_idx ON user_directory_search(user_id); |
... but not for sqlite. It's marked as unsafe to upsert into on sqlite here:
synapse/synapse/storage/database.py
Lines 534 to 538 in c3a4780
# We add the user_directory_search table to the blacklist on SQLite | |
# because the existing search table does not have an index, making it | |
# unsafe to use native upserts. | |
if isinstance(self.engine, Sqlite3Engine): | |
self._unsafe_to_upsert_tables.add("user_directory_search") |
Does that mean that with this change we're now going to start locking user_directory_search
on sqlite deployments? Ahh, but locking a table is a no-op on sqlite, presumably because there's at most one worker writing?
synapse/synapse/storage/engines/sqlite.py
Lines 103 to 104 in 8c94dd3
def lock_table(self, txn: Cursor, table: str) -> None: | |
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good spot with user_directory_search
. I wasn't expecting that. Thankfully the no-op locking saves us.
But if locking on sqlite has never done anything, then the rationale for lock=False
cannot have been because sqlite had no native upserts. Which means it must have been added as an optimization for the background update case on postgres?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if locking on sqlite has never done anything, then the rationale for lock=False cannot have been because sqlite had no native upserts.
Oops, sorry. That's probably me jumping to conclusions.
Which means it must have been added as an optimization for the background update case on postgres?
I suppose so! But to check: I don't think that suddenly makes it unsafe to remove the footgun?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, it's not unsafe, it just means this PR is rolling back a previous optimization we decided to add.
Thanks for checking all the upserts. I hadn't realised that we always do an emulated upsert on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Sean. I think this LGTM. It'd be good to check with someone else on the team though---given that that this was my suggestion in the first place, I'd like to be reassured that we haven't missed any perf implications here. @erikjohnston perhaps?
Returns: | ||
Returns True if a row was inserted or updated (i.e. if `values` is | ||
not empty then this always returns True) | ||
""" | ||
insertion_values = insertion_values or {} | ||
|
||
# We need to lock the table :(, unless we're *really* careful | ||
if lock: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we keeping this one conditional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's to support simple_upsert_many_txn_emulated
, which does its own locking beforehand.
synapse/synapse/storage/database.py
Lines 1499 to 1509 in 9cae44f
if lock: | |
# Lock the table just once, to prevent it being done once per row. | |
# Note that, according to Postgres' documentation, once obtained, | |
# the lock is held for the remainder of the current transaction. | |
self.engine.lock_table(txn, "user_ips") | |
for keyv, valv in zip(key_values, value_values): | |
_keys = {x: y for x, y in zip(key_names, keyv)} | |
_vals = {x: y for x, y in zip(value_names, valv)} | |
self.simple_upsert_txn_emulated(txn, table, _keys, _vals, lock=False) |
To perform an emulated upsert into a table safely, we must either: * lock the table, * be the only writer upserting into the table * or rely on another unique index being present. When the 2nd or 3rd cases were applicable, we previously avoided locking the table as an optimization. However, as seen in #14406, it is easy to slip up when adding new schema deltas and corrupt the database. The only time we lock when performing emulated upserts is while waiting for background updates on postgres. On sqlite, we do no locking at all. Let's remove the option to skip locking tables, so that we don't shoot ourselves in the foot again. Signed-off-by: Sean Quah <seanq@matrix.org>
Synapse 1.73.0 (2022-12-06) =========================== Please note that legacy Prometheus metric names have been removed in this release; see [the upgrade notes](https://github.com/matrix-org/synapse/blob/release-v1.73/docs/upgrade.md#legacy-prometheus-metric-names-have-now-been-removed) for more details. No significant changes since 1.73.0rc2. Synapse 1.73.0rc2 (2022-12-01) ============================== Bugfixes -------- - Fix a regression in Synapse 1.73.0rc1 where Synapse's main process would stop responding to HTTP requests when a user with a large number of devices logs in. ([\matrix-org#14582](matrix-org#14582)) Synapse 1.73.0rc1 (2022-11-29) ============================== Features -------- - Speed-up `/messages` with `filter_events_for_client` optimizations. ([\matrix-org#14527](matrix-org#14527)) - Improve DB performance by reducing amount of data that gets read in `device_lists_changes_in_room`. ([\matrix-org#14534](matrix-org#14534)) - Adds support for handling avatar in SSO OIDC login. Contributed by @ashfame. ([\matrix-org#13917](matrix-org#13917)) - Move MSC3030 `/timestamp_to_event` endpoints to stable `v1` location (`/_matrix/client/v1/rooms/<roomID>/timestamp_to_event?ts=<timestamp>&dir=<direction>`, `/_matrix/federation/v1/timestamp_to_event/<roomID>?ts=<timestamp>&dir=<direction>`). ([\matrix-org#14471](matrix-org#14471)) - Reduce database load of [Client-Server endpoints](https://spec.matrix.org/v1.5/client-server-api/#aggregations) which return bundled aggregations. ([\matrix-org#14491](matrix-org#14491), [\matrix-org#14508](matrix-org#14508), [\matrix-org#14510](matrix-org#14510)) - Add unstable support for an Extensible Events room version (`org.matrix.msc1767.10`) via [MSC1767](matrix-org/matrix-spec-proposals#1767), [MSC3931](matrix-org/matrix-spec-proposals#3931), [MSC3932](matrix-org/matrix-spec-proposals#3932), and [MSC3933](matrix-org/matrix-spec-proposals#3933). ([\matrix-org#14520](matrix-org#14520), [\matrix-org#14521](matrix-org#14521), [\matrix-org#14524](matrix-org#14524)) - Prune user's old devices on login if they have too many. ([\matrix-org#14038](matrix-org#14038), [\matrix-org#14580](matrix-org#14580)) Bugfixes -------- - Fix a long-standing bug where paginating from the start of a room did not work. Contributed by @gnunicorn. ([\matrix-org#14149](matrix-org#14149)) - Fix a bug introduced in Synapse 1.58.0 where a user with presence state `org.matrix.msc3026.busy` would mistakenly be set to `online` when calling `/sync` or `/events` on a worker process. ([\matrix-org#14393](matrix-org#14393)) - Fix a bug introduced in Synapse 1.70.0 where a receipt's thread ID was not sent over federation. ([\matrix-org#14466](matrix-org#14466)) - Fix a long-standing bug where the [List media admin API](https://matrix-org.github.io/synapse/latest/admin_api/media_admin_api.html#list-all-media-in-a-room) would fail when processing an image with broken thumbnail information. ([\matrix-org#14537](matrix-org#14537)) - Fix a bug introduced in Synapse 1.67.0 where two logging context warnings would be logged on startup. ([\matrix-org#14574](matrix-org#14574)) - In application service transactions that include the experimental `org.matrix.msc3202.device_one_time_key_counts` key, include a duplicate key of `org.matrix.msc3202.device_one_time_keys_count` to match the name proposed by [MSC3202](matrix-org/matrix-spec-proposals#3202). ([\matrix-org#14565](matrix-org#14565)) - Fix a bug introduced in Synapse 0.9 where Synapse would fail to fetch server keys whose IDs contain a forward slash. ([\matrix-org#14490](matrix-org#14490)) Improved Documentation ---------------------- - Fixed link to 'Synapse administration endpoints'. ([\matrix-org#14499](matrix-org#14499)) Deprecations and Removals ------------------------- - Remove legacy Prometheus metrics names. They were deprecated in Synapse v1.69.0 and disabled by default in Synapse v1.71.0. ([\matrix-org#14538](matrix-org#14538)) Internal Changes ---------------- - Improve type hinting throughout Synapse. ([\matrix-org#14055](matrix-org#14055), [\matrix-org#14412](matrix-org#14412), [\matrix-org#14529](matrix-org#14529), [\matrix-org#14452](matrix-org#14452)). - Remove old stream ID tracking code. Contributed by Nick @beeper (@Fizzadar). ([\matrix-org#14376](matrix-org#14376), [\matrix-org#14468](matrix-org#14468)) - Remove the `worker_main_http_uri` configuration setting. This is now handled via internal replication. ([\matrix-org#14400](matrix-org#14400), [\matrix-org#14476](matrix-org#14476)) - Refactor `federation_sender` and `pusher` configuration loading. ([\matrix-org#14496](matrix-org#14496)) ([\matrix-org#14509](matrix-org#14509), [\matrix-org#14573](matrix-org#14573)) - Faster joins: do not wait for full state when creating events to send. ([\matrix-org#14403](matrix-org#14403)) - Faster joins: filter out non local events when a room doesn't have its full state. ([\matrix-org#14404](matrix-org#14404)) - Faster joins: send events to initial list of servers if we don't have the full state yet. ([\matrix-org#14408](matrix-org#14408)) - Faster joins: use servers list approximation received during `send_join` (potentially updated with received membership events) in `assert_host_in_room`. ([\matrix-org#14515](matrix-org#14515)) - Fix type logic in TCP replication code that prevented correctly ignoring blank commands. ([\matrix-org#14449](matrix-org#14449)) - Remove option to skip locking of tables when performing emulated upserts, to avoid a class of bugs in future. ([\matrix-org#14469](matrix-org#14469)) - `scripts-dev/federation_client`: Fix routing on servers with `.well-known` files. ([\matrix-org#14479](matrix-org#14479)) - Reduce default third party invite rate limit to 216 invites per day. ([\matrix-org#14487](matrix-org#14487)) - Refactor conversion of device list changes in room to outbound pokes to track unconverted rows using a `(stream ID, room ID)` position instead of updating the `converted_to_destinations` flag on every row. ([\matrix-org#14516](matrix-org#14516)) - Add more prompts to the bug report form. ([\matrix-org#14522](matrix-org#14522)) - Extend editorconfig rules on indent and line length to `.pyi` files. ([\matrix-org#14526](matrix-org#14526)) - Run Rust CI when `Cargo.lock` changes. This is particularly useful for dependabot updates. ([\matrix-org#14571](matrix-org#14571)) - Fix a possible variable shadow in `create_new_client_event`. ([\matrix-org#14575](matrix-org#14575)) - Bump various dependencies in the `poetry.lock` file and in CI scripts. ([\matrix-org#14557](matrix-org#14557), [\matrix-org#14559](matrix-org#14559), [\matrix-org#14560](matrix-org#14560), [\matrix-org#14500](matrix-org#14500), [\matrix-org#14501](matrix-org#14501), [\matrix-org#14502](matrix-org#14502), [\matrix-org#14503](matrix-org#14503), [\matrix-org#14504](matrix-org#14504), [\matrix-org#14505](matrix-org#14505)). # -----BEGIN PGP SIGNATURE----- # # iQIzBAABCgAdFiEE8SRSDO7gYkSP4chELS76LzL74EcFAmOPLnYACgkQLS76LzL7 # 4Edwpg/+KXpg2ZdiJ0Yaly9VHVeiqdHRi5D7WPS6n8YBsdRx9EQHzOBkD5HAW8hE # oz0c+zDS01ORlEWD825NYXjgaE1ijtZFvGxsftYTVuTYlVRR2m+r9jhDv9pVHT53 # TKtQVKpG0IUsuyukRBrweDcEeO0MA0nGpvaaQUhmftzWgy4yD3AjZyIgx0Ckg8pg # OwgrzGqA7FQs4MEeOxmk1H39fZg4dlo4nmI4whvAodgaGeS9sU8t+3Qj4PVod8v/ # AkVesJcruaTHuVMb+Xp8JKezb09SsIR94gmHalC5sL+41+6XAy9BtQ/cRDfCReG3 # U1I1x1h1+EQjTP6XzMmjQHLbfI2gUJBC4I2p3e2gZ4cMm9rVz94R1dBiRk8ZgRIC # cJFD9BvaAtb2PSTvyFBoHsrrn/u12i8fYFWu4Z4rO6dOGI83dZHeZzVw4UsVeqIK # 5+njQwcwQsrwL3AKLjbbdqmbmhXcF6LchIK2L+NuuvdiOfvXvkO0bdjBryVEbMqB # IOtAAWzwYaoUwVucMbBtXt/EqQS7biGkbDxsL8CDvaBwM/JSsUWXBafsV1FmxF2A # q6KAeKpfelefoegosTYD0Md+l39xdF8Z19XaKV3GeHZEY+HE3RJXJm+Pa8SJ+IF8 # Y1od9cB/H+fYSsWCWj1OJNqTIAozh6f1Pe2nFuFDxdBwABXc/pg= # =IBEL # -----END PGP SIGNATURE----- # gpg: Signature made Tue Dec 6 11:58:46 2022 GMT # gpg: using RSA key F124520CEEE062448FE1C8442D2EFA2F32FBE047 # gpg: Can't check signature: No public key # Conflicts: # poetry.lock # synapse/push/bulk_push_rule_evaluator.py # synapse/storage/databases/main/account_data.py # synapse/storage/databases/main/receipts.py
Synapse 1.73.0 (2022-12-06) =========================== Please note that legacy Prometheus metric names have been removed in this release; see [the upgrade notes](https://github.com/matrix-org/synapse/blob/release-v1.73/docs/upgrade.md#legacy-prometheus-metric-names-have-now-been-removed) for more details. No significant changes since 1.73.0rc2. Synapse 1.73.0rc2 (2022-12-01) ============================== Bugfixes -------- - Fix a regression in Synapse 1.73.0rc1 where Synapse's main process would stop responding to HTTP requests when a user with a large number of devices logs in. ([\#14582](matrix-org/synapse#14582)) Synapse 1.73.0rc1 (2022-11-29) ============================== Features -------- - Speed-up `/messages` with `filter_events_for_client` optimizations. ([\#14527](matrix-org/synapse#14527)) - Improve DB performance by reducing amount of data that gets read in `device_lists_changes_in_room`. ([\#14534](matrix-org/synapse#14534)) - Adds support for handling avatar in SSO OIDC login. Contributed by @ashfame. ([\#13917](matrix-org/synapse#13917)) - Move MSC3030 `/timestamp_to_event` endpoints to stable `v1` location (`/_matrix/client/v1/rooms/<roomID>/timestamp_to_event?ts=<timestamp>&dir=<direction>`, `/_matrix/federation/v1/timestamp_to_event/<roomID>?ts=<timestamp>&dir=<direction>`). ([\#14471](matrix-org/synapse#14471)) - Reduce database load of [Client-Server endpoints](https://spec.matrix.org/v1.5/client-server-api/#aggregations) which return bundled aggregations. ([\#14491](matrix-org/synapse#14491), [\#14508](matrix-org/synapse#14508), [\#14510](matrix-org/synapse#14510)) - Add unstable support for an Extensible Events room version (`org.matrix.msc1767.10`) via [MSC1767](matrix-org/matrix-spec-proposals#1767), [MSC3931](matrix-org/matrix-spec-proposals#3931), [MSC3932](matrix-org/matrix-spec-proposals#3932), and [MSC3933](matrix-org/matrix-spec-proposals#3933). ([\#14520](matrix-org/synapse#14520), [\#14521](matrix-org/synapse#14521), [\#14524](matrix-org/synapse#14524)) - Prune user's old devices on login if they have too many. ([\#14038](matrix-org/synapse#14038), [\#14580](matrix-org/synapse#14580)) Bugfixes -------- - Fix a long-standing bug where paginating from the start of a room did not work. Contributed by @gnunicorn. ([\#14149](matrix-org/synapse#14149)) - Fix a bug introduced in Synapse 1.58.0 where a user with presence state `org.matrix.msc3026.busy` would mistakenly be set to `online` when calling `/sync` or `/events` on a worker process. ([\#14393](matrix-org/synapse#14393)) - Fix a bug introduced in Synapse 1.70.0 where a receipt's thread ID was not sent over federation. ([\#14466](matrix-org/synapse#14466)) - Fix a long-standing bug where the [List media admin API](https://matrix-org.github.io/synapse/latest/admin_api/media_admin_api.html#list-all-media-in-a-room) would fail when processing an image with broken thumbnail information. ([\#14537](matrix-org/synapse#14537)) - Fix a bug introduced in Synapse 1.67.0 where two logging context warnings would be logged on startup. ([\#14574](matrix-org/synapse#14574)) - In application service transactions that include the experimental `org.matrix.msc3202.device_one_time_key_counts` key, include a duplicate key of `org.matrix.msc3202.device_one_time_keys_count` to match the name proposed by [MSC3202](matrix-org/matrix-spec-proposals#3202). ([\#14565](matrix-org/synapse#14565)) - Fix a bug introduced in Synapse 0.9 where Synapse would fail to fetch server keys whose IDs contain a forward slash. ([\#14490](matrix-org/synapse#14490)) Improved Documentation ---------------------- - Fixed link to 'Synapse administration endpoints'. ([\#14499](matrix-org/synapse#14499)) Deprecations and Removals ------------------------- - Remove legacy Prometheus metrics names. They were deprecated in Synapse v1.69.0 and disabled by default in Synapse v1.71.0. ([\#14538](matrix-org/synapse#14538)) Internal Changes ---------------- - Improve type hinting throughout Synapse. ([\#14055](matrix-org/synapse#14055), [\#14412](matrix-org/synapse#14412), [\#14529](matrix-org/synapse#14529), [\#14452](matrix-org/synapse#14452)). - Remove old stream ID tracking code. Contributed by Nick @beeper (@Fizzadar). ([\#14376](matrix-org/synapse#14376), [\#14468](matrix-org/synapse#14468)) - Remove the `worker_main_http_uri` configuration setting. This is now handled via internal replication. ([\#14400](matrix-org/synapse#14400), [\#14476](matrix-org/synapse#14476)) - Refactor `federation_sender` and `pusher` configuration loading. ([\#14496](matrix-org/synapse#14496)) ([\#14509](matrix-org/synapse#14509), [\#14573](matrix-org/synapse#14573)) - Faster joins: do not wait for full state when creating events to send. ([\#14403](matrix-org/synapse#14403)) - Faster joins: filter out non local events when a room doesn't have its full state. ([\#14404](matrix-org/synapse#14404)) - Faster joins: send events to initial list of servers if we don't have the full state yet. ([\#14408](matrix-org/synapse#14408)) - Faster joins: use servers list approximation received during `send_join` (potentially updated with received membership events) in `assert_host_in_room`. ([\#14515](matrix-org/synapse#14515)) - Fix type logic in TCP replication code that prevented correctly ignoring blank commands. ([\#14449](matrix-org/synapse#14449)) - Remove option to skip locking of tables when performing emulated upserts, to avoid a class of bugs in future. ([\#14469](matrix-org/synapse#14469)) - `scripts-dev/federation_client`: Fix routing on servers with `.well-known` files. ([\#14479](matrix-org/synapse#14479)) - Reduce default third party invite rate limit to 216 invites per day. ([\#14487](matrix-org/synapse#14487)) - Refactor conversion of device list changes in room to outbound pokes to track unconverted rows using a `(stream ID, room ID)` position instead of updating the `converted_to_destinations` flag on every row. ([\#14516](matrix-org/synapse#14516)) - Add more prompts to the bug report form. ([\#14522](matrix-org/synapse#14522)) - Extend editorconfig rules on indent and line length to `.pyi` files. ([\#14526](matrix-org/synapse#14526)) - Run Rust CI when `Cargo.lock` changes. This is particularly useful for dependabot updates. ([\#14571](matrix-org/synapse#14571)) - Fix a possible variable shadow in `create_new_client_event`. ([\#14575](matrix-org/synapse#14575)) - Bump various dependencies in the `poetry.lock` file and in CI scripts. ([\#14557](matrix-org/synapse#14557), [\#14559](matrix-org/synapse#14559), [\#14560](matrix-org/synapse#14560), [\#14500](matrix-org/synapse#14500), [\#14501](matrix-org/synapse#14501), [\#14502](matrix-org/synapse#14502), [\#14503](matrix-org/synapse#14503), [\#14504](matrix-org/synapse#14504), [\#14505](matrix-org/synapse#14505)).
To perform an emulated upsert into a table safely, we must either:
When the 2nd or 3rd cases were applicable, we previously avoided locking
the table as an optimization. However, as seen in #14406, it is easy to
slip up when adding new schema deltas and corrupt the database.
The only time we lock when performing emulated upserts is while waiting
for background updates on postgres. On sqlite, we do no locking at all.
Let's remove the option to skip locking tables, so that we don't shoot
ourselves in the foot again.
Signed-off-by: Sean Quah seanq@matrix.org
Originally suggested by @DMRobertson.