Add Postgres database by benthecarman · Pull Request #863 · lightningdevkit/ldk-node

benthecarman · 2026-04-01T19:55:17Z

Add a PostgresStore implementation behind the postgres feature flag, mirroring the existing SqliteStore. Uses tokio-postgres (async-native) with an internal tokio runtime for the sync KVStoreSync trait, following the VssStore pattern.

Also gates sqlite behind a feature flag, since most tests relied on this, we ended up needing to gate a lot of them behind the flag. Curious on thoughts on how to handle this best

ldk-reviews-bot · 2026-04-01T19:55:21Z

👋 Thanks for assigning @Camillarhi 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 high-level look.

Also gates sqlite behind a feature flag, since most tests relied on this, we ended up needing to gate a lot of them behind the flag. Curious on thoughts on how to handle this best

Not sure I'm following? Why do we want to feature-gate tests based on SQLite?

Also tagging @tankyleo as a secondary reviewer here as he has a lot of recent experience with Postgres on VSS Server.

tnull · 2026-04-02T10:54:07Z

+
+	// An internal runtime we use to avoid any deadlocks we could hit when waiting on async
+	// operations to finish from a sync context.
+	internal_runtime: Option<tokio::runtime::Runtime>,


I think we could avoid using an internal runtime for this and rather just reuse crate::runtime::Runtime, if we wanted. Though, when upstreaming to LDK we'll need to rethink all of this anyways (cf. VssStore upstreaming PR).

Yeah I just copied VSS to be safe.

Yeah, let's drop the extra runtime then.

This seems to be unaddressed? (cc @benthecarman). Or I guess you interpreted 'then' as 'when we upstream'? IMO it's likely unnecessary in this case, so might as well do it now, but let me know if you'd rather keep it for now.

Ohh missed your other comment, fixed

tnull · 2026-04-02T11:08:45Z


 	/// Builds a [`Node`] instance with a [`SqliteStore`] backend and according to the options
 	/// previously configured.
+	#[cfg(feature = "sqlite")]


Why do we suddenly introduce features? We so far held off and meant to do that in a dedicated PR at some point for this or next release. Is it necessary to make Postgres work?

okay removed for now

Seems the postgres feature is still present in the current version?

benthecarman · 2026-04-03T20:00:49Z

Did fixes in follow up commits for ease of review.

ldk-reviews-bot · 2026-04-04T11:11:09Z

🔔 1st Reminder

Hey @tankyleo! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-06T11:12:09Z

🔔 2nd Reminder

Hey @tankyleo! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

tankyleo

Verbose review, please dismiss aggressively :)

benthecarman · 2026-04-08T00:15:57Z

@tankyleo addressed most of your comments. Lots of docs improvements. Better handling around the db name and creating the db. And did the code clean ups + test improvements you suggested.

ldk-reviews-bot · 2026-04-11T15:52:17Z

🔔 1st Reminder

Hey @tankyleo! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-13T15:52:53Z

🔔 2nd Reminder

Hey @tankyleo! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

tankyleo

two small nits thank you

benthecarman · 2026-04-14T22:47:06Z

two small nits thank you

fixed

ldk-reviews-bot · 2026-04-16T22:41:39Z

🔔 1st Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-20T00:00:45Z

🔔 2nd Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

tnull · 2026-04-28T12:01:14Z

@benthecarman Any update here?

benthecarman · 2026-05-04T01:16:26Z

Responded to review

tankyleo · 2026-05-04T19:48:11Z

+/// Round-robin (rather than `tokio::select!` over all slot locks) is preferred so [`POOL_SIZE`]
+/// can be `cfg`-conditional without rewriting select arms. The trade-off: under contention the
+/// fallback `lock().await` may park on a busy slot while another frees up a moment later.


Hmm not sure it's worth dropping the tokio::select to help the tests ? You probably saw this I pass --test-threads 1 over at VSS when running tests to avoid flooding connections like you mention above. But perhaps there's a better path that doesn't require passing this flag (including maybe sticking to what you have here your call).

