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

Define SQLite compatibility policy #12983

Closed
richvdh opened this issue Jun 8, 2022 · 20 comments
Closed

Define SQLite compatibility policy #12983

richvdh opened this issue Jun 8, 2022 · 20 comments
Assignees
Labels
A-Database DB stuff like queries, migrations, new/remove columns, indexes, unexpected entries in the db A-SQLite Database issues specific to SQLite T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks.

Comments

@richvdh
Copy link
Member

richvdh commented Jun 8, 2022

We don't appear to have a defined compatibility policy for SQLite. https://matrix-org.github.io/synapse/latest/deprecation_policy.html#deprecation-policy-for-platform-dependencies doesn't mention it.

I don't think we can apply the "upstream support life cycle" for SQLite, since AFAIK the upstream SQLite developers do not provide bug fixes for anything except the most recent minor version. (Currently, that is 3.38.5, so we would require SQLite 3.38.x, which is clearly impractical.)

Typically, your SQLite version is determined by your distribution. (The sqlite3 python module is part of the stdlib, which is provided by the distribution, and cannot be upgraded via a virtualenv. Normally the _sqlite3.so is dynamically linked against the distribution's libsqlite3.)

Hence, "which version of SQLite do we need to support" tends to be defined by "which distributions do we aim to support", which itself doesn't have a clear answer (normally: "anything that has a sufficiently modern python".)

@richvdh
Copy link
Member Author

richvdh commented Jun 8, 2022

For a long time, our minimum sqlite version was 3.7, for compatibility with CentOS 7 - and AFAIK, we have never explicitly announced that we're going to drop compatibility with that distribution.

However, I think it would be entirely reasonable to do so, and I strongly suspect that we've already done so in practice.

Another historical constraint was Ubuntu 18.04 LTS, which has 3.22.0, but also has an unsupported Python version (3.6.5).

@richvdh
Copy link
Member Author

richvdh commented Jun 8, 2022

In terms of what is tested in CI, we run sytest on buster (which has sqlite 3.27), but only in postgres mode 🤦. Our oldest platform where we use sqlite is (for both trial and sytest) Ubuntu Focal 20.04LTS, which has SQLite 3.31, so there's a strong argument that that is the oldest version we support in practice.

A survey of distros named in https://matrix-org.github.io/synapse/latest/setup/installation.html#installing-synapse:

  • Arch has 3.38.5
  • Debian 10 (Buster, oldstable) has 3.27.2
  • Debian 11 (Bullseye, stable) has 3.34.1
  • Fedora 35 has 3.36
  • FreeBSD 12 has 3.37.2
  • NetBSD 8.2 has 3.35.2
  • openSUSE Leap 15.3 has 3.28.0.
  • Ubuntu 18.04LTS has 3.22.0, but also an unsupported python version.
  • Ubuntu 22.04LTS has 3.31.1.

(I've not checked that their python distros are dynamically linked against libsqlite: it's possible they are statically linked against a different version.)

Accordingly I think it would be entirely reasonable to declare 3.27.2 to be our minimum requirement. However, I'd love for us to define this in a less backwards way, somehow.

@DMRobertson
Copy link
Contributor

However, I'd love for us to define this in a less backwards way, somehow.

FWIW my first thought was "we support the version of sqlite in Debian stable", and rely on Debian's conservativeness to ensure we don't screw people over. I think that's consistent with the fact that we produce Debs but not e.g. RPMs for Fedora et al.

@richvdh
Copy link
Member Author

richvdh commented Jun 8, 2022

FWIW my first thought was "we support the version of sqlite in Debian stable"

... which implies we don't support oldstable? That's kinda ok once stable has been, uh, stable for a while, but gives a bit of a sharp cutoff at debian release time.

I think that's consistent with the fact that we produce Debs but not e.g. RPMs for Fedora et al.

It's worth noting that we do produce debs for oldstable (currently, at least; it's defined more by "does it have a recent enough python" than anything more explicit).

"We support the version of sqlite in Debian oldstable" sounds like a not-terrible definition.

@DMRobertson
Copy link
Contributor

Oldstable rather than stable, yes, sorry.

@erikjohnston
Copy link
Member

We can also check the phone home stats to see what SQLite versions are currently in use: https://grafana.matrix.org/d/7A0xoAMnk/homeserver-versions?orgId=1&from=now-7d&to=now&viewPanel=6, we could also filter that by Synapse version. It currently shows a small minority on 3.22.0 and earlier.

