[Compatibility] Adding RENAMENX command (#661)

* Inital changes (Doesn't build)

* New changes from rename

* Temp changes for calling SET_Conditional

* Added basic test

* Updated CommandsInfo

* Changes done for non object keys

* Final commit

* Added proper comments

* Code format fix

* Added ACL test for renamenx

* Corrected the function name

* Fix for review comments

* Fixed code styling issue

---------

Co-authored-by: Tal Zaccai <[email protected]>
Co-authored-by: Yoganand Rajasekaran <[email protected]>

Author: 3 people

libs/server/API/GarnetApi.cs

@@ -154,6 +154,10 @@ public GarnetStatus APPEND(ArgSlice key, ArgSlice value, ref ArgSlice output)
         /// <inheritdoc />
         public GarnetStatus RENAME(ArgSlice oldKey, ArgSlice newKey, StoreType storeType = StoreType.All)
             => storageSession.RENAME(oldKey, newKey, storeType);
+
+        /// <inheritdoc />
+        public GarnetStatus RENAMENX(ArgSlice oldKey, ArgSlice newKey, out int result, StoreType storeType = StoreType.All)
+            => storageSession.RENAMENX(oldKey, newKey, storeType, out result);
         #endregion
 
         #region EXISTS

libs/server/API/IGarnetApi.cs

@@ -118,6 +118,16 @@ public interface IGarnetApi : IGarnetReadApi, IGarnetAdvancedApi
         /// <param name="storeType"></param>
         /// <returns></returns>
         GarnetStatus RENAME(ArgSlice oldKey, ArgSlice newKey, StoreType storeType = StoreType.All);
+
+        /// <summary>
+        /// Renames key to newkey if newkey does not yet exist. It returns an error when key does not exist.
+        /// </summary>
+        /// <param name="oldKey">The old key to be renamed.</param>
+        /// <param name="newKey">The new key name.</param>
+        /// <param name="result">The result of the operation.</param>
+        /// <param name="storeType">The type of store to perform the operation on.</param>
+        /// <returns></returns>
+        GarnetStatus RENAMENX(ArgSlice oldKey, ArgSlice newKey, out int result, StoreType storeType = StoreType.All);
         #endregion
 
         #region EXISTS

libs/server/Resp/KeyAdminCommands.cs

@@ -40,6 +40,37 @@ private bool NetworkRENAME<TGarnetApi>(ref TGarnetApi storageApi)
             return true;
         }
 
+        /// <summary>
+        /// TryRENAMENX
+        /// </summary>
+        private bool NetworkRENAMENX<TGarnetApi>(ref TGarnetApi storageApi)
+            where TGarnetApi : IGarnetApi
+        {
+            if (parseState.Count != 2)
+            {
+                return AbortWithWrongNumberOfArguments(nameof(RespCommand.RENAMENX));
+            }
+
+            var oldKeySlice = parseState.GetArgSliceByRef(0);
+            var newKeySlice = parseState.GetArgSliceByRef(1);
+            var status = storageApi.RENAMENX(oldKeySlice, newKeySlice, out var result);
+
+            if (status == GarnetStatus.OK)
+            {
+                // Integer reply: 1 if key was renamed to newkey.
+                // Integer reply: 0 if newkey already exists.
+                while (!RespWriteUtils.WriteInteger(result, ref dcurr, dend))
+                    SendAndReset();
+            }
+            else
+            {
+                while (!RespWriteUtils.WriteError(CmdStrings.RESP_ERR_GENERIC_NOSUCHKEY, ref dcurr, dend))
+                    SendAndReset();
+            }
+
+            return true;
+        }
+
         /// <summary>
         /// GETDEL command processor
         /// </summary>

libs/server/Resp/Parser/RespCommand.cs

@@ -117,6 +117,7 @@ public enum RespCommand : byte
         PFMERGE,
         PSETEX,
         RENAME,
+        RENAMENX,
         RPOP,
         RPOPLPUSH,
         RPUSH,
@@ -620,6 +621,7 @@ private RespCommand FastParseCommand(out int count)
                         (2 << 4) | 6 when lastWord == MemoryMarshal.Read<ulong>("INCRBY\r\n"u8) => RespCommand.INCRBY,
                         (2 << 4) | 6 when lastWord == MemoryMarshal.Read<ulong>("DECRBY\r\n"u8) => RespCommand.DECRBY,
                         (2 << 4) | 6 when lastWord == MemoryMarshal.Read<ulong>("RENAME\r\n"u8) => RespCommand.RENAME,
+                        (2 << 4) | 8 when lastWord == MemoryMarshal.Read<ulong>("NAMENX\r\n"u8) && *(ushort*)(ptr + 8) == MemoryMarshal.Read<ushort>("RE"u8) => RespCommand.RENAMENX,
                         (2 << 4) | 6 when lastWord == MemoryMarshal.Read<ulong>("GETBIT\r\n"u8) => RespCommand.GETBIT,
                         (2 << 4) | 6 when lastWord == MemoryMarshal.Read<ulong>("APPEND\r\n"u8) => RespCommand.APPEND,
                         (2 << 4) | 7 when lastWord == MemoryMarshal.Read<ulong>("UBLISH\r\n"u8) && ptr[8] == 'P' => RespCommand.PUBLISH,

libs/server/Resp/RespCommandsInfo.json

@@ -3573,6 +3573,49 @@
     ],
     "SubCommands": null
   },
