feat: add PostgreSQL database driver by DamnCrab · Pull Request #1684 · torrust/torrust-tracker

DamnCrab · 2026-04-09T08:00:35Z

Summary

Add PostgreSQL as a third supported database backend, alongside the existing SQLite and MySQL drivers.

Motivation

The tracker previously only supported SQLite and MySQL. PostgreSQL is widely used in production environments and adds an important deployment option.

Changes

`packages/configuration`

Added PostgreSQL variant to the Driver enum (serializes as "postgresql")
Added PostgreSQL connection string documentation and example
Added PostgreSQL URL password masking in mask_secrets()
Added unit test for PostgreSQL password masking

`packages/tracker-core`

Added r2d2_postgres = "0.18" dependency
New databases/driver/postgres.rs — full Database trait implementation:
- All CRUD operations: torrents, whitelist, authentication keys, global downloads
- ON CONFLICT DO UPDATE upsert semantics (equivalent to SQLite/MySQL INSERT OR REPLACE / ON DUPLICATE KEY UPDATE)
Added PostgreSQL variant to the tracker-core Driver enum
Added GenericConnectionError variant to the database Error enum for PostgreSQL-specific connection errors
Added From<r2d2_postgres::postgres::Error> conversion for Error
Updated databases/setup.rs to handle the new PostgreSQL variant
Updated module-level documentation to reference three drivers

- Add PostgreSQL variant to configuration and tracker-core Driver enums - Add r2d2_postgres dependency to tracker-core - Create PostgreSQL driver implementation with all Database trait methods - Use std::thread::scope to avoid tokio runtime conflicts (sync postgres crate creates its own internal tokio runtime) - Implement custom Drop to safely destroy connection pool outside tokio - Add GenericConnectionError variant to database Error enum - Add PostgreSQL password masking in configuration - Add tests for local PostgreSQL and testcontainers - Update module documentation Agent-Logs-Url: https://github.com/DamnCrab/torrust-tracker/sessions/b75f4713-d498-4ee2-9224-e0e452ec2f3b Co-authored-by: DamnCrab <42539593+DamnCrab@users.noreply.github.com>

josecelano · 2026-04-09T08:06:12Z

Relates to:

Hi @DamnCrab thank you for your PR! I will review it today or tomorrow. PostgreSQL will be a really good feature for the tracker.

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds PostgreSQL as a supported persistence backend (in addition to SQLite and MySQL) across configuration and tracker-core.

Changes:

Introduces a new PostgreSQL driver implementation and wires it into driver selection/build paths.
Extends configuration schema/docs and secret-masking to support PostgreSQL connection URLs.
Extends database error handling to cover PostgreSQL-specific errors.

Reviewed changes

Copilot reviewed 8 out of 9 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
project-words.txt	Adds “postgresql” to the project dictionary/wordlist.
packages/tracker-core/src/databases/setup.rs	Maps configuration driver enum to the tracker-core driver enum for PostgreSQL.
packages/tracker-core/src/databases/mod.rs	Updates module docs to list PostgreSQL as a supported driver.
packages/tracker-core/src/databases/error.rs	Adds a generic connection error variant and a conversion from postgres errors.
packages/tracker-core/src/databases/driver/postgres.rs	New PostgreSQL `Database` trait implementation plus integration tests.
packages/tracker-core/src/databases/driver/mod.rs	Registers the PostgreSQL driver and adds factory support in `build()`.
packages/tracker-core/Cargo.toml	Adds `r2d2_postgres` dependency.
packages/configuration/src/v2_0_0/database.rs	Adds PostgreSQL to config enum/docs + masks passwords for PostgreSQL URLs + unit test.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-09T08:09:58Z

packages/tracker-core/src/databases/driver/postgres.rs

+            conn.execute(create_torrents_table, &[])
+                .expect("Could not create torrents table.");
+            conn.execute(create_torrent_aggregate_metrics_table, &[])
+                .expect("Could not create torrent_aggregate_metrics table.");
+            conn.execute(&create_keys_table, &[]).expect("Could not create keys table.");
+            conn.execute(create_whitelist_table, &[])
+                .expect("Could not create whitelist table.");


