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

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.

Reconsidered offline. Fine to leave for now, for the release we'll change it anyways as part of #900

ldk-reviews-bot · 2026-05-09T00:00:50Z

🔔 2nd 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.

ldk-reviews-bot · 2026-05-11T00:02:08Z

🔔 3rd 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.

ldk-reviews-bot · 2026-05-13T00:03:08Z

🔔 4th 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.

ldk-reviews-bot · 2026-05-16T00:00:31Z

🔔 5th 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.

ldk-reviews-bot · 2026-05-18T00:01:31Z

🔔 6th 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.

ldk-reviews-bot · 2026-05-20T00:02:12Z

🔔 7th 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.

ldk-reviews-bot · 2026-05-23T00:00:43Z

🔔 8th 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.

ldk-reviews-bot · 2026-05-25T00:01:53Z

🔔 9th 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.

ldk-reviews-bot · 2026-05-27T00:02:13Z

🔔 10th 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.

ldk-reviews-bot · 2026-05-30T00:01:05Z

🔔 11th 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

This needs a minor rebase by now. Also let me know once you looked further into the blocking behavior you saw in the benchmarks (discussed offline)

benthecarman · 2026-05-31T23:12:39Z

Also let me know once you looked further into the blocking behavior you saw in the benchmarks (discussed offline)

Yeah so in the payment benchmark (see #915) it currently fails with a postgres backend because we’re hitting the blocking task limit in tokio by using block_in_place for lots of async writes (postgres writes).

We can fix this by adding a dedicated runtime to the postgres store similar to VSS or by taking the dep postgres-sync (it works by adding a runtime too). However we were removed that in this PR previously.

Otherwise the real fix is to have DataStore use the async KV Store, however, this would be a much larger refactor as this would likely require us to have the actual api of ldk-node be async

ldk-reviews-bot · 2026-06-01T00:01:46Z

🔔 12th 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-06-01T08:57:54Z

Also let me know once you looked further into the blocking behavior you saw in the benchmarks (discussed offline)

Yeah so in the payment benchmark (see #915) it currently fails with a postgres backend because we’re hitting the blocking task limit in tokio by using block_in_place for lots of async writes (postgres writes).

We can fix this by adding a dedicated runtime to the postgres store similar to VSS or by taking the dep postgres-sync (it works by adding a runtime too). However we were removed that in this PR previously.

Otherwise the real fix is to have DataStore use the async KV Store, however, this would be a much larger refactor as this would likely require us to have the actual api of ldk-node be async

Okay, good to hear it was indeed the blocking worker limit. Mind opening an issue for this? We plan to make DataStore async anyways (though not in this PR ofc), I think then we should just be able to merge.

tnull · 2026-06-01T11:08:55Z

+		// keeps the previous value untouched on UPSERT-update; the sequence persists across
+		// restarts.
+		let sql = format!(
+			"CREATE TABLE IF NOT EXISTS {kv_table_name} (


Codex nit:

[P2] Quote or validate PostgreSQL identifiers — /home/tnull/workspace/ldk-node/src/io/postgres_store/mod.rs:348-348
Because kv_table_name is interpolated directly as an unquoted SQL identifier, a caller-provided table name such as tenant-1, a reserved word, or a schema-qualified name will fail during store setup even though the API accepts an arbitrary String. Quote identifiers safely or reject
unsupported names before formatting them into the table/comment/index SQL.

Seems now all Postgres tests are failing.

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>

tnull · 2026-06-02T07:59:53Z

+		// fails; otherwise it gets dropped silently. Log it at debug! so a later failure
+		// (e.g. an auth error that surfaces only when the pool is built) can be traced
+		// back to the real cause.
+		log::debug!(


Huh, what's this? We're not (always) using log facade in LDK Node. You'll need to give PostgresStore an Arc<Logger> and log towards that using the LDK macros to make sure users will always see the outputs independently of chosen backend.

tnull · 2026-06-02T08:07:45Z

+		let inner = PostgresStoreInner::new(connection_string, db_name, kv_table_name, tls).await?;
+		let inner = Arc::new(inner);
+		let next_write_version = AtomicU64::new(1);
+		let handle = tokio::runtime::Handle::current();


No, please hand in the Arc<Runtime> use that for the blocking tasks. We should also drop the custom block_on helper as it makes things even more confusing. Also note there is a bug here:

[P2] Build PostgresStore on the configured runtime — /home/tnull/workspace/ldk-node/src/builder.rs:676-677
When set_runtime is used and build_with_postgres_store is called from a different Tokio runtime, Runtime::block_on uses the caller's current runtime instead of the configured handle. PostgresStore::new captures Handle::current() and spawns its connection drivers there, so a node built from
a temporary caller runtime can later run persistence against a dropped runtime rather than the node runtime. Please force construction onto the configured runtime handle.

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

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

tnull reviewed May 6, 2026

View reviewed changes

benthecarman force-pushed the postgres branch from b373ff3 to 2ad7498 Compare May 7, 2026 16:56

nicolasburtey mentioned this pull request May 9, 2026

postgres lightningdevkit/ldk-server#204

Open

benthecarman force-pushed the postgres branch from 2ad7498 to 060cb69 Compare May 12, 2026 23:51

tnull mentioned this pull request May 20, 2026

One-shot persist: unified atomic persistence for ChannelManager and ChannelMonitors lightningdevkit/rust-lightning#4457

Open

tnull added this to the 0.8 milestone May 30, 2026

tnull reviewed May 30, 2026

View reviewed changes

benthecarman mentioned this pull request May 31, 2026

Add database persistence benchmarks #915

Draft

benthecarman force-pushed the postgres branch from 060cb69 to c697009 Compare May 31, 2026 23:06

tnull reviewed Jun 1, 2026

View reviewed changes

benthecarman force-pushed the postgres branch from c697009 to e15fa86 Compare June 1, 2026 12:37

benthecarman force-pushed the postgres branch from e15fa86 to d4803f4 Compare June 1, 2026 16:44

tnull reviewed Jun 2, 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!

ldk-reviews-bot commented May 6, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldk-reviews-bot commented May 9, 2026

ldk-reviews-bot commented Apr 1, 2026 •

edited

Loading