From f28cd22980a38332613d8a75749c3d3672c5caca Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 3 Jul 2026 10:01:39 +0000 Subject: [PATCH] Let the test suite run against the Rust Postgres backend MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add SYNAPSE_TEST_RUST_DRIVER to run the Postgres test suite against the native Rust driver (sets use_rust_driver on the test database config, off by default). Teach the harness's make_fake_db_pool to build a RustConnectionPool for the Rust engine, with its worker thread pool swapped for the threadless test pool so queries run deterministically on the reactor during `pump` (the shim's block_on waits there for the tokio workers). Test-database create/drop is admin work, so it now goes directly over psycopg2 (always available in tests) rather than the engine module's `connect`. Running the suite this way surfaces a few real psycopg2-compatibility gaps: - `fixup_config_defaults` (was `fixup_default_host`) now also fills libpq's PGUSER / PGPASSWORD environment defaults, not just the host — tokio-postgres reads none of them from the environment, but Synapse's test setup relies on them (as psycopg2 does). - `build_dsn` skips `None`-valued args, matching psycopg2's handling of unset kwargs. With these, a homeserver boots on the Rust backend under the test harness and starts running real transactions and background updates. The remaining psycopg2 type-coercion / behaviour differences are addressed in the commits that follow; psycopg2 and sqlite runs are unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01W3G4M92AmwSSZCbmtMJU3d --- rust/src/database/postgres/mod.rs | 47 ++++++++++++++++--------- rust/src/database/postgres/pool.rs | 6 ++-- synapse/storage/rust_dbapi.py | 24 ++++++++++--- tests/server.py | 55 ++++++++++++++++++++++++++---- tests/storage/test_rust_dbapi.py | 16 +++++++++ tests/utils.py | 3 ++ 6 files changed, 121 insertions(+), 30 deletions(-) diff --git a/rust/src/database/postgres/mod.rs b/rust/src/database/postgres/mod.rs index d231dd1dd6..16a38b906b 100644 --- a/rust/src/database/postgres/mod.rs +++ b/rust/src/database/postgres/mod.rs @@ -39,27 +39,42 @@ pub fn register_module(py: Python<'_>, m: &Bound<'_, PyModule>) -> PyResult<()> } /// Fix up a DSN to ensure it has a host, using libpq's default host if -/// necessary. +/// necessary, and fill in libpq's environment defaults for the fields +/// [`tokio_postgres`] doesn't read from the environment. /// -/// [`tokio_postgres`] has a different default host than libpq, which is what -/// Synapse previously used (and is what e.g. `psql` uses). libpq's default host -/// is configurable, so when the DSN omits a host we ask libpq what its default -/// would be and use that instead (see [`libpq::default_host`]). -pub(crate) fn fixup_default_host(dsn: &str) -> Result { +/// [`tokio_postgres`] parses only the DSN string: unlike libpq (which is what +/// Synapse previously used, and what e.g. `psql` uses) it does not consult +/// `PGHOST` / `PGUSER` / `PGPASSWORD` or the compiled-in defaults. So when the +/// DSN omits one of these we fill it the way libpq would, so a config that +/// relied on those environment variables (as Synapse's test setup does) +/// connects the same way it did under psycopg2. +pub(crate) fn fixup_config_defaults(dsn: &str) -> Result { let mut config = dsn.parse::()?; - // `tokio_postgres` parses only the DSN string (it does not consult `PGHOST` - // or the compiled-in default), so an empty host list means the DSN really - // omitted the host. A DSN that gives a `hostaddr` instead of a `host` is - // still connectable as-is, so leave it alone too — injecting a default host - // there would just confuse TLS/SNI. - if !config.get_hosts().is_empty() || !config.get_hostaddrs().is_empty() { - return Ok(config); + // Host. A DSN that gives a `hostaddr` instead of a `host` is still + // connectable as-is, so leave it alone too — injecting a default host there + // would just confuse TLS/SNI. Otherwise resolve libpq's default host + // without connecting (see `libpq::default_host`). + if config.get_hosts().is_empty() && config.get_hostaddrs().is_empty() { + config.host(&libpq::default_host()?); } - // Resolve libpq's default host without connecting (see `libpq::default_host`). - let host = libpq::default_host()?; - config.host(&host); + // User: libpq falls back to `PGUSER`, then the OS user. + if config.get_user().is_none() { + if let Some(user) = std::env::var("PGUSER") + .ok() + .or_else(|| std::env::var("USER").ok()) + { + config.user(&user); + } + } + + // Password: libpq falls back to `PGPASSWORD`. + if config.get_password().is_none() { + if let Ok(password) = std::env::var("PGPASSWORD") { + config.password(&password); + } + } Ok(config) } diff --git a/rust/src/database/postgres/pool.rs b/rust/src/database/postgres/pool.rs index 6631d78361..8fb63ed865 100644 --- a/rust/src/database/postgres/pool.rs +++ b/rust/src/database/postgres/pool.rs @@ -4,7 +4,7 @@ //! We use the generic `deadpool::managed` pool with our own [`ConnectionManager`] //! rather than the `deadpool-postgres` crate, so that creating a connection //! reuses our own DSN handling (libpq's default host, see -//! [`super::fixup_default_host`]) and drives the connection task on the shared +//! [`super::fixup_config_defaults`]) and drives the connection task on the shared //! runtime. //! //! The pooled item is a plain [`tokio_postgres::Client`]. Rust-native code takes @@ -21,7 +21,7 @@ use tokio_postgres::{Client, Config, NoTls}; use crate::database::postgres::connection::Connection; use crate::database::postgres::errors::{pg_err_to_py, OperationalError}; -use crate::database::postgres::fixup_default_host; +use crate::database::postgres::fixup_config_defaults; use crate::database::postgres::helpers::BlockingPostgres; use crate::database::runtime::runtime; @@ -85,7 +85,7 @@ impl ConnectionManager { /// Build a manager from a libpq-style DSN and session settings. pub fn from_dsn(dsn: &str, session: SessionConfig) -> Result { Ok(Self { - config: fixup_default_host(dsn)?, + config: fixup_config_defaults(dsn)?, session, }) } diff --git a/synapse/storage/rust_dbapi.py b/synapse/storage/rust_dbapi.py index e384ee6d17..c0b3e31116 100644 --- a/synapse/storage/rust_dbapi.py +++ b/synapse/storage/rust_dbapi.py @@ -53,14 +53,30 @@ def _quote_dsn_value(value: Any) -> str: return s +# psycopg2 accepts a few connection kwargs that are *not* libpq keywords and +# translates them itself. The Rust pool instead parses a strict libpq DSN +# (via ``tokio_postgres``), which only knows the real keywords, so we map the +# aliases here. ``database`` -> ``dbname`` is the important one: Synapse's +# sample config and most real deployments spell the database name ``database``. +_PSYCOPG2_KEY_ALIASES = {"database": "dbname"} + + def build_dsn(params: Mapping[str, Any]) -> str: """Build a libpq keyword/value DSN from psycopg2-style connection kwargs. - Synapse's database `args` are libpq-compatible keywords (``dbname``, - ``user``, ``host``, ``port``, ``password``, …); the Rust pool takes a DSN - string rather than kwargs, so join them into one. + Synapse's database `args` are psycopg2 connection kwargs — mostly libpq + keywords (``user``, ``host``, ``port``, ``password``, …), but ``database`` + is a psycopg2 alias for libpq's ``dbname`` (see ``_PSYCOPG2_KEY_ALIASES``). + The Rust pool takes a strict libpq DSN string rather than kwargs, so join + them into one, mapping any aliases to their real keyword. ``None`` values + are skipped, matching psycopg2's handling of `None` kwargs (fall back to the + libpq default). """ - return " ".join(f"{key}={_quote_dsn_value(value)}" for key, value in params.items()) + return " ".join( + f"{_PSYCOPG2_KEY_ALIASES.get(key, key)}={_quote_dsn_value(value)}" + for key, value in params.items() + if value is not None + ) def connect( diff --git a/tests/server.py b/tests/server.py index 0d6643286c..ed6a82207d 100644 --- a/tests/server.py +++ b/tests/server.py @@ -112,6 +112,7 @@ from tests.utils import ( POSTGRES_USER, SQLITE_PERSIST_DB, USE_POSTGRES_FOR_TESTS, + USE_RUST_DRIVER_FOR_TESTS, default_config, ) @@ -746,6 +747,11 @@ def make_fake_db_pool( is a drop-in replacement for the normal `make_pool` which builds such a connection pool. """ + from synapse.storage.engines.postgres_rust import RustPostgresEngine + + if isinstance(engine, RustPostgresEngine): + return _make_fake_rust_db_pool(reactor, clock, db_config, engine, server_name) + pool = make_pool( reactor=reactor, clock=clock, @@ -787,6 +793,35 @@ def make_fake_db_pool( return pool +def _make_fake_rust_db_pool( + reactor: ISynapseReactor, + clock: "Clock", + db_config: DatabaseConnectionConfig, + engine: BaseDatabaseEngine, + server_name: str, +) -> Any: + """`make_fake_db_pool` for the native Rust backend. + + Builds a real `RustConnectionPool`, then swaps its worker thread pool for the + threadless test `ThreadPool` so `runWithConnection` runs on the reactor's + main thread during `pump` (the shim's `block_on` waits there for the tokio + workers that actually drive the connection). This keeps db queries + deterministic, matching the adbapi fake. + """ + pool = make_pool( + reactor=reactor, + clock=clock, + db_config=db_config, + engine=engine, + server_name=server_name, + ) + # `make_pool` started a real worker thread pool; stop it and swap in the + # threadless one so nothing actually runs on background threads. + pool.threadpool.stop() + pool.threadpool = ThreadPool(reactor) + return pool + + class ThreadPool: """ Threadless thread pool. @@ -1141,6 +1176,8 @@ def setup_test_homeserver( "cp_max": 5, }, } + if USE_RUST_DRIVER_FOR_TESTS: + database_config["use_rust_driver"] = True else: if SQLITE_PERSIST_DB: # The current working directory is in _trial_temp, so this gets created within that directory. @@ -1186,19 +1223,23 @@ def setup_test_homeserver( database = DatabaseConnectionConfig("master", database_config) config.database.databases = [database] - db_engine = create_engine(database.config) - # Create the database before we actually try and connect to it, based off # the template database we generate in setupdb() if USE_POSTGRES_FOR_TESTS: - db_conn = db_engine.module.connect( + # Creating and dropping databases is admin work, done directly over + # psycopg2 (always available in tests) regardless of the driver under + # test — CREATE DATABASE can't run inside a transaction, so it needs an + # autocommit connection. + import psycopg2 + + db_conn = psycopg2.connect( dbname=POSTGRES_BASE_DB, user=POSTGRES_USER, host=POSTGRES_HOST, port=POSTGRES_PORT, password=POSTGRES_PASSWORD, ) - db_engine.attempt_to_set_autocommit(db_conn, True) + db_conn.autocommit = True cur = db_conn.cursor() cur.execute("DROP DATABASE IF EXISTS %s;" % (test_db,)) cur.execute( @@ -1212,15 +1253,15 @@ def setup_test_homeserver( dropped = False - # Drop the test database - db_conn = db_engine.module.connect( + # Drop the test database (admin work over psycopg2, as above). + db_conn = psycopg2.connect( dbname=POSTGRES_BASE_DB, user=POSTGRES_USER, host=POSTGRES_HOST, port=POSTGRES_PORT, password=POSTGRES_PASSWORD, ) - db_engine.attempt_to_set_autocommit(db_conn, True) + db_conn.autocommit = True cur = db_conn.cursor() # Try a few times to drop the DB. Some things may hold on to the diff --git a/tests/storage/test_rust_dbapi.py b/tests/storage/test_rust_dbapi.py index f7270d860e..828d11cc50 100644 --- a/tests/storage/test_rust_dbapi.py +++ b/tests/storage/test_rust_dbapi.py @@ -58,6 +58,22 @@ class BuildDsnTestCase(unittest.TestCase): "dbname=synapse user=u host=db port=5432", ) + def test_maps_database_alias_to_dbname(self) -> None: + # psycopg2 accepts `database` as an alias for libpq's `dbname` (Synapse's + # sample config and most deployments use it); the strict libpq DSN the + # Rust pool parses only knows `dbname`, so it must be translated. + self.assertEqual( + rust_dbapi.build_dsn({"database": "synapse", "user": "u"}), + "dbname=synapse user=u", + ) + + def test_skips_none_values(self) -> None: + # `None` kwargs (unset config) are omitted, as psycopg2 treats them. + self.assertEqual( + rust_dbapi.build_dsn({"dbname": "d", "host": None, "port": None}), + "dbname=d", + ) + def test_quotes_values_needing_it(self) -> None: # Spaces / quotes / backslashes get single-quoted and escaped; empty → ''. self.assertEqual( diff --git a/tests/utils.py b/tests/utils.py index f3d5129097..39a7d59006 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -51,6 +51,9 @@ except ImportError: # POSTGRES_BASE_DB and update it to the current schema. Then, for each test case, we # create another unique database, using the base database as a template. USE_POSTGRES_FOR_TESTS = os.environ.get("SYNAPSE_POSTGRES", False) +# Run the Postgres tests against the native Rust driver instead of psycopg2. +# Only meaningful together with SYNAPSE_POSTGRES. +USE_RUST_DRIVER_FOR_TESTS = os.environ.get("SYNAPSE_TEST_RUST_DRIVER", False) LEAVE_DB = os.environ.get("SYNAPSE_LEAVE_DB", False) POSTGRES_USER = os.environ.get("SYNAPSE_POSTGRES_USER", None) POSTGRES_HOST = os.environ.get("SYNAPSE_POSTGRES_HOST", None)