Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MSETNX atomic fix #2 #996

Merged
merged 5 commits into from
Feb 5, 2025
Merged

MSETNX atomic fix #2 #996

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a8dd5fb
MSETNX should be atomic and not set any key if any one of them exists.
Jan 30, 2025
da9b363
Lets not forget the object case too.
Feb 4, 2025
9785069
The one run failing in one unrelated test (CanDoHashExpire) seems spu…
Feb 4, 2025
a80d349
Merge branch 'main' into msetnxver2
badrishc Feb 5, 2025
f3e3d16
Merge branch 'main' into msetnxver2
badrishc Feb 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions libs/server/API/GarnetApi.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,12 @@ public GarnetStatus SETRANGE(ArgSlice key, ref RawStringInput input, ref ArgSlic

#endregion

#region MSETNX
/// <inheritdoc />
public GarnetStatus MSET_Conditional(ref RawStringInput input) =>
storageSession.MSET_Conditional(ref input, ref context);
#endregion

#region APPEND

/// <inheritdoc />
Expand Down
9 changes: 9 additions & 0 deletions libs/server/API/IGarnetApi.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,15 @@ public interface IGarnetApi : IGarnetReadApi, IGarnetAdvancedApi

#endregion

#region MSETNX
/// <summary>
/// MSETNX
/// </summary>
/// <param name="input"></param>
/// <returns></returns>
GarnetStatus MSET_Conditional(ref RawStringInput input);
#endregion

#region APPEND

/// <summary>
Expand Down
26 changes: 11 additions & 15 deletions libs/server/Resp/ArrayCommands.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,10 @@ private bool NetworkMGET_SG<TGarnetApi>(ref TGarnetApi storageApi)
private bool NetworkMSET<TGarnetApi>(ref TGarnetApi storageApi)
where TGarnetApi : IGarnetApi
{
Debug.Assert(parseState.Count % 2 == 0);
if (parseState.Count == 0 || parseState.Count % 2 != 0)
{
return AbortWithWrongNumberOfArguments(nameof(RespCommand.MSET));
}

for (int c = 0; c < parseState.Count; c += 2)
{
Expand All @@ -176,23 +179,16 @@ private bool NetworkMSET<TGarnetApi>(ref TGarnetApi storageApi)
private bool NetworkMSETNX<TGarnetApi>(ref TGarnetApi storageApi)
where TGarnetApi : IGarnetApi
{
var anyValuesSet = false;
var input = new RawStringInput(RespCommand.SETEXNX);

for (var c = 0; c < parseState.Count; c += 2)
if (parseState.Count == 0 || parseState.Count % 2 != 0)
{
var key = parseState.GetArgSliceByRef(c).SpanByte;

input.parseState = parseState.Slice(c + 1, 1);
var status = storageApi.SET_Conditional(ref key, ref input);

// Status tells us whether an old image was found during RMW or not
// For a "set if not exists", NOTFOUND means that the operation succeeded
if (status == GarnetStatus.NOTFOUND)
anyValuesSet = true;
return AbortWithWrongNumberOfArguments(nameof(RespCommand.MSETNX));
}

while (!RespWriteUtils.TryWriteInt32(anyValuesSet ? 1 : 0, ref dcurr, dend))
var input = new RawStringInput(RespCommand.MSETNX, ref parseState);
var status = storageApi.MSET_Conditional(ref input);

// For a "set if not exists", NOTFOUND means that the operation succeeded
while (!RespWriteUtils.TryWriteInt32(status == GarnetStatus.NOTFOUND ? 1 : 0, ref dcurr, dend))
SendAndReset();
return true;
}
Expand Down
51 changes: 51 additions & 0 deletions libs/server/Storage/Session/MainStore/MainStoreOps.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -401,6 +401,57 @@ public unsafe GarnetStatus SET_Conditional<TContext>(ref SpanByte key, ref RawSt
}
}

internal GarnetStatus MSET_Conditional<TContext>(ref RawStringInput input, ref TContext ctx)
where TContext : ITsavoriteContext<SpanByte, SpanByte, RawStringInput, SpanByteAndMemory, long, MainSessionFunctions, MainStoreFunctions, MainStoreAllocator>
{
var error = false;
var count = input.parseState.Count;

var createTransaction = false;
if (txnManager.state != TxnState.Running)
{
createTransaction = true;
for (var i = 0; i < count; i += 2)
{
var srcKey = input.parseState.GetArgSliceByRef(i);
txnManager.SaveKeyEntryToLock(srcKey, false, LockType.Exclusive);
txnManager.SaveKeyEntryToLock(srcKey, true, LockType.Exclusive);
}
txnManager.Run(true);
}

var context = txnManager.LockableContext;
var objContext = txnManager.ObjectStoreLockableContext;

try
{
for (var i = 0; i < count; i += 2)
{
var srcKey = input.parseState.GetArgSliceByRef(i);
var status = EXISTS(srcKey, StoreType.All, ref context, ref objContext);
if (status != GarnetStatus.NOTFOUND)
{
count = 0;
error = true;
}
}

for (var i = 0; i < count; i += 2)
{
var srcKey = input.parseState.GetArgSliceByRef(i);
var srcVal = input.parseState.GetArgSliceByRef(i + 1);
SET(srcKey, srcVal, ref context);
}
}
finally
{
if (createTransaction)
txnManager.Commit(true);
}

return error ? GarnetStatus.OK : GarnetStatus.NOTFOUND;
}

public GarnetStatus SET<TContext>(ArgSlice key, ArgSlice value, ref TContext context)
where TContext : ITsavoriteContext<SpanByte, SpanByte, RawStringInput, SpanByteAndMemory, long, MainSessionFunctions, MainStoreFunctions, MainStoreAllocator>
{
Expand Down
73 changes: 62 additions & 11 deletions test/Garnet.test/RespTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -494,7 +494,10 @@ public void MultiSetGet()
var db = redis.GetDatabase(0);

const int length = 15000;
KeyValuePair<RedisKey, RedisValue>[] input = new KeyValuePair<RedisKey, RedisValue>[length];
var input = new KeyValuePair<RedisKey, RedisValue>[length];
var inputNX = new KeyValuePair<RedisKey, RedisValue>[length];
var inputXX = new KeyValuePair<RedisKey, RedisValue>[length];

for (int i = 0; i < length; i++)
input[i] = new KeyValuePair<RedisKey, RedisValue>(i.ToString(), i.ToString());

Expand All @@ -512,42 +515,90 @@ public void MultiSetGet()
result = db.StringSet(input);
ClassicAssert.IsTrue(result);

value = db.StringGet(input.Select(e => e.Key).ToArray());
var keys = input.Select(e => e.Key).ToArray();
value = db.StringGet(keys);
ClassicAssert.AreEqual(length, value.Length);

for (int i = 0; i < length; i++)
ClassicAssert.AreEqual(input[i].Value, value[i]);

// MSET NX - existing values
for (int i = 0; i < length; i++)
input[i] = new KeyValuePair<RedisKey, RedisValue>(i.ToString(), (i + 1).ToString());
inputXX[i] = new KeyValuePair<RedisKey, RedisValue>(i.ToString(), (i + 1).ToString());

result = db.StringSet(input, When.NotExists);
result = db.StringSet(inputXX, When.NotExists);
ClassicAssert.IsFalse(result);

value = db.StringGet(input.Select(e => e.Key).ToArray());
value = db.StringGet(keys);
ClassicAssert.AreEqual(length, value.Length);

// Should not change existing keys
for (int i = 0; i < length; i++)
ClassicAssert.AreEqual(new RedisValue(i.ToString()), value[i]);

// MSET NX - non-existing and existing values
for (int i = 0; i < length; i++)
input[i] = new KeyValuePair<RedisKey, RedisValue>((i % 2 == 0 ? i : i + length).ToString(), (i + length).ToString());
inputNX[i] = new KeyValuePair<RedisKey, RedisValue>((i % 2 == 0 ? i : i + length).ToString(), (i + length).ToString());

result = db.StringSet(input, When.NotExists);
ClassicAssert.IsTrue(result);
result = db.StringSet(inputNX, When.NotExists);
ClassicAssert.IsFalse(result);

value = db.StringGet(input.Select(e => e.Key).ToArray());
value = db.StringGet(keys);
ClassicAssert.AreEqual(length, value.Length);

for (int i = 0; i < length; i++)
{
ClassicAssert.AreEqual(i % 2 == 0 ? new RedisValue((int.Parse(input[i].Value) - length).ToString()) :
input[i].Value, value[i]);
ClassicAssert.AreEqual(new RedisValue(i.ToString()), value[i]);
}

// Should not create new keys if any existing keys were also specified
var nxKeys = inputNX.Select(x => x.Key).Where(x => !keys.Contains(x)).Take(10).ToArray();
value = db.StringGet(nxKeys);
for (int i = 0; i < value.Length; i++)
{
ClassicAssert.IsEmpty(value[i].ToString());
}
}

[Test]
public void MultiSetNX()
{
var lightClientRequest = TestUtils.CreateRequest();

// Set keys
var response = lightClientRequest.SendCommand("MSETNX key1 5 key2 6");
var expectedResponse = ":1\r\n";
ClassicAssert.AreEqual(expectedResponse, response.AsSpan().Slice(0, expectedResponse.Length).ToArray());

// MSETNX command should fail since key exists
response = lightClientRequest.SendCommand("MSETNX key3 7 key1 8");
expectedResponse = ":0\r\n";
ClassicAssert.AreEqual(expectedResponse, response.AsSpan().Slice(0, expectedResponse.Length).ToArray());

// Verify values
response = lightClientRequest.SendCommand("GET key1");
expectedResponse = "$1\r\n5\r\n";
ClassicAssert.AreEqual(expectedResponse, response.AsSpan().Slice(0, expectedResponse.Length).ToArray());

response = lightClientRequest.SendCommand("GET key2");
expectedResponse = "$1\r\n6\r\n";
ClassicAssert.AreEqual(expectedResponse, response.AsSpan().Slice(0, expectedResponse.Length).ToArray());

// Should not be set even though it was 'before' existing key1.
response = lightClientRequest.SendCommand("GET key3");
expectedResponse = "$-1\r\n";
ClassicAssert.AreEqual(expectedResponse, response.AsSpan().Slice(0, expectedResponse.Length).ToArray());

response = lightClientRequest.SendCommand("HSET key4 first 1");
expectedResponse = ":1\r\n";
ClassicAssert.AreEqual(expectedResponse, response.AsSpan().Slice(0, expectedResponse.Length).ToArray());

// MSETNX command should fail since key exists even if it's an object.
response = lightClientRequest.SendCommand("MSETNX key3 7 key4 8");
expectedResponse = ":0\r\n";
ClassicAssert.AreEqual(expectedResponse, response.AsSpan().Slice(0, expectedResponse.Length).ToArray());
}

[Test]
public void LargeSetGet()
{
Expand Down