Lock Postgres stores on initialization - #1012
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
| 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, |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
If we go ahead with advisory locks for now, maybe some form of monitoring can be added?
There was a problem hiding this comment.
Yeah if we're going with this, will add it so we attempt to reacquire the lock if it drops
|
@joostjager Mind sharing again why you think this PR is not sufficient for a first stopgap measure for v0.8/LDK Server v0.1? From a first impression it looks like the kind of minimally invasive thing we want to land right now, before we can then take time to discuss the right approach / review #1000 etc. |
|
The reason it is not sufficient is above in #1012 (comment). Maybe there are other edge cases that can happen with a connection-level lock. If it is not possible to get the safer fix in and postgres must be included in the release, the advisory lock is of course better than no lock. |
|
Let's move forward with this minimalistic approach for v0.8, and do the more thorough #1000 in the v0.9 release cycle. |
| /// | ||
| /// 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 |
There was a problem hiding this comment.
Let's make these docs a more explicit warning that it's generally discouraged and considered unsafe to point multiple instances towards the same backend. It should be clear that it's the users responsibility to avoid this for now, while we do have some stop gaps in place.
c5e7bab to
270a7d9
Compare
|
Added it so it'll reacquire the lock if we lose it. |
|
Do we already need to think how we can upgrade from this intermediary solution later? |
| self.reacquire_store_lock(&mut lock_client).await | ||
| } | ||
|
|
||
| async fn reacquire_store_lock(&self, lock_client: &mut ClientConnection) -> io::Result<()> { |
There was a problem hiding this comment.
The whole concept of reacquire sounds unsafe. If the node continued running with it's in-memory state, and another instance had the lock in the mean time and wrote data, we have a problem? #1000 hard-exits in this case, and a process restart can reload the latest state then.
There was a problem hiding this comment.
Changed so we panic on loss of the lock
Prevent multiple nodes from opening the same PostgreSQL database table at once while allowing separate database and table pairs to coexist. Retain the session-scoped advisory lock for the store lifetime, if we lose the lock, panic to prevent mismatched states. This change was created with OpenAI Codex.
270a7d9 to
6271185
Compare
4 participants
If we aren't going to get #1000 in before the release, we can add a simple lock on the postgres using its native
pg_try_advisory_lock.Prevent multiple nodes from opening the same PostgreSQL database table at once while allowing separate database and table pairs to coexist. Retain the session-scoped advisory lock for the store lifetime.