The method returns Result<(), Error> but uses expect(...), which will panic instead of returning a structured Error. This breaks the error-contract of the Database trait and makes initialization failures harder to handle/diagnose in production. Prefer propagating the postgres errors with ? so callers receive an Err(Error::...) instead of a panic.

Copilot · 2026-04-09T08:09:59Z

packages/tracker-core/src/databases/driver/postgres.rs

+            conn.execute("DROP TABLE whitelist;", &[])
+                .expect("Could not drop whitelist table.");
+            conn.execute("DROP TABLE torrents;", &[])
+                .expect("Could not drop torrents table.");
+            conn.execute("DROP TABLE torrent_aggregate_metrics;", &[])
+                .expect("Could not drop torrent_aggregate_metrics table.");
+            conn.execute("DROP TABLE keys;", &[]).expect("Could not drop keys table.");


Similar to table creation, this uses expect(...) inside a Result-returning method, causing panics rather than returning Error. Additionally, unconditional DROP TABLE ...; will error if tables are already missing (e.g., cleanup called twice), which currently becomes a panic. Use error propagation and consider DROP TABLE IF EXISTS ... to make teardown idempotent.

Copilot · 2026-04-09T08:09:59Z

packages/tracker-core/src/databases/driver/postgres.rs

+                completed INTEGER DEFAULT 0 NOT NULL
+            );";
+
+            let create_torrent_aggregate_metrics_table = "
+            CREATE TABLE IF NOT EXISTS torrent_aggregate_metrics (
+                id SERIAL PRIMARY KEY,
+                metric_name VARCHAR(50) NOT NULL UNIQUE,
+                value INTEGER DEFAULT 0 NOT NULL


completed/value are modeled as download counters but stored as Postgres INTEGER (32-bit signed). This will overflow in real-world usage, and the current code also converts via i32::try_from(...).unwrap() / u32::try_from(...).unwrap(), which will panic once counters exceed i32::MAX. Use BIGINT in the schema and i64/u64 (with fallible conversion or clamping strategy) in code to avoid panics and data loss.

Suggested change

completed INTEGER DEFAULT 0 NOT NULL

);";

let create_torrent_aggregate_metrics_table = "

CREATE TABLE IF NOT EXISTS torrent_aggregate_metrics (

id SERIAL PRIMARY KEY,

metric_name VARCHAR(50) NOT NULL UNIQUE,

value INTEGER DEFAULT 0 NOT NULL

completed BIGINT DEFAULT 0 NOT NULL

);";

let create_torrent_aggregate_metrics_table = "

