Use `sqlx` inplace of `rusqlite` by mariocynicys · Pull Request #242 · talaia-labs/rust-teos

mariocynicys · 2023-09-20T18:39:38Z

This PR replaces the rusqlite SQLite engine with sqlx.

sqlx is an asynchronous versatile sql engine that supports both SQLite & PostgreSQL.

Due to the asynchronous-only nature of sqlx, every method that interacted with the DB had to be asynchronous as well. This ripple effect propagated to nearly all the methods of the tower components. Also to the lightning::chain::Listen trait, thus a new AsyncListen trait was introduced to replace it.

The sqlite driver is enabled with the sqlite feature, and the postgresql driver with the postgres feature.
By default, both of them are enabled, meaning that the tower program can run on top of either a sqlite or postgresql db.

To enable postgres-only driver: cargo build --no-default-features --features postgres (replace postgres with sqlite for sqlite-only build)

Fixes #32

This is temporary to avoid a dependency conflict that arises in `libsqlite3-sys`

The two DBMs are now in use, this is an intermediary commit and `dbm.rs` should be deleted and replaced with `dbm_new.rs`, and the tower components should be adjusted accordingly (drop the mutex around the dbm since the pool handles it by itself & await on dbm methods since they are now async)

Some mutexed needed their lifetimes be shortened due to the fact that an std mutex guard can't live across await point. one specific mutex on GateKeeper::registered_users was more fitting to be a tokio mutex instead due to the nature of how it is uses beside the database usage (std mutex is generally better/has less overhead than a tokio mutex). tests still need to be patched for async

Postgres tests are still not working because with each test we need to spawn a brand new database (best done inside a docker container) from within the tests

mariocynicys · 2023-10-14T10:10:48Z

teos/src/main.rs

+    // This spawns a separate async actor in the background that will be fed new blocks from a sync block listener.
+    // In this way we can have our components listen to blocks in an async manner from the async actor.
+    let listener = AsyncBlockListener::wrap_listener(listeners, dbm);
+
    let cache = &mut UnboundedCache::new();
-    let spv_client = SpvClient::new(tip, poller, cache, listener);
+    let spv_client = SpvClient::new(tip, poller, cache, &listener);
    let mut chain_monitor = ChainMonitor::new(


I think it's gonna be cleaner to merge all of these into the chain monitor.
And also merge async_listener.rs into chain_monitor.rs and change the chain monitor tests to accommodate the AsyncListen trait.

this still needs an external test db running on the background. one can easily be spawned with docker

remove rusqlite dependency from teos-common

mariocynicys · 2023-10-16T14:55:44Z

To test the tower with postgresql DB, this docker compose script might be handy:

version: '3.1'

services:

  db:
    image: postgres
    restart: always
    ports:
      - 5432:5432
    environment:
      POSTGRES_PASSWORD: pass
      POSTGRES_USER: user
      POSTGRES_DB: teos

orbitalturtle

Awesome PR :confetti: I'm looking forward to being able to use a postgres backend.

A couple thoughts from my first look:

Would be nice to have a high-level explanation in the README (or another doc if the main README is getting to long?) explaining that both sqlite and postgres are now supported & how sqlite is the default, but users can use the --database-url config option to use Postgres, etc.
Commits could maybe be cleaned up a bit to be more self-contained, looks like some of the later ones can be squashed with the earlier ones.

Will give this a more thorough test run soon as well

orbitalturtle · 2023-10-21T03:27:20Z

teos/src/dbm.rs

 )",
 ];

+/// Packs the errors than can raise when interacting with the underlying database.


nit: typo "that"

orbitalturtle · 2023-10-21T07:23:52Z

teos/src/config.rs

    pub btc_rpc_port: u16,

+    // Database
+    pub database_url: String,


looks like a repeat of line 91

orbitalturtle · 2023-10-21T07:44:06Z

teos/src/config.rs

    #[structopt(long, default_value = "~/.teos")]
    pub data_dir: String,

+    /// Database connection string [default: managed (managed SQLite database)]


Since this is an option and the default seems to be setting up a sqlite db, we could perhaps just let the default be "None" instead of the string "managed"?

orbitalturtle · 2023-10-21T08:00:39Z

teos/src/dbm_new.rs

+        #[cfg(feature = "postgres")]
+        return_db_if_matching!(connection_string, postgres, "migrations/postgres");
+
+        let supported_dbs = vec![


orbitalturtle · 2023-10-21T08:01:47Z

teos/src/dbm_new.rs

+            .bind(user_info.subscription_expiry as i64)
+            .execute(&self.pool)
+            .await
+            .map(|_| ())


? instead of .map to bubble up the error? (and same thing for some of the calls below)

Also the debug and error messages on successful/failed db insertions/etc are missing here and below

mariocynicys added 6 commits September 17, 2023 19:22

abandon teos-common dbm

e37782d

Use an actor to provide async runtime for the tower components

8bbe5fa

Upgrade to rusqlite 0.29

60e3eb7

This is temporary to avoid a dependency conflict that arises in `libsqlite3-sys`

Adjusted the tests (basically adding .awaits)

38abec0

Postgres tests are still not working because with each test we need to spawn a brand new database (best done inside a docker container) from within the tests

mariocynicys commented Oct 14, 2023

View reviewed changes

mariocynicys added 9 commits October 16, 2023 11:24

fixing lint errors

4bd2051

fix grammar issues

ba321f4

no need for type =

e5519d0

mysql todo

233b604

fix postgresql tests

178729f

this still needs an external test db running on the background. one can easily be spawned with docker

document clear_db method

d42d4bc

remove DB logic from teos-common

3aaa7c9

remove rusqlite dependency from teos-common

elaborating a comment

0d4e6ea

fix a typo

782ed39

mariocynicys force-pushed the general-sql-engines-with-mem-opts branch from c955cda to 782ed39 Compare October 16, 2023 08:27

mariocynicys marked this pull request as ready for review October 16, 2023 19:32

mariocynicys requested a review from sr-gi October 16, 2023 19:35

orbitalturtle reviewed Oct 27, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `sqlx` inplace of `rusqlite`#242

Use `sqlx` inplace of `rusqlite`#242
mariocynicys wants to merge 15 commits intotalaia-labs:masterfrom
mariocynicys:general-sql-engines-with-mem-opts

mariocynicys commented Sep 20, 2023 •

edited

Loading

Uh oh!

mariocynicys Oct 14, 2023

Uh oh!

mariocynicys commented Oct 16, 2023

Uh oh!

orbitalturtle left a comment

Uh oh!

orbitalturtle Oct 21, 2023

Uh oh!

orbitalturtle Oct 21, 2023

Uh oh!

orbitalturtle Oct 21, 2023

Uh oh!

orbitalturtle Oct 21, 2023

Uh oh!

orbitalturtle Oct 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

mariocynicys commented Sep 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mariocynicys Oct 14, 2023

Choose a reason for hiding this comment

Uh oh!

mariocynicys commented Oct 16, 2023

Uh oh!

orbitalturtle left a comment

Choose a reason for hiding this comment

Uh oh!

orbitalturtle Oct 21, 2023

Choose a reason for hiding this comment

Uh oh!

orbitalturtle Oct 21, 2023

Choose a reason for hiding this comment

Uh oh!

orbitalturtle Oct 21, 2023

Choose a reason for hiding this comment

Uh oh!

orbitalturtle Oct 21, 2023

Choose a reason for hiding this comment

Uh oh!

orbitalturtle Oct 21, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mariocynicys commented Sep 20, 2023 •

edited

Loading