init db and table, check health #55

tankyleo · 2025-08-15T07:39:59Z

Soft-push, seems to be working !

for now check health is just used internally to test the database on startup.

ldk-reviews-bot · 2025-08-15T07:40:02Z

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

tnull

Took a first look and left some higher-level comments. Have yet to review the actual database statements and operations.

rust/impls/Cargo.toml

rust/impls/src/postgres_store.rs

rust/server/src/main.rs

tankyleo · 2025-08-15T16:55:54Z

A possible todo I thought of: right now our TABLE_CHECK_STMT SELECT 1 FROM vss_db WHERE false merely checks whether a vss_db table exists in the database. We could have a deeper check of all the columns in the table etc...

tnull · 2025-08-18T12:40:32Z

rust/impls/src/postgres_store.rs

+	pub async fn new(postgres_endpoint: &str, db_name: &str, init_db: bool) -> Result<Self, Error> {
+		if init_db {
+			tokio::time::timeout(
+				tokio::time::Duration::from_secs(3),


Can we extract the timeout value(s) into appropriately named consts?

Yes thank you !

tnull · 2025-08-18T12:41:42Z

rust/impls/src/postgres_store.rs

+		Ok(())
+	}
+
+	async fn check_health(&self) -> Result<(), Error> {


Do we really need this? I think we should be able to trust that the CREATE TABLE statement does the right thing if it doesn't error out?

This mostly useful if the user does not set --init-db; in that case we want to make sure we can establish a connection to the database as part of startup checks before entering the main loop ?

If this passes, then we can log something like "Connected to PostgreSQL backend" in main as you suggested.

tnull · 2025-08-18T12:44:05Z

rust/server/src/main.rs

 		std::process::exit(1);
 	}
+	let init_db = args.iter().any(|arg| arg == "--init-db");


Why is this an optional flag to begin with?

When the flag is not set, I want the startup sequence to abort if the database is not present.

When the flag is set, I want the startup sequence to create the database if it is not present.

Right, just wondering when we'd ever want the first case?

If the intention is that we can initiate a VSS server instance even if we don't have / can establish a connection to the Postgres backend (which I don't fully buy), then we'd need to take additional steps. AFAIU, Pool::build(), which we call ~just after the init_db block would just as well fail if we can't connect to the pool:

"The Pool will not be returned until it has established its configured minimum number of connections, or it times out." (https://docs.rs/bb8/0.9.0/bb8/struct.Builder.html#method.build)

Right, just wondering when we'd ever want the first case?

If the user does not intend to initialize a new database (which is most cases), I don't think we should silently create a new database if we don't find the one we are looking for. Instead, I think we should complain loudly, and make sure the user actually intends to create a new one.

If the intention is that we can initiate a VSS server instance even if we don't have / can establish a connection to the Postgres backend (which I don't fully buy), then we'd need to take additional steps. AFAIU, Pool::build(), which we call ~just after the init_db block would just as well fail if we can't connect to the pool:

Actually Pool::build returns Ok(()) even if it cannot establish a connection to the Postgres backend - see #52 (comment) . On the main branch, we then enter the main loop even though we have established no connections to the Postgres backend.

If the user does not intend to initialize a new database (which is most cases), I don't think we should silently create a new database if we don't find the one we are looking for. Instead, I think we should complain loudly, and make sure the user actually intends to create a new one.

Hmm, I don't know. A binary that needs a special flag to init just on first start is a bit weird tbh. I can't think of another daemon that works that way.

Actually Pool::build returns Ok(()) even if it cannot establish a connection to the Postgres backend - see #52 (comment) . On the main branch, we then enter the main loop even though we have established no connections to the Postgres backend.

Huh, this API is very unexpected then. You'd expect it to return a TimedOut error if the connection attempts failed, just as get does. Might even be good to add a comment there in our code.

Huh, this API is very unexpected then. You'd expect it to return a TimedOut error if the connection attempts failed, just as get does. Might even be good to add a comment there in our code.

Sounds good will add the comment. Agreed this is unexpected.

Hmm, I don't know. A binary that needs a special flag to init just on first start is a bit weird tbh. I can't think of another daemon that works that way.

I agree it is weird. FWIW, CLN, LDK-node automatically generates a new wallet, but LND requires you to explicitly run lncli createwallet instead of lncli unlock (lncli unlock aborts startup if the wallet does not exist).

If we always create a new database if it does not exist, when a user misconfigures the DB connection string to an existing database, we'll create a new database and complete startup, when instead we should(?) have aborted startup. See #52 (comment)

cc @TheBlueMatt

I don't have a super strong feeling. I've seen it done both ways, though it is generally the case for binaries like this that they'll auto-create the table by default, I can see why that might be surprising.

tnull · 2025-08-18T12:45:15Z

rust/server/src/main.rs

 				.await
 				.unwrap(),
 		);