+  {
+    "Command": "RENAMENX",
+    "Name": "RENAMENX",
+    "IsInternal": false,
+    "Arity": 3,
+    "Flags": "Fast, Write",
+    "FirstKey": 1,
+    "LastKey": 2,
+    "Step": 1,
+    "AclCategories": "Fast, KeySpace, Write",
+    "Tips": null,
+    "KeySpecifications": [
+      {
+        "BeginSearch": {
+          "TypeDiscriminator": "BeginSearchIndex",
+          "Index": 1
+        },
+        "FindKeys": {
+          "TypeDiscriminator": "FindKeysRange",
+          "LastKey": 0,
+          "KeyStep": 1,
+          "Limit": 0
+        },
+        "Notes": null,
+        "Flags": "RW, Access, Delete"
+      },
+      {
+        "BeginSearch": {
+          "TypeDiscriminator": "BeginSearchIndex",
+          "Index": 2
+        },
+        "FindKeys": {
+          "TypeDiscriminator": "FindKeysRange",
+          "LastKey": 0,
+          "KeyStep": 1,
+          "Limit": 0
+        },
+        "Notes": null,
+        "Flags": "OW, Insert"
+      }
+    ],
+    "SubCommands": null
+  },
   {
     "Command": "REPLICAOF",
     "Name": "REPLICAOF",

libs/server/Resp/RespServerSession.cs

@@ -512,6 +512,7 @@ private bool ProcessBasicCommands<TGarnetApi>(RespCommand cmd, ref TGarnetApi st
                 RespCommand.SETEXNX => NetworkSETEXNX(ref storageApi),
                 RespCommand.DEL => NetworkDEL(ref storageApi),
                 RespCommand.RENAME => NetworkRENAME(ref storageApi),
+                RespCommand.RENAMENX => NetworkRENAMENX(ref storageApi),
                 RespCommand.EXISTS => NetworkEXISTS(ref storageApi),
                 RespCommand.EXPIRE => NetworkEXPIRE(RespCommand.EXPIRE, ref storageApi),
                 RespCommand.PEXPIRE => NetworkEXPIRE(RespCommand.PEXPIRE, ref storageApi),

libs/server/Storage/Session/MainStore/MainStoreOps.cs

@@ -521,12 +521,33 @@ public GarnetStatus DELETE<TContext, TObjectContext>(byte[] key, StoreType store
         }
 
         public unsafe GarnetStatus RENAME(ArgSlice oldKeySlice, ArgSlice newKeySlice, StoreType storeType)
+        {
+            return RENAME(oldKeySlice, newKeySlice, storeType, false, out _);
+        }
+
+        /// <summary>
+        /// Renames key to newkey if newkey does not yet exist. It returns an error when key does not exist.
+        /// </summary>
+        /// <param name="oldKeySlice">The old key to be renamed.</param>
+        /// <param name="newKeySlice">The new key name.</param>
+        /// <param name="storeType">The type of store to perform the operation on.</param>
+        /// <returns></returns>
+        public unsafe GarnetStatus RENAMENX(ArgSlice oldKeySlice, ArgSlice newKeySlice, StoreType storeType, out int result)
+        {
+            return RENAME(oldKeySlice, newKeySlice, storeType, true, out result);
+        }
+
+        private unsafe GarnetStatus RENAME(ArgSlice oldKeySlice, ArgSlice newKeySlice, StoreType storeType, bool isNX, out int result)
         {
             GarnetStatus returnStatus = GarnetStatus.NOTFOUND;
+            result = -1;
 
             // If same name check return early.
             if (oldKeySlice.ReadOnlySpan.SequenceEqual(newKeySlice.ReadOnlySpan))
+            {
+                result = 1;
                 return GarnetStatus.OK;
+            }
 
             bool createTransaction = false;
             if (txnManager.state != TxnState.Running)
@@ -570,23 +591,68 @@ public unsafe GarnetStatus RENAME(ArgSlice oldKeySlice, ArgSlice newKeySlice, St
                             // If the key has an expiration, set the new key with the expiration
                             if (expireTimeMs > 0)
                             {
-                                SETEX(newKeySlice, new ArgSlice(ptrVal, headerLength), TimeSpan.FromMilliseconds(expireTimeMs), ref context);
+                                if (isNX)
+                                {
+                                    // Move payload forward to make space for RespInputHeader and Metadata
+                                    var setValue = scratchBufferManager.FormatScratch(RespInputHeader.Size + sizeof(long), new ArgSlice(ptrVal, headerLength));
+                                    var setValueSpan = setValue.SpanByte;
+                                    var setValuePtr = setValueSpan.ToPointerWithMetadata();
+                                    setValueSpan.ExtraMetadata = DateTimeOffset.UtcNow.Ticks + TimeSpan.FromMilliseconds(expireTimeMs).Ticks;
+                                    ((RespInputHeader*)(setValuePtr + sizeof(long)))->cmd = RespCommand.SETEXNX;
+                                    ((RespInputHeader*)(setValuePtr + sizeof(long)))->flags = 0;
+                                    var newKey = newKeySlice.SpanByte;
+                                    var setStatus = SET_Conditional(ref newKey, ref setValueSpan, ref context);
+
+                                    // For SET NX `NOTFOUND` means the operation succeeded
+                                    result = setStatus == GarnetStatus.NOTFOUND ? 1 : 0;
+                                    returnStatus = GarnetStatus.OK;
+                                }
+                                else
+                                {
+                                    SETEX(newKeySlice, new ArgSlice(ptrVal, headerLength), TimeSpan.FromMilliseconds(expireTimeMs), ref context);
+                                }
                             }
                             else if (expireTimeMs == -1) // Its possible to have expireTimeMs as 0 (Key expired or will be expired now) or -2 (Key does not exist), in those cases we don't SET the new key
                             {
-                                SpanByte newKey = newKeySlice.SpanByte;
-                                var value = SpanByte.FromPinnedPointer(ptrVal, headerLength);
-                                SET(ref newKey, ref value, ref context);
+                                if (isNX)
+                                {
+                                    // Move payload forward to make space for RespInputHeader
+                                    var setValue = scratchBufferManager.FormatScratch(RespInputHeader.Size, new ArgSlice(ptrVal, headerLength));
+                                    var setValueSpan = setValue.SpanByte;
+                                    var setValuePtr = setValueSpan.ToPointerWithMetadata();
+                                    ((RespInputHeader*)setValuePtr)->cmd = RespCommand.SETEXNX;
+                                    ((RespInputHeader*)setValuePtr)->flags = 0;
+                                    var newKey = newKeySlice.SpanByte;
+                                    var setStatus = SET_Conditional(ref newKey, ref setValueSpan, ref context);
+
+                                    // For SET NX `NOTFOUND` means the operation succeeded
+                                    result = setStatus == GarnetStatus.NOTFOUND ? 1 : 0;
+                                    returnStatus = GarnetStatus.OK;
+                                }
+                                else
+                                {
+                                    SpanByte newKey = newKeySlice.SpanByte;
+                                    var value = SpanByte.FromPinnedPointer(ptrVal, headerLength);
+                                    SET(ref newKey, ref value, ref context);
+                                }
                             }
 
                             expireSpan.Memory.Dispose();
                             memoryHandle.Dispose();
                             o.Memory.Dispose();
 
-                            // Delete the old key
-                            DELETE(ref oldKey, StoreType.Main, ref context, ref objectContext);
+                            // Delete the old key only when SET NX succeeded
+                            if (isNX && result == 1)
+                            {
+                                DELETE(ref oldKey, StoreType.Main, ref context, ref objectContext);
+                            }
+                            else if (!isNX)
+                            {
+                                // Delete the old key
+                                DELETE(ref oldKey, StoreType.Main, ref context, ref objectContext);
 
-                            returnStatus = GarnetStatus.OK;
+                                returnStatus = GarnetStatus.OK;
+                            }
                         }
                     }
                 }
@@ -618,32 +684,29 @@ public unsafe GarnetStatus RENAME(ArgSlice oldKeySlice, ArgSlice newKeySlice, St
                         var valObj = value.garnetObject;
                         byte[] newKeyArray = newKeySlice.ToArray();
 
-                        var expireSpan = new SpanByteAndMemory();
-                        var ttlStatus = TTL(ref oldKey, StoreType.Object, ref expireSpan, ref context, ref objectContext, true);
-
-                        if (ttlStatus == GarnetStatus.OK && !expireSpan.IsSpanByte)
+                        returnStatus = GarnetStatus.OK;
+                        var canSetAndDelete = true;
+                        if (isNX)
                         {
-                            using var expireMemoryHandle = expireSpan.Memory.Memory.Pin();
-                            var expirePtrVal = (byte*)expireMemoryHandle.Pointer;
-                            RespReadUtils.TryRead64Int(out var expireTimeMs, ref expirePtrVal, expirePtrVal + expireSpan.Length, out var _);
-                            expireSpan.Memory.Dispose();
+                            // Not using EXISTS method to avoid new allocation of Array for key
+                            var getNewStatus = GET(newKeyArray, out _, ref objectContext);
+                            canSetAndDelete = getNewStatus == GarnetStatus.NOTFOUND;
+                        }
 
-                            if (expireTimeMs > 0)
-                            {
-                                SET(newKeyArray, valObj, ref objectContext);
-                                EXPIRE(newKeySlice, TimeSpan.FromMilliseconds(expireTimeMs), out _, StoreType.Object, ExpireOption.None, ref context, ref objectContext, true);
-                            }
-                            else if (expireTimeMs == -1) // Its possible to have expire as 0 or -2, in those cases we don't SET the new key
-                            {
-                                SET(newKeyArray, valObj, ref objectContext);
-                            }
+                        if (canSetAndDelete)
+                        {
+                            // valObj already has expiration time, so no need to write expiration logic here
+                            SET(newKeyArray, valObj, ref objectContext);
 
                             // Delete the old key
                             DELETE(oldKeyArray, StoreType.Object, ref context, ref objectContext);
 
-                            returnStatus = GarnetStatus.OK;
+                            result = 1;
+                        }
+                        else
+                        {
+                            result = 0;
                         }
-
                     }
                 }
                 finally
