I was thinking of implementing and making a PR that allows to scan all the keys using a module api without the need to use RM_Call. I thought its easy to do and a quick win but after talking to @oranagra I realize that its not that easy. So I wanted to consult before start implementing.

My main goal is to supply an API that allows the module writer to scan the keys in a background thread and to release the GIL from time to time so the module will not block the redis for to long. So for example if I have an operation that needs to be performed on each key but do not require to lock the GIL then I could do something like this:

GIL_LOCK();
iter = KeysIterator();
currKey = NULL;
while(currKey=next(iter)){
    GIL_UNLOCK();
    do job on currKey
    GIL_LOCK();
}

At the beginning I thought of using the Dict safeiterator, the problem with this approach is that from the moment I release the GIL to the moment I reacquire it, the key might be deleted and my iterator will be broken. So after talking to @oranagra I realize that its only safe to release the GIL once I finish to scan an entire bucket of the keys dictionary. This leaves me with 2 options: 1. Follow the scan command api. Create a function that receive a cursor and a callback and calls the callback with each key in the bucket. After finish, it returns another cursor that need to passed to it again in order to continue. It will look something like this.

static void AddKeyToList(key, privateData){
    list = privateData;
    list.add(key);
}

cursor = 0;
list = [];
GIL_LOCK();
do{
    cursor = scan(cursor, AddKeyToList, list)
    GIL_UNLOCK();
    do job on each key in list
    list = []
    GIL_LOCK();
}while(cursor)
  1. The other option is to supply an api that tells the user when its safe to stop and release the GIL, it will look something like this:
safeToStop = false
list = [];
GIL_LOCK();
iter = KeysIterator();
currKey = NULL;
while(currKey=next(iter, &safeToStop)){
    list.add(currKey);
    if(safeToStop){
        GIL_UNLOCK();
        do job on each key in list
        list = []
        GIL_LOCK();
    }
}

Appreciate feedback, honestly I am not sure which way is better. If there is more options/ideas I will be happy to hear about them.

Comment From: oranagra

One of the main concerns here is to design an API that we'll be able to support in the future, when the main dict is implemented using Radix tree. So i suppose Scan based API (although perfect at this point in time) is out of the question. we've used this in the past (before Scan existed)

#define dictIteratorResumable(iter) ((iter)->nextEntry == NULL)

Comment From: MeirShpilraien

@oranagra RE the scan, we can modified the api such that the cursor will be a pointer to an opaque struct. This way we can support it with Radix tree in the future (on the Radix tree implementation it will contains information from which key we need to continue).

Comment From: oranagra

We'll have to have the user release the iterator in case he decides to stop it before reaching the end (which isn't needed for scan, this is its main advantage). This means that we can/should, even now with scan, allocate a struct holding the cursor, and avoid returning the new cursor on each call. So what's left to decide is if the iteration is calling a callback for each key (using scan at the moment), or returns a single element each time, and provides a Resumable indication. both of these can be later supported by radix tree.

Comment From: antirez

Hello, what about just masking the scan cursor under an opaque object called RedisModuleScanCursor? For now it will hold the number returned by SCAN itself, and in the doc we'll warn the users that certain keys may be returned multiple times. Once we switch (if ever :-D) to the radix tree, we'll store the latest key fetched in the opaque cursor, without changing the API, but just updating the documentation to tell that it is no longer possible that keys are reported multiple times.

Comment From: MeirShpilraien

@antirez thanks, So if I understand you correctly you suggest to go with the first approach and make the cursor as an opaque object?

Comment From: antirez

@MeirShpilraien exactly. Would you take this and implement it? Thanks.

Comment From: MeirShpilraien

@antirez yes I will do it.

Comment From: MeirShpilraien

@antirez

After starting to look at implementation details an idea came up. We already scan the keys, so why providing only keys names and not also the values (as a RedisModuleKey*). This way a module writer who wants to scan keys and values will be able to do so without issuing another lookup for the value. This will surly increase the performance for such usecases.

I talked to @oranagra about it and the only issue we saw here is that sometimes the value might not be available. For example, once we implement the lock per key feature (threaded command execution), the key might be locked during the scan and we will not be able to get the value. In addition, on RedisOnFlash, the value might not even be on the ram. Those facts might, in the future, prevent us from continue supporting such an api.

The two solutions we thought about are:

  1. Document that values are supplied to the callback using the best efforts approach. In some cases those might not be supplied and its the module writer responsibility to get them using RM_OpenKey.
  2. Supply a flag to the scan function indicating whether or not to retrieve the values. If the flag is enable and value is not available, the scan will be blocked until value will be available.
  3. ignore this concern, and just expose the values anyway,
  4. any other reason not to expose values at all?

I honestly prefer option number one. I think we should give the module writer the freedom of deciding what to do in absence of values, maybe he do not need them right away and there is no point of blocking him (he can retry rm_OpenKey later).

what do you think?

Comment From: MeirShpilraien

@antirez any thoughts here?