Conversation
|
👋 Thanks for assigning @tankyleo as a reviewer! |
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.
|
|
||
| // 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).
There was a problem hiding this comment.
Yeah I just copied VSS to be safe.
src/io/postgres_store/mod.rs
Outdated
| 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).
| ) -> 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.
There was a problem hiding this comment.
Yeah I had same thought.
src/io/postgres_store/mod.rs
Outdated
| ) -> 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?
There was a problem hiding this comment.
Technically we'd still need this if someone is gonna do a join! on two writes to the same key. But yeah, could probably be okay to remove?
src/builder.rs
Outdated
|
|
||
| /// 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?
There was a problem hiding this comment.
okay removed for now
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>
- Add #[cfg(test)] to RefUnwindSafe impl (matches VssStore) - Take connection_string as String instead of &str - Bump actions/checkout to v6 in postgres CI workflow Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Parse the connection string to check for a dbname. If present, connect to the default "postgres" database first to create the target database if it doesn't already exist, then connect to it. Follows the pattern used by vss-server. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Check connection health before each operation using is_closed() and check_connection(). If the connection is dead, transparently create a new one. This prevents permanent failures when the PostgreSQL server restarts or the connection is otherwise dropped. Pattern modeled after vss-server's ensure_connected approach. Generated with AI (Claude Code) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Use native-tls and postgres-native-tls to support encrypted connections, following the same pattern as the VSS server. A new PostgresTlsConfig struct controls TLS behavior: - None: plaintext (existing behavior) - Some with certificate_pem: None: TLS using system root CAs - Some with certificate_pem: Some(pem): TLS with a custom CA Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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