Support command "COPY"

[pjeziorowski]

https://redis.io/commands/copy

[barkanido]

Can I own this one?

Also, for that matter, do we support logical DB's? COPY source destination [DB destination-db] [REPLACE]

[evoxmusic]

I guess yes :) it's open for long time

[barkanido]

Also, for that matter, do we support logical DB's? COPY source destination [DB destination-db] [REPLACE]

@evoxmusic ?

[evoxmusic]

I don't know if we should support logical DB right now. I would suggest supporting it once we have a first version working in cluster mode. Then we can see if it makes sense to support it. What do you think?

[barkanido]

well, afaik, logical DBs is an independent feature from clustering. E.g, users can use one of them without the other, but not both.

When using Redis Cluster, the SELECT command cannot be used, since Redis Cluster only supports database zero. In the case of a Redis Cluster, having multiple databases would be useless and an unnecessary source of complexity. Commands operating atomically on a single database would not be possible with the Redis Cluster design and goals.

I have personally never used logical DB's, but I can see why people might find them useful. The docs kind of hint that in SELECT:

In practical terms, Redis databases should be used to separate different keys belonging to the same application (if needed), and not to use a single Redis instance for multiple unrelated applications.

Saying that, it is a veteran feature (since 1.0.0). It is also pretty straightforward to implement. Until we implement it, for all commands taking a logical DB as an argument, we can implement the parsing, and fail if one enables this. COPY is an example of that case. I vote for postponing it until users actually ask it.

[barkanido]

@clarity0 @evoxmusic I need some advice here. COPY is inherently unsafe because it needs to burrow self the storage as mutable more than once:

// in run_command.rs
let mut storage = lock_then_release(storage);                            
if let Some(src) = storage.read(&copy_cmd.source) { // <- here
    if !copy_cmd.replace && storage.contains(&copy_cmd.destination) { // <- here
        RedisResponse::single(Integer(0))                            
    } else {                                                         
        storage.write(&copy_cmd.destination, src); // <- and here
        RedisResponse::single(Integer(1))                            
    }                                                                
} else {                                                             
    RedisResponse::single(Integer(0))                                
}                                                                    

implementing a

    fn copy_key(&mut self, src: &[u8], dst: &[u8], replace: bool) -> u32;

in Storage merely moves the problem into InMemoryStorage, and is also nondesirable since the trait has already read and write and so composing them seems like the right direction to go.

Can you propose a safe way to implement this?

[clarity0]

The reason read and contains methods in the current implementation need to take &mut self is that they check if the key is expired, maybe this behavior could be changed so that they return a value regardless but have another method that does the lazy garbage collection(for expired entries) outside of the actual read, and then have a wrapper method that calls the actual read and then the GC method?

[barkanido]

The reason read and contains methods in the current implementation need to take &mut self is that they check if the key is expired, maybe this behavior could be changed so that they return a value regardless but have another method that does the lazy garbage collection(for expired entries) outside of the actual read, and then have a wrapper method that calls the actual read and then the GC method?

Yeah, assume we factor out the GC method, this behavior that Redis cleans up expired entries on access is still desirable right? Even if we change its semantics, for example, marking expire objects as such and putting them in a queue, this must still happen upon access. And access can be potentially a write. My point is, that no matter how you look at it, you still need a mutable reference to the storage. My hunch here that your supposed refactoring, although a needed one anyway, will only move this inherent complexity around, not solving it.

Another possible view of this is that this is still safe to do because all this is done under a lock. The compiler just fails to understand this hence we could try to switch to runtime burrow checking (with RefCell as shown here) I have never tried it but it might work. WDYT?