Though another thing to note here is that we don't heavily recommend for production use that you use SQLite, so I think we might get away with being a bit stricter/looser (depending on how you view things) with our compatibility policy for SQLite

@richvdh
Copy link
Member Author

richvdh commented Jun 8, 2022

oh HELLO: https://github.com/matrix-org/synapse/pull/9766/files#diff-c8a83d17fa29e88528380dc6514c58da23572f93dab57bfc3fbdb9adca7fbd1fR71

So, our current minimum version turns out to be 3.22, though I don't think we've tested with that since #11683, which moved our CI from bionic to focal.

I think we should bump this to 3.27, and try and actually test with that version. (One big advantage is that SQLite 3.23 fixes the "FALSE is truthy" footgun.)

@erikjohnston
Copy link
Member

Plus it lets us use native upserts:

@property
def can_native_upsert(self) -> bool:
"""
Do we support native UPSERTs? This requires SQLite3 3.24+, plus some
more work we haven't done yet to tell what was inserted vs updated.
"""
return sqlite3.sqlite_version_info >= (3, 24, 0)

@richvdh
Copy link
Member Author

richvdh commented Jun 9, 2022

"We support the version of sqlite in Debian oldstable" sounds like a not-terrible definition.

As a point of reference, that would have meant dropping support for Ubuntu 18.04 rather earlier than we did. Following the suggested scheme, we would have supported SQLite 3.16.2 (as used in Debian 9/Stretch) until Bullseye was released in August 2021, at which point the constraint would have been 3.27 (as used in Debian 10/Buster), which postdates Ubuntu 18.04. As it was, we dropped support for Ubuntu 18.04 when Python 3.6 went out of support in January 2022.

A release date for Bookworm has not yet been set, but we can expect a similar problem then: it is likely to happen in Summer 2023, at which point our supported SQLite version would become 3.34 (as used by Bullseye). That is insufficient for Ubuntu 20.04, which uses Python 3.8, so would otherwise go out of support in October 2024.

We're not duty-bound to support 3-year-old Ubuntu releases, and as Erik notes: if you care about this stuff, you probably ought to be on Postgres anyway. But I wanted to record that "We support the version of sqlite in Debian oldstable", while sounding generous, is actually a rather tighter constraint than we have worked to in the past.

@reivilibre reivilibre added the T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks. label Jun 13, 2022
@MadLittleMods MadLittleMods added the A-Database DB stuff like queries, migrations, new/remove columns, indexes, unexpected entries in the db label Aug 31, 2022
@richvdh
Copy link
Member Author

richvdh commented Sep 1, 2022

Steps here:

@DMRobertson
Copy link
Contributor

  • remove can_native_upsert from the codebase

Being able to use sqlite upserts would mean that we don't have to insert lock values into rows in make_full_schema. See #6311 (comment) and #6394.

@DMRobertson DMRobertson self-assigned this Sep 6, 2022
@DMRobertson DMRobertson added the A-SQLite Database issues specific to SQLite label Sep 6, 2022
@DMRobertson
Copy link
Contributor

DMRobertson commented Sep 6, 2022

https://repology.org/project/sqlite/versions

  • Fedora 35: 3.36.0
  • Debian 10 (Buster, current oldstable): 3.27.2
  • Ubuntu 20.04 (Focal): 3.31.1. Older LTS versions exist but we don't support them.
  • Arch BTW: 3.39.2
  • Gentoo: 3.39.2
  • OpenBSD: 3.39.2
  • FreeBSD: 3.39.2
  • NetBSD/pkgsrc: 3.39.2
  • Alpine 3.13: 3.34.1 Older versions exist but are EOL
    I think that's representative enough.

@gdt
Copy link

gdt commented Sep 6, 2022

While pkgsrc has recent sqlite3 as repology says, python sqlite support is linked against base system sqlite3 which is older. NetBSD 9 has 3.26.0, so if you can set that as the limit rather than 3.27.0, it saves a fair bit of pain for anyone using sqlite3 (even though they should be using postgresql anyway, which is what I do).

@erikjohnston
Copy link
Member

For native upserts we only need 3.24, so 3.26.0 seems plausible?

@DMRobertson
Copy link
Contributor

DMRobertson commented Sep 7, 2022

I'd be okay with 3.26, but according to https://github.com/NetBSD/pkgsrc/blob/a8c98c2eccd6f61de36bb2a2401fd25ca4dc19ac/chat/matrix-synapse/MESSAGE#L14-L16 pkgsrc expects you to use its version of sqlite. In which case I think I'd prefer to stick with 3.27: don't want to introduce a policy only to not follow it.