@@ -652,7 +715,6 @@ public unsafe GarnetStatus RENAME(ArgSlice oldKeySlice, ArgSlice newKeySlice, St
                         txnManager.Commit(true);
                 }
             }
-
             return returnStatus;
         }
 

playground/CommandInfoUpdater/SupportedCommand.cs

@@ -194,6 +194,7 @@ public class SupportedCommand
             new("READONLY", RespCommand.READONLY),
             new("READWRITE", RespCommand.READWRITE),
             new("RENAME", RespCommand.RENAME),
+            new("RENAMENX", RespCommand.RENAMENX),
             new("REPLICAOF", RespCommand.REPLICAOF),
             new("RPOP", RespCommand.RPOP),
             new("RPOPLPUSH", RespCommand.RPOPLPUSH),

test/Garnet.test/Resp/ACL/RespCommandTests.cs

@@ -4278,10 +4278,10 @@ public async Task RenameACLsAsync()
         {
             await CheckCommandsAsync(
                 "RENAME",
-                [DoPTTLAsync]
+                [DoRENAMEAsync]
             );
 
-            static async Task DoPTTLAsync(GarnetClient client)
+            static async Task DoRENAMEAsync(GarnetClient client)
             {
                 try
                 {
@@ -4300,6 +4300,33 @@ static async Task DoPTTLAsync(GarnetClient client)
             }
         }
 
+        [Test]
+        public async Task RenameNxACLsAsync()
+        {
+            await CheckCommandsAsync(
+                "RENAMENX",
+                [DoRENAMENXAsync]
+            );
+
+            static async Task DoRENAMENXAsync(GarnetClient client)
+            {
+                try
+                {
+                    await client.ExecuteForStringResultAsync("RENAMENX", ["foo", "bar"]);
+                    Assert.Fail("Shouldn't succeed, key doesn't exist");
+                }
+                catch (Exception e)
+                {
+                    if (e.Message == "ERR no such key")
+                    {
+                        return;
+                    }
+
+                    throw;
+                }
+            }
+        }
+
         [Test]
         public async Task ReplicaOfACLsAsync()
         {