Switched to be a select, just have a smaller one for tests

tankyleo · 2026-05-04T19:51:37Z

+			kv_table_name,
+			tls_config,
+		)
+		.map_err(|_| BuildError::KVStoreSetupFailed)?;


Can we log the error returned here, like what I do in #893 ?

that needs build_with_store_and_logger. Maybe just need to have one of these depend on the other

Landed #893, please rebase to use build_with_store_and_logger.

tnull · 2026-05-05T08:24:24Z

+
+	// An internal runtime we use to avoid any deadlocks we could hit when waiting on async
+	// operations to finish from a sync context.
+	internal_runtime: Option<tokio::runtime::Runtime>,


This seems to be unaddressed? (cc @benthecarman). Or I guess you interpreted 'then' as 'when we upstream'? IMO it's likely unnecessary in this case, so might as well do it now, but let me know if you'd rather keep it for now.

tnull · 2026-05-05T08:28:30Z

+	/// `"postgres://user:password@localhost/ldk_db"`.
+	///
+	/// The given `db_name` will be used or default to [`DEFAULT_DB_NAME`]. The
+	/// `connection_string` must not include a `dbname` when `db_name` is set, providing both


I believe in VSS Server we discovered that different providers have different names for default databases, and we'll need to connect to the default database to be able to initialize the configured database if it's not pre-existing. It seems we might need to add the same config option for 'default db' here, too, which will only be used for the initial connection? Or do we see a better option (given this is a quite ugly/confusing API)?

@tankyleo discussed this and thought it might be better to punt on it for now. You can always just use whatever bespoke default db they use as your db instead of the DEFAULT_DB_NAME in your connection string. But yeah can add if you think it'll cause issues

tnull · 2026-05-05T08:32:45Z