@gdt does that sound reasonable? (Sorry, I don't completely follow the details of pkgsrc works.)

@gdt
Copy link

gdt commented Sep 7, 2022

Yes, I had forgotten that there are other issues. This is what happens when something becomes important enough to be in the base system, and at the same time has way too many knobs becaused it's designed to be embedded one program at a time rather than as a system library :-(. I personally lean to "sqlite3 is not reasonable for significant workloads (volume or complexity), so just us pgsq". (I don't mean to criticize sqlite as "need sql for some program and don't want the user to have to think about it".)

It might also be good to specify, if it isn't really clear, which sqlite3 features need to be turned on. The issue is that sqlite does not have a single defined API/ABI, given the ability to build with and without various options and the apparent lack of a base specification of "this is sqlite3" vs "this is sqlite3 minus a standard feature". But, it might be just NetBSD base that doesn't have what is needed.

In general, though, I think it is best to only require what needs to be required, because there are always other environments, even if there isn't CI all the way back. It's extra effort when things work to have to patch a dependency expression (similar to the faux cryptography requirement).

@DMRobertson
Copy link
Contributor

It might also be good to specify, if it isn't really clear, which sqlite3 features need to be turned on.

Taking a very quick look at https://www.sqlite.org/compile.html#_options_to_enable_features_normally_turned_off , the only option I can see that we make use of is SQLITE_ENABLE_FTS4 for full text search, in these two places:

CREATE VIRTUAL TABLE user_directory_search
USING fts4 ( user_id, value );

"CREATE VIRTUAL TABLE event_search"
" USING fts4 ( event_id, room_id, sender, key, value )"

See also https://www.sqlite.org/fts3.html#compiling_and_enabling_fts3_and_fts4

In general, we're less interested in compile-time toggle-able features; we're more interested in new things we can query for and relevant bugfixes.

@DMRobertson
Copy link
Contributor