CREATE TABLE IF NOT EXISTS torrent_aggregate_metrics (

id SERIAL PRIMARY KEY,

metric_name VARCHAR(50) NOT NULL UNIQUE,

value BIGINT DEFAULT 0 NOT NULL

Copilot · 2026-04-09T08:09:59Z

packages/tracker-core/src/databases/driver/postgres.rs

+    fn with_connection<F, T>(&self, f: F) -> Result<T, Error>
+    where
+        F: FnOnce(&mut r2d2::PooledConnection<PostgresConnectionManager<NoTls>>) -> Result<T, Error> + Send,
+        T: Send,
+    {
+        let pool = self.pool();
+        std::thread::scope(|s| {
+            s.spawn(|| {
+                let mut conn = pool.get().map_err(|e| (e, DRIVER))?;
+                f(&mut conn)
+            })
+            .join()
+            .expect("PostgreSQL worker thread panicked")
+        })
+    }


Spawning and joining an OS thread for every DB operation is extremely expensive and can become a throughput bottleneck under load (and risks thread exhaustion if this pattern spreads). If the goal is to avoid nested-tokio-runtime panics, consider switching to an async-native client/pool (e.g., tokio-postgres + a pool) or isolating all sync-postgres work onto a dedicated long-lived worker thread (or bounded thread pool) instead of per-call thread creation.

Copilot · 2026-04-09T08:10:00Z

packages/tracker-core/src/databases/driver/postgres.rs

+        let info_hash_hex = info_hash.to_hex_string();
+        self.with_connection(move |conn| {
+            let rows = conn.query("SELECT completed FROM torrents WHERE info_hash = $1", &[&info_hash_hex])?;


The driver mixes to_hex_string() and to_string() for serializing InfoHash values written/read from the DB. Even if Display currently matches hex, relying on it is brittle and makes cross-driver consistency harder to reason about. Prefer a single explicit encoding (likely to_hex_string() everywhere for DB storage) to guarantee round-tripping and compatibility.

Copilot · 2026-04-09T08:10:00Z

packages/tracker-core/src/databases/driver/postgres.rs

+    fn save_torrent_downloads(&self, info_hash: &InfoHash, completed: u32) -> Result<(), Error> {
+        let info_hash_str = info_hash.to_string();
+        self.with_connection(move |conn| {
+            let completed_i32 = i32::try_from(completed).unwrap();
+
+            conn.execute(
+                "INSERT INTO torrents (info_hash, completed) VALUES ($1, $2) ON CONFLICT (info_hash) DO UPDATE SET completed = EXCLUDED.completed",
+                &[&info_hash_str, &completed_i32],
+            )?;


The driver mixes to_hex_string() and to_string() for serializing InfoHash values written/read from the DB. Even if Display currently matches hex, relying on it is brittle and makes cross-driver consistency harder to reason about. Prefer a single explicit encoding (likely to_hex_string() everywhere for DB storage) to guarantee round-tripping and compatibility.

Copilot · 2026-04-09T08:10:00Z

packages/tracker-core/src/databases/error.rs

+        let e: DynError = Arc::new(err);
+        Error::InvalidQuery {
+            source: e.into(),
+            driver: Driver::PostgreSQL,


This conversion maps all postgres::Error instances to Error::InvalidQuery, including connection/IO failures that can occur during query execution. Since a GenericConnectionError variant was added, consider mapping connection-related postgres errors to GenericConnectionError (and reserving InvalidQuery for SQL/constraint/type errors) so callers can distinguish operational outages from query bugs.

Suggested change

let e: DynError = Arc::new(err);

Error::InvalidQuery {

source: e.into(),

driver: Driver::PostgreSQL,

let is_db_error = err.as_db_error().is_some();

let e: DynError = Arc::new(err);

if is_db_error {

Error::InvalidQuery {

source: e.into(),

driver: Driver::PostgreSQL,

}

} else {

Error::GenericConnectionError {

source: e.into(),

driver: Driver::PostgreSQL,

}

Copilot · 2026-04-09T08:10:01Z

packages/configuration/src/v2_0_0/database.rs

@@ -14,6 +14,8 @@ pub struct Database {
    /// `./storage/tracker/lib/database/sqlite3.db`.
    /// For `Mysql`, the format is `mysql://db_user:db_user_password:port/db_name`, for


The MySQL URL format description is missing the @host portion (mysql://user:pass@host:port/db). Since this block was updated for PostgreSQL, it’s a good opportunity to correct the MySQL format line to avoid misleading configuration docs.

Suggested change

/// For `Mysql`, the format is `mysql://db_user:db_user_password:port/db_name`, for

/// For `Mysql`, the format is `mysql://db_user:db_user_password@host:port/db_name`, for

Copilot · 2026-04-09T08:10:01Z

packages/tracker-core/src/databases/driver/postgres.rs

+    /*
+    We run a PostgreSQL container and run all the tests against the same container and database.
+
+    Test for this driver are executed with:


Correct grammar in this comment for clarity.

Suggested change

Test for this driver are executed with:

Tests for this driver are executed with:

Agent-Logs-Url: https://github.com/DamnCrab/torrust-tracker/sessions/1b7dc46e-9918-4a95-91fe-29c1b375eb8a Co-authored-by: DamnCrab <42539593+DamnCrab@users.noreply.github.com>

josecelano

Thank you for the PR and for the work to add PostgreSQL support. I reviewed it carefully locally and I appreciate the effort that went into getting the new driver working and covered by tests.

After reviewing the implementation, I’m going to reject this PR in its current form.

The main reason is architectural: the PostgreSQL driver currently spawns a new OS thread for every database operation in order to isolate the sync postgres client from the tracker’s tokio runtime. Even though the implementation uses an r2d2 connection pool, it does not use a reusable execution pool for the blocking PostgreSQL work, so every query still pays the cost of thread creation and join. I do not think that performance model is acceptable for merge.

There is also a correctness issue in the current PostgreSQL implementation: torrent counters are stored as 32-bit INTEGER values and converted through i32, which creates a real overflow/panic risk once counters exceed 2,147,483,647.

In addition, I think the PR is still missing a couple of important pieces for a complete PostgreSQL feature:

a default PostgreSQL container config file parallel to the existing MySQL one
proper migrations or at least a migration structure/documentation in the persistence layer

My conclusion is that PostgreSQL support should be implemented after, or together with, the persistence redesign tracked in issue #1525:

#1525

I think it will be much easier to introduce PostgreSQL properly once persistence is overhauled, rather than adding it on top of the current design with a costly runtime workaround.

I wrote a more detailed local review here for reference:

https://github.com/josecelano/torrust-tracker/blob/pr-1684-review/docs/pr-reviews/pr-1684-postgresql-driver.md

Thank you again for the contribution. I would be happy to review a follow-up version once the persistence redesign is in place or this is reworked on top of it.

DamnCrab temporarily deployed to coverage April 9, 2026 08:01 — with GitHub Actions Inactive

josecelano requested a review from Copilot April 9, 2026 08:01

josecelano self-assigned this Apr 9, 2026

Copilot AI reviewed Apr 9, 2026

View reviewed changes

Copilot started reviewing on behalf of josecelano April 9, 2026 08:10 View session

fix: address code review suggestions (docs + grammar)

8614d7f

Agent-Logs-Url: https://github.com/DamnCrab/torrust-tracker/sessions/1b7dc46e-9918-4a95-91fe-29c1b375eb8a Co-authored-by: DamnCrab <42539593+DamnCrab@users.noreply.github.com>

Copilot AI temporarily deployed to coverage April 9, 2026 08:23 Inactive

fix: correct MySQL URL format in docs and fix grammar typo

7cb13d4

Agent-Logs-Url: https://github.com/DamnCrab/torrust-tracker/sessions/1b7dc46e-9918-4a95-91fe-29c1b375eb8a Co-authored-by: DamnCrab <42539593+DamnCrab@users.noreply.github.com>

Copilot AI temporarily deployed to coverage April 9, 2026 11:46 Inactive

josecelano requested changes Apr 9, 2026

View reviewed changes

DamnCrab marked this pull request as draft April 9, 2026 14:06

DamnCrab closed this Apr 11, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add PostgreSQL database driver#1684

feat: add PostgreSQL database driver#1684
DamnCrab wants to merge 3 commits intotorrust:developfrom
DamnCrab:copilot/assess-pg-adaptation-difficulty

DamnCrab commented Apr 9, 2026

Uh oh!

josecelano commented Apr 9, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 9, 2026

Uh oh!

Copilot AI Apr 9, 2026

Uh oh!

Copilot AI Apr 9, 2026

Uh oh!

Copilot AI Apr 9, 2026

Uh oh!

Copilot AI Apr 9, 2026

Uh oh!

Copilot AI Apr 9, 2026

Uh oh!

Copilot AI Apr 9, 2026

Uh oh!

Copilot AI Apr 9, 2026

Uh oh!

Copilot AI Apr 9, 2026

Uh oh!

josecelano left a comment •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		@@ -14,6 +14,8 @@ pub struct Database {
		/// `./storage/tracker/lib/database/sqlite3.db`.
		/// For `Mysql`, the format is `mysql://db_user:db_user_password:port/db_name`, for

	Test for this driver are executed with:
	Tests for this driver are executed with:

Conversation

DamnCrab commented Apr 9, 2026

Summary

Motivation

Changes

packages/configuration

packages/tracker-core

Uh oh!

josecelano commented Apr 9, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

josecelano left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

`packages/configuration`

`packages/tracker-core`

josecelano left a comment •

edited

Loading