+/// TLS configuration for PostgreSQL connections.
+#[cfg(any(feature = "postgres", feature = "uniffi"))]
+#[derive(Debug, Clone)]
+pub struct PostgresTlsConfig {


This doesn't belong into this file but should be part of the postgres_store sub-module or if we deem it part of the user-facing builder API, it should be in builder.rs.

Put it here so the bindings had access to it. Was unsure if we wanted to have the bindings require postgres enabled or not. Currently its behind the feature flag but we still need the type exposed

We usually have the pattern of exposing any types required by the bindigns by re-exporting at crate level via the use statements in ffi/types.rs. That should work here, too? I.e., have the type live in the sub-module, and just expose it via that at crate level under uniffi?

We def. want to expose it also under bindings. While probably not super interesting for Swift/Kotlin mobile users, we also have some Go bindings users for which postgres could be interesting at some point, potentially.

tnull · 2026-05-05T08:34:56Z

+	///
+	/// If `tls_config` is `Some`, TLS will be used for database connections. A custom CA
+	/// certificate can be provided via
+	/// [`PostgresTlsConfig::certificate_pem`](io::postgres_store::PostgresTlsConfig::certificate_pem),


Given the PostgresTlsConfig currently only holds one field, should certificate_pem just be an optional field directly on build_with_postgres_store? And then we could use PostgresTlsConfig internally only for now?

yeah this solves the bindings issue above too

Add a PostgresStore implementation behind the "postgres" feature flag, mirroring the existing SqliteStore. Uses tokio-postgres (async-native) with an internal tokio runtime for the sync KVStoreSync trait, following the VssStore pattern. Includes unit tests, integration tests (channel full cycle and node restart), and a CI workflow that runs both against a PostgreSQL service container. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

ldk-reviews-bot · 2026-05-06T01:16:47Z

🔔 1st Reminder

Hey @Camillarhi! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

tnull · 2026-05-06T08:57:41Z


 [features]
 default = []
+postgres = ["dep:tokio-postgres", "dep:native-tls", "dep:postgres-native-tls"]


Hmm, we probably want to introduce features even before the next release, but still not a fan of just adding a one-off feature for this.

benthecarman requested a review from tnull April 1, 2026 19:55

benthecarman force-pushed the postgres branch from c334535 to 8becefe Compare April 1, 2026 20:12

tnull reviewed Apr 2, 2026

View reviewed changes

tnull requested a review from tankyleo April 2, 2026 11:10

benthecarman added this to Weekly Goals Apr 2, 2026

benthecarman self-assigned this Apr 2, 2026

benthecarman force-pushed the postgres branch 2 times, most recently from b5d297e to e8f478b Compare April 3, 2026 19:57

tankyleo reviewed Apr 7, 2026

View reviewed changes

benthecarman force-pushed the postgres branch 4 times, most recently from a8589ea to a43b13b Compare April 8, 2026 01:08

tankyleo self-requested a review April 9, 2026 15:52

tankyleo reviewed Apr 13, 2026

View reviewed changes

benthecarman force-pushed the postgres branch from a43b13b to a198c18 Compare April 14, 2026 20:21

benthecarman requested a review from tankyleo April 14, 2026 20:22

benthecarman force-pushed the postgres branch from a198c18 to 3c1e82f Compare April 14, 2026 20:34

tankyleo reviewed Apr 14, 2026

View reviewed changes

Comment thread src/builder.rs Outdated

benthecarman force-pushed the postgres branch 2 times, most recently from 3bb9810 to 437f11f Compare April 14, 2026 21:03

tankyleo self-requested a review April 14, 2026 21:42

tankyleo reviewed Apr 14, 2026

View reviewed changes

Comment thread src/builder.rs Outdated

Comment thread src/builder.rs Outdated

Comment thread src/io/postgres_store/mod.rs Outdated

tankyleo requested a review from tnull April 14, 2026 22:40

benthecarman force-pushed the postgres branch from 437f11f to 63b42e5 Compare April 14, 2026 22:46

benthecarman mentioned this pull request Apr 15, 2026

Beta Release lightningdevkit/ldk-server#121

Open

25 tasks

tnull reviewed Apr 20, 2026

View reviewed changes

Camillarhi reviewed Apr 20, 2026

View reviewed changes

Comment thread src/builder.rs Outdated

Comment thread src/io/postgres_store/mod.rs Outdated

benthecarman force-pushed the postgres branch 3 times, most recently from 85652a1 to f81b8a0 Compare May 3, 2026 19:33

benthecarman requested review from Camillarhi and tnull May 4, 2026 01:16

tankyleo reviewed May 4, 2026

View reviewed changes

benthecarman force-pushed the postgres branch from f81b8a0 to 3eb76df Compare May 5, 2026 02:42

tnull mentioned this pull request May 5, 2026

Log the error returned from SqliteStore::new and fs::create_dir_all #893

Merged

tnull reviewed May 5, 2026

View reviewed changes

benthecarman force-pushed the postgres branch from 3eb76df to 69e82d4 Compare May 5, 2026 15:46

benthecarman force-pushed the postgres branch from 69e82d4 to b373ff3 Compare May 5, 2026 15:55

tnull reviewed May 6, 2026

View reviewed changes

Conversation

benthecarman commented Apr 1, 2026

Uh oh!

ldk-reviews-bot commented Apr 1, 2026 • 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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

benthecarman commented Apr 3, 2026

Uh oh!

ldk-reviews-bot commented Apr 4, 2026

Uh oh!

ldk-reviews-bot commented Apr 6, 2026

Uh oh!

tankyleo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

benthecarman commented Apr 8, 2026

Uh oh!

ldk-reviews-bot commented Apr 11, 2026

Uh oh!

ldk-reviews-bot commented Apr 13, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tankyleo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

benthecarman commented Apr 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldk-reviews-bot commented Apr 16, 2026

Uh oh!

ldk-reviews-bot commented Apr 20, 2026

Uh oh!

ldk-reviews-bot commented Apr 1, 2026 •

edited

Loading

benthecarman commented Apr 14, 2026 •

edited

Loading

benthecarman May 5, 2026 •

edited

Loading

tnull May 6, 2026 •

edited

Loading