valkey-io
[NEW] Provide Notification on Key Load
The problem/use-case that the feature addresses
Today, loading an RDB with defined indexes is done sequentially in two stages. In the first stage, the Valkey databases (keys) are restored. In the second phase indexes are reloaded or rebuilt.
The contents of a vector index/field are saved in the RDB, because the time to recompute them greatly exceeds the time to reload them. This is not done for the other index types because regenerating them is generally faster than save/restoring.
Even with the recently developed Multi-stream Reload code, large instances will have many idle CPUs during the reload phase. This feature request enables the Search index reconstruction to be performed during the database restore operation, effectively overlapping phase 1 and phase 2 above and substantially reducing reload times on large instances by better using idle CPUs.
Description of the feature
What's needed is key by key callback from the core when a key has been restored. Essentially, this is a request to extend the KeySpace notification system (or something equivalent) currently used by Search to capture key mutations one by one. Implicitly upon receipt of the notification, Search would use the existing Module interfaces to open the key and read out its content form the callback and then to ingest that data into the Search indexes on background threads.
If I understand the multi-thread reload description in the PR (which was excellent!) then key restorations are single threaded, but not performed by the main thread. At this time, the mainthread is dedicated to performing I/O operations, effectively allowing the background temporary exclusive use of the Valkey data structures -- essentially the background thread and the mainthread have temporarily swapped ownership of the Key databases. This should mean that the existing Key access APIs should function just fine.
Alternatives you've considered
N/A
Additional information
Any additional information that is relevant to the feature request.
What's needed is key by key callback from the core when a key has been restored
Just to clarify: this should be triggered both from rdb load AND restore command context AND check rdb context right?
If I understand the multi-thread reload description in the PR (which was excellent!) then key restorations are single threaded, but not performed by the main thread. At this time, the mainthread is dedicated to performing I/O operations, effectively allowing the background temporary exclusive use of the Valkey data structures -- essentially the background thread and the mainthread have temporarily swapped ownership of the Key databases. This should mean that the existing Key access APIs should function just fine.
@allenss-amazon. for reference I understand you refer to this PR (maybe add the link in the top comment?). My question is: IIUC the suggestion in the mutithreaded rdb load is to keep DB locks. Do we identify any risks by triggering module hooks under lock contention?
What's needed is key by key callback from the core when a key has been restored
Just to clarify: this should be triggered both from rdb load AND restore command context AND check rdb context right?
I think yes for rdb load and restore. Not sure what you mean by "check rdb context"
If I understand the multi-thread reload description in the PR (which was excellent!) then key restorations are single threaded, but not performed by the main thread. At this time, the mainthread is dedicated to performing I/O operations, effectively allowing the background temporary exclusive use of the Valkey data structures -- essentially the background thread and the mainthread have temporarily swapped ownership of the Key databases. This should mean that the existing Key access APIs should function just fine.
@allenss-amazon. for reference I understand you refer to this PR (maybe add the link in the top comment?). My question is: IIUC the suggestion in the mutithreaded rdb load is to keep DB locks. Do we identify any risks by triggering module hooks under lock contention?
Yes, that's the PR, if you look at the sections under "Detailed Design" it's clear that the DB insertion logic is single threaded, but done on a background thread. The simplest solution at that point would be to have the single-threaded insertion logic trigger a callback to search, which would synchronously grab the contents of the restored key (if it's indexed) using some API (could be the existing Openkey,HGet, Json.Get etc. APIs, or a custom set of APIs -- doesn't matter). Actually, Search would be fine with the multi-threading the ingestion in this case, as long as access to the key contents was available to the search threads.
Not sure what you mean by "check rdb context"
Sorry. was unclear. I just wanted to understand if this should be "context aware" or triggered in all cases we operate load or we should somehow propagate the context of the load. I think it might be fine to just always trigger it.
Yes, that's the PR, if you look at the sections under "Detailed Design" it's clear that the DB insertion logic is single threaded, but done on a background thread. The simplest solution at that point would be to have the single-threaded insertion logic trigger a callback to search, which would synchronously grab the contents of the restored key (if it's indexed) using some API (could be the existing Openkey,HGet, Json.Get etc. APIs, or a custom set of APIs -- doesn't matter). Actually, Search would be fine with the multi-threading the ingestion in this case, as long as access to the key contents was available to the search threads.
I was mainly referring to that part of the design:
When the buffer is full, the thread acquires the shared "Insert Lock" and adds its batch of keys to the database
Personally I was yet unable to identify an existing path where this becomes problematic, I was just pointing out that we now have a module code which will be run under shared lock contention when the main engine thread is probably not locked.
I'd say it should get trigger in all contexts. As long as there's enough server-events floating around for the Search module to understand what's going on. My reading of the documentation is that during the multi-part load, essentially the identity of the main-thread and the "background thread" is effectively inverted. This suggests to me that there's no particular reason why this background insertion thread can't masquerade as the main thread and just invoke the standard keyspace notification event machinery. This would generate a callback into Search which would then synchronously execute the KeyOpen and HGet/JSON.GET API calls to capture the updated values of the key. Search won't actually care that this is happening off of the actual main thread. All Search cares about is that the notification contains the Key Name AND that Search can synchronous grab the values from the key during the callback.