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

Fix MSETNX not allow overriding the same key #1631

Merged
merged 7 commits into from
Aug 4, 2023
Merged

Fix MSETNX not allow overriding the same key #1631

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3860daa
Fix MSETNX not allow overriding the same key
enjoy-binbin Aug 3, 2023
ecd0f5e
code review from mapleFU. mark and guard life
enjoy-binbin Aug 3, 2023
2ea7000
code review from twice, thanks a lot
enjoy-binbin Aug 3, 2023
ed65995
fix ci
enjoy-binbin Aug 3, 2023
8a5666e
Merge branch 'unstable' into fix_msetnx_issue
enjoy-binbin Aug 4, 2023
8163be6
Merge branch 'unstable' into fix_msetnx_issue
enjoy-binbin Aug 4, 2023
12ae9a4
code review
enjoy-binbin Aug 4, 2023
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
2 changes: 1 addition & 1 deletion src/commands/cmd_string.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -613,7 +613,7 @@ REDIS_REGISTER_COMMANDS(
MakeCmdAttr<CommandAppend>("append", 3, "write", 1, 1, 1), MakeCmdAttr<CommandSet>("set", -3, "write", 1, 1, 1),
MakeCmdAttr<CommandSetEX>("setex", 4, "write", 1, 1, 1), MakeCmdAttr<CommandPSetEX>("psetex", 4, "write", 1, 1, 1),
MakeCmdAttr<CommandSetNX>("setnx", 3, "write", 1, 1, 1),
MakeCmdAttr<CommandMSetNX>("msetnx", -3, "write exclusive", 1, -1, 2),
MakeCmdAttr<CommandMSetNX>("msetnx", -3, "write", 1, -1, 2),
MakeCmdAttr<CommandMSet>("mset", -3, "write", 1, -1, 2), MakeCmdAttr<CommandIncrBy>("incrby", 3, "write", 1, 1, 1),
MakeCmdAttr<CommandIncrByFloat>("incrbyfloat", 3, "write", 1, 1, 1),
MakeCmdAttr<CommandIncr>("incr", 2, "write", 1, 1, 1), MakeCmdAttr<CommandDecrBy>("decrby", 3, "write", 1, 1, 1),
Expand Down
49 changes: 20 additions & 29 deletions src/types/redis_string.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#include <cstddef>
#include <cstdint>
#include <limits>
#include <optional>
#include <string>

#include "parse_util.h"
Expand Down Expand Up @@ -213,12 +214,12 @@ rocksdb::Status String::GetDel(const std::string &user_key, std::string *value)

rocksdb::Status String::Set(const std::string &user_key, const std::string &value) {
std::vector<StringPair> pairs{StringPair{user_key, value}};
return MSet(pairs, 0);
return MSet(pairs, /*ttl=*/0, /*lock=*/true);
}

rocksdb::Status String::SetEX(const std::string &user_key, const std::string &value, uint64_t ttl) {
std::vector<StringPair> pairs{StringPair{user_key, value}};
return MSet(pairs, ttl);
return MSet(pairs, /*ttl=*/ttl, /*lock=*/true);
}

rocksdb::Status String::SetNX(const std::string &user_key, const std::string &value, uint64_t ttl, bool *flag) {
Expand Down Expand Up @@ -363,7 +364,7 @@ rocksdb::Status String::IncrByFloat(const std::string &user_key, double incremen
return updateRawValue(ns_key, raw_value);
}

rocksdb::Status String::MSet(const std::vector<StringPair> &pairs, uint64_t ttl) {
rocksdb::Status String::MSet(const std::vector<StringPair> &pairs, uint64_t ttl, bool lock) {
uint64_t expire = 0;
if (ttl > 0) {
uint64_t now = util::GetTimeStampMS();
Expand All @@ -384,7 +385,10 @@ rocksdb::Status String::MSet(const std::vector<StringPair> &pairs, uint64_t ttl)
batch->PutLogData(log_data.Encode());
AppendNamespacePrefix(pair.key, &ns_key);
batch->Put(metadata_cf_handle_, ns_key, bytes);
LockGuard guard(storage_->GetLockManager(), ns_key);
std::optional<LockGuard> guard;
if (lock) {
guard.emplace(storage_->GetLockManager(), ns_key);
}
auto s = storage_->Write(storage_->DefaultWriteOptions(), batch->GetWriteBatch());
if (!s.ok()) return s;
}
Expand All @@ -394,41 +398,28 @@ rocksdb::Status String::MSet(const std::vector<StringPair> &pairs, uint64_t ttl)
rocksdb::Status String::MSetNX(const std::vector<StringPair> &pairs, uint64_t ttl, bool *flag) {
*flag = false;

uint64_t expire = 0;
if (ttl > 0) {
uint64_t now = util::GetTimeStampMS();
expire = now + ttl;
}

int exists = 0;
std::string ns_key;
std::vector<std::string> lock_keys;
std::vector<Slice> keys;
keys.reserve(pairs.size());

git-hulk marked this conversation as resolved.
Show resolved Hide resolved
for (StringPair pair : pairs) {
AppendNamespacePrefix(pair.key, &ns_key);
lock_keys.emplace_back(ns_key);
keys.emplace_back(pair.key);
}

// Lock these keys before doing anything.
MultiLockGuard guard(storage_->GetLockManager(), lock_keys);

if (Exists(keys, &exists).ok() && exists > 0) {
return rocksdb::Status::OK();
}

std::string ns_key;
for (StringPair pair : pairs) {
AppendNamespacePrefix(pair.key, &ns_key);
LockGuard guard(storage_->GetLockManager(), ns_key);
if (Exists({pair.key}, &exists).ok() && exists == 1) {
return rocksdb::Status::OK();
}
std::string bytes;
Metadata metadata(kRedisString, false);
metadata.expire = expire;
metadata.Encode(&bytes);
bytes.append(pair.value.data(), pair.value.size());
auto batch = storage_->GetWriteBatchBase();
WriteBatchLogData log_data(kRedisString);
batch->PutLogData(log_data.Encode());
batch->Put(metadata_cf_handle_, ns_key, bytes);
auto s = storage_->Write(storage_->DefaultWriteOptions(), batch->GetWriteBatch());
if (!s.ok()) return s;
}
rocksdb::Status s = MSet(pairs, ttl, false);
enjoy-binbin marked this conversation as resolved.
Show resolved Hide resolved
if (!s.ok()) return s;

*flag = true;
return rocksdb::Status::OK();
}
Expand Down
2 changes: 1 addition & 1 deletion src/types/redis_string.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ class String : public Database {
rocksdb::Status IncrBy(const std::string &user_key, int64_t increment, int64_t *new_value);
rocksdb::Status IncrByFloat(const std::string &user_key, double increment, double *new_value);
std::vector<rocksdb::Status> MGet(const std::vector<Slice> &keys, std::vector<std::string> *values);
rocksdb::Status MSet(const std::vector<StringPair> &pairs, uint64_t ttl = 0);
rocksdb::Status MSet(const std::vector<StringPair> &pairs, uint64_t ttl = 0, bool lock = true);
rocksdb::Status MSetNX(const std::vector<StringPair> &pairs, uint64_t ttl, bool *flag);
rocksdb::Status CAS(const std::string &user_key, const std::string &old_value, const std::string &new_value,
uint64_t ttl, int *flag);
Expand Down
15 changes: 15 additions & 0 deletions tests/gocase/unit/type/strings/strings_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,21 @@ func TestString(t *testing.T) {
require.Equal(t, "yyy", rdb.Get(ctx, "y2").Val())
})

t.Run("MSETNX with already existent key - same key", func(t *testing.T) {
require.NoError(t, rdb.Del(ctx, "x").Err())
require.NoError(t, rdb.Set(ctx, "x", "v0", 0).Err())
require.Equal(t, int64(0), rdb.Do(ctx, "MSETNX", "x", "v1", "x", "v2").Val())
require.EqualValues(t, 1, rdb.Exists(ctx, "x").Val())
require.Equal(t, "v0", rdb.Get(ctx, "x").Val())
})

t.Run("MSETNX with not existing keys - same key", func(t *testing.T) {
require.NoError(t, rdb.Del(ctx, "x").Err())
require.Equal(t, int64(1), rdb.Do(ctx, "MSETNX", "x", "v1", "x", "v2").Val())
require.EqualValues(t, 1, rdb.Exists(ctx, "x").Val())
require.Equal(t, "v2", rdb.Get(ctx, "x").Val())
})

t.Run("STRLEN against non-existing key", func(t *testing.T) {
require.EqualValues(t, 0, rdb.StrLen(ctx, "notakey").Val())
})
Expand Down