+		println!("Loaded postgres!");


If I read that message I'd be a bit confused what's meant exactly. Maybe "Connected to PostgreSQL backend with address .."?

Yes that's clearer I agree thank you

tnull · 2025-08-18T12:45:41Z

rust/server/src/main.rs

 		let rest_svc_listener =
 			TcpListener::bind(&addr).await.expect("Failed to bind listening port");
+		println!("Bound to {}", addr);


Maybe "Listening for incoming connections on .."

rust/server/src/util/config.rs

tnull

Let me know when this is ready for another round of review.

rust/server/src/util/config.rs

rust/impls/src/postgres_store.rs

tankyleo · 2025-08-19T14:53:14Z

Let me know when this is ready for another round of review.

Thanks for the review @tnull I've responded to your comments above would be good to see your responses before I write more code :)

TheBlueMatt · 2025-08-20T17:54:39Z

rust/impls/src/postgres_store.rs

+const DB_INIT_CMD: &str = "CREATE DATABASE";
+const TABLE_CHECK_STMT: &str = "SELECT 1 FROM vss_db WHERE false";
+const TABLE_INIT_STMT: &str = "
+	CREATE TABLE IF NOT EXISTS vss_db (


In addition to the main table itself, we really need a "config" table that at least just stores the version of the schema we're currently using. That way we can easily check that the schema in use is compatible with this version and, in the future, we can upgrade the schema as needed.

I think in Postgres migrations are usually done by sequentially applying the corresponding XXX_YYY.sql files. I agree it's probably good to track what version was last applied in a separate schema_version table or similar.

However, this probably also highlights that it would make sense to move the SQL statements to a dedicated migrations.rs, and rename TABLE_INIT_STMT in accordance to the .sql file to const V0_CREATE_VSS_DB, essentially to prepare for the eventual V1_.., V2_, .. migrations to follow.

FWIW, we could even consider to read in the sql files on startup. If we don't want to do that, we could consider dropping the sql files and just have the migrations.rs to avoid the risk of having them get out-of-sync at some point.

The timeouts were not needed as the inner futures have a 30 second timeout themselves, I was just too impatient.

…ections

init db and table, check health

6b09b86

tankyleo requested a review from tnull August 15, 2025 07:42

tnull reviewed Aug 15, 2025

View reviewed changes

rust/impls/Cargo.toml Outdated Show resolved Hide resolved

rust/impls/src/postgres_store.rs Outdated Show resolved Hide resolved

rust/impls/src/postgres_store.rs Outdated Show resolved Hide resolved

rust/server/src/main.rs Outdated Show resolved Hide resolved

tankyleo added 2 commits August 15, 2025 17:02

fixup: Remove String allocation

14c39a9

fixup: Add required time tokio feature

837b300

tankyleo force-pushed the init branch from 08ed0fc to 837b300 Compare August 15, 2025 17:03

tnull self-requested a review August 18, 2025 11:15

tnull reviewed Aug 18, 2025

View reviewed changes

tnull reviewed Aug 19, 2025

View reviewed changes

rust/server/src/util/config.rs Show resolved Hide resolved

rust/impls/src/postgres_store.rs Outdated Show resolved Hide resolved

TheBlueMatt reviewed Aug 20, 2025

View reviewed changes

tankyleo added 10 commits August 21, 2025 01:21

fixup: remove --init-db flag

ecf9a09

fixup: extract postgres init timeout into its own constant

8e3a84a

fixup: edit stdout messages for clarity

fcd709c

fixup: make all error messages consistent

7281aca

fixup: adjust comments

7ca8e92

fixup: Remove tokio::time::timeout, and unnecessary map_err

a4782eb

The timeouts were not needed as the inner futures have a 30 second timeout themselves, I was just too impatient.

fixup: Do not log anything if the database already exists

ab51ee7

fixup: Add comment to highlight that by default Pool maintains 0 conn…

046d8d3

…ections

fixup: Use Pool for the one-off connection to /postgres

62aec0c

fixup: revert database name in given configuration file

054c880

init db and table, check health #55

Are you sure you want to change the base?

init db and table, check health #55

Uh oh!

Conversation

tankyleo commented Aug 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldk-reviews-bot commented Aug 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tankyleo commented Aug 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tankyleo Aug 19, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

tankyleo Aug 20, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tankyleo commented Aug 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull Aug 21, 2025 • 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!

Uh oh!

tankyleo commented Aug 15, 2025 •

edited

Loading

ldk-reviews-bot commented Aug 15, 2025 •

edited

Loading

tankyleo Aug 19, 2025 •

edited

Loading

tankyleo Aug 20, 2025 •

edited

Loading

tnull Aug 21, 2025 •

edited

Loading