Lock Postgres stores on initialization - #1012
Conversation
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. This change was created with OpenAI Codex.
|
👋 Thanks for assigning @joostjager 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.
| 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); |
There was a problem hiding this comment.
[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
3 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.