Update docs and add more tests for edgecase

libs/server/Storage/Functions/MainStore/PrivateMethods.cs

@@ -36,7 +36,8 @@ static void CopyTo(ref SpanByte src, ref SpanByteAndMemory dst, MemoryPool<byte>
 
         void CopyRespTo(ref SpanByte src, ref SpanByteAndMemory dst, int start = 0, int end = -1)
         {
-            int srcLength = end == -1 ? src.LengthWithoutMetadata : Math.Max(end - start, 0);
+            // src length of the value indicating no end is supplied defaults to lengthWithoutMetadata, else it chooses the bigger of 0 or (end - start)
+            int srcLength = end == -1 ? src.LengthWithoutMetadata : ((start < end) ? (end - start) : 0);
             if (srcLength == 0)
             {
                 CopyDefaultResp(CmdStrings.RESP_EMPTY, ref dst);
@@ -82,7 +83,7 @@ void CopyRespTo(ref SpanByte src, ref SpanByteAndMemory dst, int start = 0, int
             }
         }
 
-        void CopyRespToWithInput(ref SpanByte input, ref SpanByte value, ref SpanByteAndMemory dst, bool isFromPending, int payloadEtagEnd, int etagIgnoredDataEnd, bool hasEtagInVal)
+        void CopyRespToWithInput(ref SpanByte input, ref SpanByte value, ref SpanByteAndMemory dst, bool isFromPending, int payloadEtagAccountedEndOffset, int etagAccountedEnd, bool hasEtagInVal)
         {
             var inputPtr = input.ToPointer();
             switch ((RespCommand)(*inputPtr))
@@ -93,7 +94,7 @@ void CopyRespToWithInput(ref SpanByte input, ref SpanByte value, ref SpanByteAnd
                     // This is accomplished by calling ConvertToHeap on the destination SpanByteAndMemory
                     if (isFromPending)
                         dst.ConvertToHeap();
-                    CopyRespTo(ref value, ref dst, payloadEtagEnd, etagIgnoredDataEnd);
+                    CopyRespTo(ref value, ref dst, payloadEtagAccountedEndOffset, etagAccountedEnd);
                     break;
 
                 case RespCommand.MIGRATE:
@@ -123,38 +124,38 @@ void CopyRespToWithInput(ref SpanByte input, ref SpanByte value, ref SpanByteAnd
                     // Get value without RESP header; exclude expiration
                     if (value.LengthWithoutMetadata <= dst.Length)
                     {
-                        dst.Length = value.LengthWithoutMetadata - payloadEtagEnd;
-                        value.AsReadOnlySpan(payloadEtagEnd).CopyTo(dst.SpanByte.AsSpan());
+                        dst.Length = value.LengthWithoutMetadata - payloadEtagAccountedEndOffset;
+                        value.AsReadOnlySpan(payloadEtagAccountedEndOffset).CopyTo(dst.SpanByte.AsSpan());
                         return;
                     }
 
                     dst.ConvertToHeap();
-                    dst.Length = value.LengthWithoutMetadata - payloadEtagEnd;
+                    dst.Length = value.LengthWithoutMetadata - payloadEtagAccountedEndOffset;
                     dst.Memory = functionsState.memoryPool.Rent(value.LengthWithoutMetadata);
-                    value.AsReadOnlySpan(payloadEtagEnd).CopyTo(dst.Memory.Memory.Span);
+                    value.AsReadOnlySpan(payloadEtagAccountedEndOffset).CopyTo(dst.Memory.Memory.Span);
                     break;
 
                 case RespCommand.GETBIT:
-                    byte oldValSet = BitmapManager.GetBit(inputPtr + RespInputHeader.Size, value.ToPointer() + payloadEtagEnd, value.Length - payloadEtagEnd);
+                    byte oldValSet = BitmapManager.GetBit(inputPtr + RespInputHeader.Size, value.ToPointer() + payloadEtagAccountedEndOffset, value.Length - payloadEtagAccountedEndOffset);
                     if (oldValSet == 0)
                         CopyDefaultResp(CmdStrings.RESP_RETURN_VAL_0, ref dst);
                     else
                         CopyDefaultResp(CmdStrings.RESP_RETURN_VAL_1, ref dst);
                     break;
 
                 case RespCommand.BITCOUNT:
-                    long count = BitmapManager.BitCountDriver(inputPtr + RespInputHeader.Size, value.ToPointer() + payloadEtagEnd, value.Length - payloadEtagEnd);
+                    long count = BitmapManager.BitCountDriver(inputPtr + RespInputHeader.Size, value.ToPointer() + payloadEtagAccountedEndOffset, value.Length - payloadEtagAccountedEndOffset);
                     CopyRespNumber(count, ref dst);
                     break;
 
                 case RespCommand.BITPOS:
-                    long pos = BitmapManager.BitPosDriver(inputPtr + RespInputHeader.Size, value.ToPointer() + payloadEtagEnd, value.Length - payloadEtagEnd);
+                    long pos = BitmapManager.BitPosDriver(inputPtr + RespInputHeader.Size, value.ToPointer() + payloadEtagAccountedEndOffset, value.Length - payloadEtagAccountedEndOffset);
                     *(long*)dst.SpanByte.ToPointer() = pos;
                     CopyRespNumber(pos, ref dst);
                     break;
 
                 case RespCommand.BITOP:
-                    IntPtr bitmap = (IntPtr)value.ToPointer() + payloadEtagEnd;
+                    IntPtr bitmap = (IntPtr)value.ToPointer() + payloadEtagAccountedEndOffset;
                     byte* output = dst.SpanByte.ToPointer();
 
                     *(long*)output = (long)bitmap.ToInt64();
@@ -165,36 +166,36 @@ void CopyRespToWithInput(ref SpanByte input, ref SpanByte value, ref SpanByteAnd
                 case RespCommand.BITFIELD:
                     long retValue = 0;
                     bool overflow;
-                    (retValue, overflow) = BitmapManager.BitFieldExecute(inputPtr + RespInputHeader.Size, value.ToPointer() + payloadEtagEnd, value.Length - payloadEtagEnd);
+                    (retValue, overflow) = BitmapManager.BitFieldExecute(inputPtr + RespInputHeader.Size, value.ToPointer() + payloadEtagAccountedEndOffset, value.Length - payloadEtagAccountedEndOffset);
                     if (!overflow)
                         CopyRespNumber(retValue, ref dst);
                     else
                         CopyDefaultResp(CmdStrings.RESP_ERRNOTFOUND, ref dst);
                     return;
 
                 case RespCommand.PFCOUNT:
-                    if (!HyperLogLog.DefaultHLL.IsValidHYLL(value.ToPointer() + payloadEtagEnd, value.Length - payloadEtagEnd))
+                    if (!HyperLogLog.DefaultHLL.IsValidHYLL(value.ToPointer() + payloadEtagAccountedEndOffset, value.Length - payloadEtagAccountedEndOffset))
                     {
                         *(long*)dst.SpanByte.ToPointer() = -1;
                         return;
                     }
 
                     long E = 13;
-                    E = HyperLogLog.DefaultHLL.Count(value.ToPointer() + payloadEtagEnd);
+                    E = HyperLogLog.DefaultHLL.Count(value.ToPointer() + payloadEtagAccountedEndOffset);
                     *(long*)dst.SpanByte.ToPointer() = E;
                     return;
 
                 case RespCommand.PFMERGE:
-                    if (!HyperLogLog.DefaultHLL.IsValidHYLL(value.ToPointer() + payloadEtagEnd, value.Length - payloadEtagEnd))
+                    if (!HyperLogLog.DefaultHLL.IsValidHYLL(value.ToPointer() + payloadEtagAccountedEndOffset, value.Length - payloadEtagAccountedEndOffset))
                     {
                         *(long*)dst.SpanByte.ToPointer() = -1;
                         return;
                     }
 
-                    if (value.Length - payloadEtagEnd <= dst.Length)
+                    if (value.Length - payloadEtagAccountedEndOffset <= dst.Length)
                     {
-                        Buffer.MemoryCopy(value.ToPointer() + payloadEtagEnd, dst.SpanByte.ToPointer(), value.Length - payloadEtagEnd, value.Length - payloadEtagEnd);
-                        dst.SpanByte.Length = value.Length - payloadEtagEnd;
+                        Buffer.MemoryCopy(value.ToPointer() + payloadEtagAccountedEndOffset, dst.SpanByte.ToPointer(), value.Length - payloadEtagAccountedEndOffset, value.Length - payloadEtagAccountedEndOffset);
+                        dst.SpanByte.Length = value.Length - payloadEtagAccountedEndOffset;
                         return;
                     }
                     throw new GarnetException("Not enough space in PFMERGE buffer");
@@ -210,12 +211,12 @@ void CopyRespToWithInput(ref SpanByte input, ref SpanByte value, ref SpanByteAnd
                     return;
 
                 case RespCommand.GETRANGE:
-                    int len = value.LengthWithoutMetadata - payloadEtagEnd;
+                    int len = value.LengthWithoutMetadata - payloadEtagAccountedEndOffset;
                     int start = *(int*)(inputPtr + RespInputHeader.Size);
                     int end = *(int*)(inputPtr + RespInputHeader.Size + 4);
 
                     (start, end) = NormalizeRange(start, end, len);
-                    CopyRespTo(ref value, ref dst, start + payloadEtagEnd, end + payloadEtagEnd);
+                    CopyRespTo(ref value, ref dst, start + payloadEtagAccountedEndOffset, end + payloadEtagAccountedEndOffset);
                     return;
                 case RespCommand.SETIFMATCH:
                     // extract ETAG, write as long into dst, and then value

libs/server/Storage/Functions/MainStore/VarLenInputMethods.cs

@@ -112,7 +112,7 @@ public int GetRMWModifiedValueLength(ref SpanByte t, ref SpanByte input, bool ha
                 var inputspan = input.AsSpan();
                 var inputPtr = input.ToPointer();
                 var cmd = inputspan[0];
-                int etagOffset = hasEtag ? 8 : 0;
+                int etagOffset = hasEtag ? sizeof(long) : 0;
                 bool retainEtag = ((RespInputHeader*)inputPtr)->CheckRetainEtagFlag();
 
                 switch ((RespCommand)cmd)

test/Garnet.test/Resp/RespReadUtilsTests.cs

@@ -295,10 +295,10 @@ public static unsafe void ReadBoolWithLengthHeaderTest(string text, bool expecte
         /// </summary>
         [TestCase("", false, null)] // Too short
         [TestCase("S$-1\r\n", false, "S")] // Long enough but not nil leading
-        [TestCase("$-1\n1738\r\n", false, "1")] // Long enough but not nil
+        [TestCase("$-1\n1738\r\n", false, "\n")] // Long enough but not nil
         [TestCase("$-1\r\n", true, null)] // exact nil
         [TestCase("$-1\r\nxyzextra", true, null)] // leading nil but with extra bytes after
-        public static unsafe void ReadBoolWithLengthHeaderTest(string testSequence, bool expected, string firstMismatch)
+        public static unsafe void ReadNilTest(string testSequence, bool expected, string firstMismatch)
         {
             ReadOnlySpan<byte> testSeq = new ReadOnlySpan<byte>(Encoding.ASCII.GetBytes(testSequence));
 

test/Garnet.test/RespEtagTests.cs

@@ -1911,6 +1911,37 @@ public void HyperLogLogCommandsShouldReturnWrongTypeErrorForEtagSetData()
             ClassicAssert.AreEqual(Encoding.ASCII.GetString(CmdStrings.RESP_ERR_WRONG_TYPE_HLL), ex.Message);
         }
 
+        [Test]
+        public void SetWithRetainEtagOnANewUpsertWillCreateKeyValueWithoutEtag()
+        {
+            using var redis = ConnectionMultiplexer.Connect(TestUtils.GetConfig());
+            var db = redis.GetDatabase(0);
+
+            string key = "mickey";
+            string val = "mouse";
+
+            // a new upsert on a non-existing key will retain the "nil" etag
+            db.Execute("SET", [key, val, "RETAINETAG"]).ToString();
+
+            RedisResult[] res = (RedisResult[])db.Execute("GETWITHETAG", [key]);
+            RedisResult etag = res[0];
+            string value = res[1].ToString();
+
+            ClassicAssert.IsTrue(etag.IsNull);
+            ClassicAssert.AreEqual(val, value);
+
+            string newval = "clubhouse";
+
+            // a new upsert on an existing key will retain the "nil" etag from the prev
+            db.Execute("SET", [key, newval, "RETAINETAG"]).ToString();
+            res = (RedisResult[])db.Execute("GETWITHETAG", [key]);
+            etag = res[0];
+            value = res[1].ToString();
+
+            ClassicAssert.IsTrue(etag.IsNull);
+            ClassicAssert.AreEqual(newval, value);
+        }
+
         #endregion
     }
 }

website/docs/commands/etag-commands.md

@@ -31,8 +31,6 @@ Inserts a key-value string pair into Garnet, associating an ETag that will be up
 
 #### **Response**
 
-One of the following:
-
 - **Integer reply**: A response integer indicating the initial ETag value on success.
 
 ---
@@ -72,7 +70,7 @@ One of the following:
 
 - **Integer reply**: The updated ETag if the value was successfully updated.
 - **Nil reply**: If the key does not exist.
-- **Error reply (ETag mismatch)**: If the provided ETag does not match the current ETag. If the command is called on a record without an ETag we will return ETag mismatch as well.
+- **Simple string reply**: If the provided ETag does not match the current ETag or If the command is called on a record without an ETag a simple string indicating ETag mismatch is returned.
 
 ---
 
@@ -92,55 +90,20 @@ One of the following:
 
 - **Array reply**: If the ETag does not match, an array of two items is returned. The first item is the updated ETag, and the second item is the value associated with the key. If called on a record without an ETag the first item in the array will be nil.
 - **Nil reply**: If the key does not exist.
-- **Simple string reply**: Returns a string indicating the value is unchanged if the provided ETag matches the current ETag.
+- **Simple string reply**: if the provided ETag matches the current ETag, returns a simple string indicating the value is unchanged.
 
 ---
 
 ## Compatibility and Behavior with Non-ETag Commands
 
-ETag commands executed on keys that were not set with `SETWITHETAG` will return a type mismatch error. Additionally, invoking `SETWITHETAG` on an existing key will overwrite the key-value pair and reset the associated ETag.
-
 Below is the expected behavior of ETag-associated key-value pairs when non-ETag commands are used.
 
 - **MSET, BITOP**: These commands will replace an existing ETag-associated key-value pair with a non-ETag key-value pair, effectively removing the ETag.
-- **SET**: If only if used with additional option "RETAINETAG" will update the etag while inserting the key-value pair over the existing key-value pair.
-- **RENAME**: Renaming an ETag-associated key-value pair will reset the ETag to 0 for the renamed key.
 
----
+- **SET**: Only if used with additional option "RETAINETAG" will calling SET update the etag while inserting the new key-value pair over the existing key-value pair.
+
+- **RENAME**: Renaming an ETag-associated key-value pair will reset the ETag to 0 for the renamed key. Unless the key being renamed to already existed before hand, in that case it will retain the etag of the existing key that was the target of the rename.
 
-### **Same Behavior as Non-ETag Key-Value Pairs**
-
-The following commands do not expose the ETag to the user and behave the same as non-ETag key-value pairs. From the user's perspective, there is no indication that a key-value pair is associated with an ETag.
-
-- **GET**
-- **DEL**
-- **EXISTS**
-- **EXPIRE**
-- **PEXPIRE**
-- **PERSIST**
-- **GETRANGE**
-- **TTL**
-- **PTTL**
-- **GETDEL**
-- **STRLEN**
-- **GETBIT**
-- **BITCOUNT**
-- **BITPOS**
-- **BITFIELD_RO**
-
-### **Commands That Update ETag Internally**
-
-The following commands update the underlying data and consequently update the ETag of the key-value pair. However, the new ETag will not be exposed to the user until explicitly retrieved via an ETag-related command.
-
-- **SETRANGE**
-- **APPEND**
-- **INCR**
-- **INCRBY**
-- **DECR**
-- **DECRBY**
-- **SETBIT**
-- **UNLINK**
-- **MGET**
-- **BITFIELD**
+All other commands will update the etag internally if they modify the underlying data, and any responses from them will not expose the etag to the client. To the users the etag and it's updates remain hidden in non-etag commands.
 
 ---

website/docs/commands/raw-string.md

@@ -203,7 +203,7 @@ Set **key** to hold the string value. If key already holds a value, it is overwr
 * NX -- Only set the key if it does not already exist.
 * XX -- Only set the key if it already exists.
 * KEEPTTL -- Retain the time to live associated with the key.
-* RETAINETAG -- Update the Etag associated with the previous key-value pair, while setting the new value for the key. If no etag existed on the previous key-value pair this initialize one. 
+* RETAINETAG -- Update the Etag associated with the previous key-value pair, while setting the new value for the key. If no etag existed on the previous key-value pair this will create the new key-value pair without any etag as well.
 
 #### Resp Reply