@gdt does that sound reasonable? (Sorry, I don't completely follow the details of pkgsrc works.)

Yes, I had forgotten that there are other issues. [...]

To be explicit: I understand this as "yes, this sounds reasonable" with additional commentary. (And commentary that I'm very sympathetic too at that!)

@gdt
Copy link

gdt commented Sep 7, 2022

Yes, I did mean to say that it is a reasonable approach given the situation.

And I see that that synapse is depending on a feature that sqlite documents as not enabled by default. But I can see how you got there...

@DMRobertson
Copy link
Contributor

Checklist in #12983 (comment) complete; closing.

Fizzadar added a commit to beeper/synapse-legacy-fork that referenced this issue Sep 15, 2022
Synapse 1.67.0 (2022-09-13)
===========================

This release removes using the deprecated direct TCP replication configuration
for workers. Server admins should use Redis instead. See the [upgrade
notes](https://matrix-org.github.io/synapse/v1.67/upgrade.html#upgrading-to-v1670).

The minimum version of `poetry` supported for managing source checkouts is now
1.2.0.

**Notice:** from the next major release (1.68.0) installing Synapse from a source
checkout will require a recent Rust compiler. Those using packages or
`pip install matrix-synapse` will not be affected. See the [upgrade
notes](https://matrix-org.github.io/synapse/v1.67/upgrade.html#upgrading-to-v1670).

**Notice:** from the next major release (1.68.0), running Synapse with a SQLite
database will require SQLite version 3.27.0 or higher. (The [current minimum
 version is SQLite 3.22.0](https://github.com/matrix-org/synapse/blob/release-v1.67/synapse/storage/engines/sqlite.py#L69-L78).)
See [matrix-org#12983](matrix-org#12983) and the [upgrade notes](https://matrix-org.github.io/synapse/v1.67/upgrade.html#upgrading-to-v1670) for more details.

No significant changes since 1.67.0rc1.

Synapse 1.67.0rc1 (2022-09-06)
==============================

Features
--------

- Support setting the registration shared secret in a file, via a new `registration_shared_secret_path` configuration option. ([\matrix-org#13614](matrix-org#13614))
- Change the default startup behaviour so that any missing "additional" configuration files (signing key, etc) are generated automatically. ([\matrix-org#13615](matrix-org#13615))
- Improve performance of sending messages in rooms with thousands of local users. ([\matrix-org#13634](matrix-org#13634))

Bugfixes
--------

- Fix a bug introduced in Synapse 1.13 where the [List Rooms admin API](https://matrix-org.github.io/synapse/develop/admin_api/rooms.html#list-room-api) would return integers instead of booleans for the `federatable` and `public` fields when using a Sqlite database. ([\matrix-org#13509](matrix-org#13509))
- Fix bug that user cannot `/forget` rooms after the last member has left the room. ([\matrix-org#13546](matrix-org#13546))
- Faster Room Joins: fix `/make_knock` blocking indefinitely when the room in question is a partial-stated room. ([\matrix-org#13583](matrix-org#13583))
- Fix loading the current stream position behind the actual position. ([\matrix-org#13585](matrix-org#13585))
- Fix a longstanding bug in `register_new_matrix_user` which meant it was always necessary to explicitly give a server URL. ([\matrix-org#13616](matrix-org#13616))
- Fix the running of [MSC1763](matrix-org/matrix-spec-proposals#1763) retention purge_jobs in deployments with background jobs running on a worker by forcing them back onto the main worker. Contributed by Brad @ Beeper. ([\matrix-org#13632](matrix-org#13632))
- Fix a long-standing bug that downloaded media for URL previews was not deleted while database background updates were running. ([\matrix-org#13657](matrix-org#13657))
- Fix [MSC3030](matrix-org/matrix-spec-proposals#3030) `/timestamp_to_event` endpoint to return the correct next event when the events have the same timestamp. ([\matrix-org#13658](matrix-org#13658))
- Fix bug where we wedge media plugins if clients disconnect early. Introduced in v1.22.0. ([\matrix-org#13660](matrix-org#13660))
- Fix a long-standing bug which meant that keys for unwhitelisted servers were not returned by `/_matrix/key/v2/query`. ([\matrix-org#13683](matrix-org#13683))
- Fix a bug introduced in Synapse v1.20.0 that would cause the unstable unread counts from [MSC2654](matrix-org/matrix-spec-proposals#2654) to be calculated even if the feature is disabled. ([\matrix-org#13694](matrix-org#13694))

Updates to the Docker image
---------------------------

- Update docker image to use a stable version of poetry. ([\matrix-org#13688](matrix-org#13688))

Improved Documentation
----------------------

- Improve the description of the ["chain cover index"](https://matrix-org.github.io/synapse/latest/auth_chain_difference_algorithm.html) used internally by Synapse. ([\matrix-org#13602](matrix-org#13602))
- Document how ["monthly active users"](https://matrix-org.github.io/synapse/latest/usage/administration/monthly_active_users.html) is calculated and used. ([\matrix-org#13617](matrix-org#13617))
- Improve documentation around user registration. ([\matrix-org#13640](matrix-org#13640))
- Remove documentation of legacy `frontend_proxy` worker app. ([\matrix-org#13645](matrix-org#13645))
- Clarify documentation that HTTP replication traffic can be protected with a shared secret. ([\matrix-org#13656](matrix-org#13656))
- Remove unintentional colons from [config manual](https://matrix-org.github.io/synapse/latest/usage/configuration/config_documentation.html) headers. ([\matrix-org#13665](matrix-org#13665))
- Update docs to make enabling metrics more clear. ([\matrix-org#13678](matrix-org#13678))
- Clarify `(room_id, event_id)` global uniqueness and how we should scope our database schemas. ([\matrix-org#13701](matrix-org#13701))

Deprecations and Removals
-------------------------

- Drop support for calling `/_matrix/client/v3/rooms/{roomId}/invite` without an `id_access_token`, which was not permitted by the spec. Contributed by @Vetchu. ([\matrix-org#13241](matrix-org#13241))
- Remove redundant `_get_joined_users_from_context` cache. Contributed by Nick @ Beeper (@Fizzadar). ([\matrix-org#13569](matrix-org#13569))
- Remove the ability to use direct TCP replication with workers. Direct TCP replication was deprecated in Synapse v1.18.0. Workers now require using Redis. ([\matrix-org#13647](matrix-org#13647))
- Remove support for unstable [private read receipts](matrix-org/matrix-spec-proposals#2285). ([\matrix-org#13653](matrix-org#13653), [\matrix-org#13692](matrix-org#13692))

Internal Changes
----------------

- Extend the release script to wait for GitHub Actions to finish and to be usable as a guide for the whole process. ([\matrix-org#13483](matrix-org#13483))
- Add experimental configuration option to allow disabling legacy Prometheus metric names. ([\matrix-org#13540](matrix-org#13540))
- Cache user IDs instead of profiles to reduce cache memory usage. Contributed by Nick @ Beeper (@Fizzadar). ([\matrix-org#13573](matrix-org#13573), [\matrix-org#13600](matrix-org#13600))
- Optimize how Synapse calculates domains to fetch from during backfill. ([\matrix-org#13575](matrix-org#13575))
- Comment about a better future where we can get the state diff between two events. ([\matrix-org#13586](matrix-org#13586))
- Instrument `_check_sigs_and_hash_and_fetch` to trace time spent in child concurrent calls for understandable traces in Jaeger. ([\matrix-org#13588](matrix-org#13588))
- Improve performance of `@cachedList`. ([\matrix-org#13591](matrix-org#13591))
- Minor speed up of fetching large numbers of push rules. ([\matrix-org#13592](matrix-org#13592))
- Optimise push action fetching queries. Contributed by Nick @ Beeper (@Fizzadar). ([\matrix-org#13597](matrix-org#13597))
- Rename `event_map` to `unpersisted_events` when computing the auth differences. ([\matrix-org#13603](matrix-org#13603))
- Refactor `get_users_in_room(room_id)` mis-use with dedicated `get_current_hosts_in_room(room_id)` function. ([\matrix-org#13605](matrix-org#13605))
- Use dedicated `get_local_users_in_room(room_id)` function to find local users when calculating `join_authorised_via_users_server` of a `/make_join` request. ([\matrix-org#13606](matrix-org#13606))
- Refactor `get_users_in_room(room_id)` mis-use to lookup single local user with dedicated `check_local_user_in_room(...)` function. ([\matrix-org#13608](matrix-org#13608))
- Drop unused column `application_services_state.last_txn`. ([\matrix-org#13627](matrix-org#13627))
- Improve readability of Complement CI logs by printing failure results last. ([\matrix-org#13639](matrix-org#13639))
- Generalise the `@cancellable` annotation so it can be used on functions other than just servlet methods. ([\matrix-org#13662](matrix-org#13662))
- Introduce a `CommonUsageMetrics` class to share some usage metrics between the Prometheus exporter and the phone home stats. ([\matrix-org#13671](matrix-org#13671))
- Add some logging to help track down matrix-org#13444. ([\matrix-org#13679](matrix-org#13679))
- Update poetry lock file for v1.2.0. ([\matrix-org#13689](matrix-org#13689))
- Add cache to `is_partial_state_room`. ([\matrix-org#13693](matrix-org#13693))
- Update the Grafana dashboard that is included with Synapse in the `contrib` directory. ([\matrix-org#13697](matrix-org#13697))
- Only run trial CI on all python versions on non-PRs. ([\matrix-org#13698](matrix-org#13698))
- Fix typechecking with latest types-jsonschema. ([\matrix-org#13712](matrix-org#13712))
- Reduce number of CI checks we run for PRs. ([\matrix-org#13713](matrix-org#13713))

# -----BEGIN PGP SIGNATURE-----
#
# iQFEBAABCgAuFiEEBTGR3/RnAzBGUif3pULk7RsPrAkFAmMgR2QQHGVyaWtAbWF0
# cml4Lm9yZwAKCRClQuTtGw+sCfG7B/94PwW1ChsaI8hkz/3e+93PEl/mNJ6YFaEB
# 5pP4Dh/0dipP/iKbpgNuj5xz/JFnIi8D49A8sKNnku3jk0/8AZHgqDiBgOkrN76z
# Y3awo5Q9ag4xww/105V3bhdnX1NrX8Avf6F2jchDv6/9q8wQHGBPg6DMgfZ/m/BL
# SB4dypbbNpgLykuwtWxx6YMUYH+trsXJOn/MoAqld3QcZsqkDR25wXCt9+Dr+6AT
# dPd/czi8kV8ruU59tf2K5HB7XKzBW9S3Qb3dJJmGOTTJ7ccUkN/XuTwqnII950Mo
# bSlMXjY2hqk8rKUNhGZpi9bqUkwNhMgOkZl9A0Y1XtsXx6yjy0T/
# =zSGi
# -----END PGP SIGNATURE-----
# gpg: Signature made Tue Sep 13 10:03:32 2022 BST
# gpg:                using RSA key 053191DFF4670330465227F7A542E4ED1B0FAC09
# gpg:                issuer "[email protected]"
# gpg: Can't check signature: No public key

# Conflicts:
#	synapse/config/experimental.py
#	synapse/push/bulk_push_rule_evaluator.py
#	synapse/storage/databases/main/event_push_actions.py
#	synapse/util/caches/descriptors.py
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Database DB stuff like queries, migrations, new/remove columns, indexes, unexpected entries in the db A-SQLite Database issues specific to SQLite T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks.
Projects
None yet
Development

No branches or pull requests

6 participants