Conversation
Make rusqlite an optional dependency behind the new "sqlite" feature, which is enabled by default. This allows users who use an alternative storage backend to avoid compiling the bundled SQLite C library. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
👋 Thanks for assigning @tankyleo as a reviewer! |
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
left a comment
tnull
left a comment
There was a problem hiding this comment.
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.
|
|
||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v3 |
There was a problem hiding this comment.
I think there are newer action versions available? Cf #861.
|
|
||
| // 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>, |
There was a problem hiding this comment.
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).
|
|
||
| // tokio::sync::Mutex (used for the DB client) contains UnsafeCell which opts out of | ||
| // RefUnwindSafe. std::sync::Mutex (used by SqliteStore) doesn't have this issue because | ||
| // it poisons on panic. This impl is needed for do_read_write_remove_list_persist which |
There was a problem hiding this comment.
Okay, but then we should probably cfg(test) this, right?
| .build() | ||
| .unwrap(); | ||
|
|
||
| let connection_string = connection_string.to_string(); |
There was a problem hiding this comment.
Why not take a String if we'd allocate anyways? It would at least avoid a re-allocation if users held a String already.
| let kv_table_name = kv_table_name.unwrap_or(DEFAULT_KV_TABLE_NAME.to_string()); | ||
|
|
||
| let (client, connection) = | ||
| tokio_postgres::connect(connection_string, NoTls).await.map_err(|e| { |
There was a problem hiding this comment.
We probably should support TLS from the getgo?
Also, we'll want users to pick non-default databases and create them if they don't exist yet. Note for this to work you'll first need to create a connection with the default, to then create the database you'll have in mind (see lightningdevkit/vss-server#55 and follow-ups for reference).
|
|
||
| // Spawn the connection task so it runs in the background. | ||
| tokio::spawn(async move { | ||
| if let Err(e) = connection.await { |
There was a problem hiding this comment.
How will we recover from connection errors? Should this somehow loop or similar?
| ) -> io::Result<()> { | ||
| check_namespace_key_validity(primary_namespace, secondary_namespace, Some(key), "write")?; | ||
|
|
||
| self.execute_locked_write(inner_lock_ref, locking_key, version, async move || { |
There was a problem hiding this comment.
Super annoying that we have that replicated three times (at least) by now. We should look to DRY this up, when upstreaming to lightning-persister at the very latest.
| ) -> io::Result<Vec<u8>> { | ||
| check_namespace_key_validity(primary_namespace, secondary_namespace, Some(key), "read")?; | ||
|
|
||
| let locked_client = self.client.lock().await; |
There was a problem hiding this comment.
If all operations take a MutexGuard and await the operation anyways, do we really need the write-lock-versioning? Isn't this practically sync?
|
|
||
| /// Builds a [`Node`] instance with a [`SqliteStore`] backend and according to the options | ||
| /// previously configured. | ||
| #[cfg(feature = "sqlite")] |
There was a problem hiding this comment.
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?
3 participants
Add a PostgresStore implementation behind the
postgresfeature flag, mirroring the existingSqliteStore. Usestokio-postgres(async-native) with an internal tokio runtime for the syncKVStoreSynctrait, following theVssStorepattern.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