Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -695,6 +695,8 @@ impl NodeBuilder {
///
/// The given `kv_table_name` will be used or default to
/// [`DEFAULT_KV_TABLE_NAME`](io::postgres_store::DEFAULT_KV_TABLE_NAME).
/// Building fails if another PostgreSQL-backed node using the same database and table is still
/// alive. Nodes using a different database or table on the same server may coexist.
///
/// If `certificate_pem` is `Some`, TLS will be used for database connections and the
/// provided PEM-encoded CA certificate will be added to the system's default root
Expand Down Expand Up @@ -1229,6 +1231,8 @@ impl ArcedNodeBuilder {
///
/// The given `kv_table_name` will be used or default to
/// [`DEFAULT_KV_TABLE_NAME`](io::postgres_store::DEFAULT_KV_TABLE_NAME).
/// Building fails if another PostgreSQL-backed node using the same database and table is still
/// alive. Nodes using a different database or table on the same server may coexist.
///
/// If `certificate_pem` is `Some`, TLS will be used for database connections and the
/// provided PEM-encoded CA certificate will be added to the system's default root
Expand Down
70 changes: 67 additions & 3 deletions src/io/postgres_store/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use std::future::Future;
use std::sync::atomic::{AtomicU64, Ordering};
use std::sync::{Arc, Mutex};

use bitcoin::hashes::{sha256, Hash, HashEngine};
use lightning::io;
use lightning::util::persist::{
KVStore, MigratableKVStore, PageToken, PaginatedKVStore, PaginatedListResponse,
Expand Down Expand Up @@ -44,6 +45,18 @@ const PAGE_SIZE: usize = 50;
// Keep this small while still allowing progress if one runtime worker blocks on sync store access.
const INTERNAL_RUNTIME_WORKERS: usize = 2;

fn advisory_lock_id(db_name: &str, kv_table_name: &str) -> i64 {
let mut engine = sha256::Hash::engine();
engine.input(b"ldk-node:postgres-store");
for component in [db_name, kv_table_name] {
engine.input(&(component.len() as u64).to_be_bytes());
engine.input(component.as_bytes());
}

let hash = sha256::Hash::from_engine(engine).to_byte_array();
i64::from_be_bytes(hash[..8].try_into().expect("SHA-256 prefix has the expected length"))
}

fn sql_identifier(identifier: &str) -> io::Result<String> {
if identifier.is_empty() || identifier.contains('\0') {
return Err(io::Error::new(
Expand Down Expand Up @@ -128,6 +141,8 @@ impl PostgresStore {
/// the default `postgres` database to create it.
///
/// The given `kv_table_name` will be used or default to [`DEFAULT_KV_TABLE_NAME`].
/// Construction fails if another [`PostgresStore`] using the same database and table is still
/// alive. Stores using a different database or table on the same PostgreSQL server may coexist.
///
/// If `certificate_pem` is `Some`, TLS will be used for database connections and the
/// provided PEM-encoded CA certificate will be added to the system's default root
Expand Down Expand Up @@ -373,6 +388,9 @@ impl MigratableKVStore for PostgresStore {

struct PostgresStoreInner {
pool: SmallPool,
// PostgreSQL advisory locks are session-scoped, so keep the connection that acquired our lock
// alive for the lifetime of the store.
_lock_client: ClientConnection,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Fail closed when the lock session disconnects

This client is retained but never monitored. If its PostgreSQL session ends, the advisory lock is released while the independent pool can reconnect and continue serving operations. A second store can then acquire the lock while this store resumes writing. Please treat lock-session loss as terminal before any further operation, or otherwise reacquire and validate ownership without allowing stale writes. A regression test should terminate this backend, start a replacement store, and verify that the original store cannot operate.

config: Config,
kv_table_name_sql: String,
tls: PgTlsConnector,
Expand Down Expand Up @@ -426,6 +444,23 @@ impl PostgresStoreInner {
Self::create_database_if_not_exists(&config, &tls, logger.as_deref()).await?;

let client = make_config_connection(&config, &tls).await?;
let lock_id = advisory_lock_id(&db_name, &kv_table_name);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Derive the lock from the canonical schema and table identity

This hashes the configured table string, so ldk_data and public.ldk_data normally produce different lock IDs despite resolving to the same physical table. Both stores can consequently initialize and write concurrently.

Please parse the table into optional schema and table components, resolve an omitted schema using current_schema(), and derive the lock from the actual database OID, schema OID, and table component. This identity is available before the table exists, allowing the lock to remain ahead of table creation and persisted-state reads. Please also reject identifier components exceeding PostgreSQL's max_identifier_length, since PostgreSQL otherwise truncates them and can make distinct strings resolve to the same relation.

Add an integration test opening the same table through qualified and unqualified names and verify that the second store receives AlreadyExists.

--
Not sure if we need to go this far... it is easy to oversee though when code changes

let row = client.query_one("SELECT pg_try_advisory_lock($1)", &[&lock_id]).await.map_err(
|e| {
let msg = format!(
"Failed to acquire PostgreSQL store lock for database {db_name} and table {kv_table_name}: {e}"
);
io::Error::new(io::ErrorKind::Other, msg)
},
)?;
if !row.get::<_, bool>(0) {
return Err(io::Error::new(
io::ErrorKind::AlreadyExists,
format!(
"PostgreSQL store for database {db_name} and table {kv_table_name} is already in use"
),
));
}

// Create the KV data table if it doesn't exist. `sort_order` uses BIGSERIAL so
// the database assigns a fresh, monotonically increasing value on each INSERT and
Expand Down Expand Up @@ -502,12 +537,18 @@ impl PostgresStoreInner {
io::Error::new(io::ErrorKind::Other, msg)
})?;

// Drop the setup client; the pool builds its own POOL_SIZE fresh connections.
drop(client);
let pool = SmallPool::new(&config, &tls).await?;

let write_version_locks = Mutex::new(HashMap::new());
Ok(Self { pool, config, kv_table_name_sql, tls, write_version_locks, logger })
Ok(Self {
pool,
_lock_client: client,
config,
kv_table_name_sql,
tls,
write_version_locks,
logger,
})
}

async fn create_database_if_not_exists(
Expand Down Expand Up @@ -927,6 +968,29 @@ mod tests {
assert!(sql_table_identifier("schema.").is_err());
}

#[test]
fn test_postgres_advisory_lock_id_uses_database_and_table() {
let lock_id = advisory_lock_id("database_a", "table_a");
assert_eq!(lock_id, advisory_lock_id("database_a", "table_a"));
assert_ne!(lock_id, advisory_lock_id("database_b", "table_a"));
assert_ne!(lock_id, advisory_lock_id("database_a", "table_b"));
}

#[tokio::test(flavor = "multi_thread")]
async fn test_postgres_store_advisory_lock() {
let table_name = "test_pg_advisory_lock";
let store = create_test_store(table_name).await;

let err =
PostgresStore::new(test_connection_string(), None, Some(table_name.to_string()), None)
.await
.err()
.expect("a second store using the same database and table must fail");
assert_eq!(err.kind(), io::ErrorKind::AlreadyExists);

cleanup_store(&store).await;
}

#[tokio::test(flavor = "multi_thread")]
async fn read_write_remove_list_persist() {
let store = create_test_store("test_rwrl").await;
Expand Down
Loading