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.

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

Conversation

enjoy-binbin
Copy link
Member

The changes in #337 causing this issue:

127.0.0.1:6379> flushall
OK
127.0.0.1:6379> msetnx k v1 k v2
(integer) 1
127.0.0.1:6379> get k
"v2

127.0.0.1:6666> flushall
OK
127.0.0.1:6666> msetnx k v1 k v2
(integer) 0
127.0.0.1:6666> get k
"v1"

Redis allow we overriding the same key but kvrocks will
fail in this case. The way to handle it is to revert the
changes in #337.

So this PR is based on that, after reverting the changes
of #337, we can reuse the logic of MSet. And hulk mentions
that MSETNX is an exclusive command and we better to lock
those keys and then remove the exclusive flag from the command.

So we use MultiLockGuard before Exists call to lock multi
keys. And change MSet to support non-lock calls. And also
remove the exclusive flag from MSETNX command.

Co-authored-by: hulk hulk.website@gmail.com

@enjoy-binbin enjoy-binbin requested a review from git-hulk August 3, 2023 07:31
Copy link
Member

@mapleFU mapleFU left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't review it carefully, just a tiny round.

src/types/redis_string.cc Outdated Show resolved Hide resolved
src/types/redis_string.cc Outdated Show resolved Hide resolved
git-hulk
git-hulk previously approved these changes Aug 3, 2023
src/types/redis_string.cc Show resolved Hide resolved
src/types/redis_string.cc Outdated Show resolved Hide resolved
@mapleFU
Copy link
Member

mapleFU commented Aug 4, 2023

After a round of review I get to understand this patch. There is a problem:

class MultiLockGuard {
 public:
  explicit MultiLockGuard(LockManager *lock_mgr, const std::vector<std::string> &keys) : lock_mgr_(lock_mgr) {
    locks_ = lock_mgr_->MultiGet(keys);
    for (const auto &iter : locks_) {
      iter->lock();
    }
  }

  ~MultiLockGuard() {
    // Lock with order `A B C` and unlock should be `C B A`
    for (auto iter = locks_.rbegin(); iter != locks_.rend(); ++iter) {
      (*iter)->unlock();
    }
  }

  MultiLockGuard(const MultiLockGuard &) = delete;
  MultiLockGuard &operator=(const MultiLockGuard &) = delete;

 private:
  LockManager *lock_mgr_ = nullptr;
  std::vector<std::mutex *> locks_;
};

Previously, the lock happens per-key. It was a bit wierd but would not introduce problem. Now, exclusive is removed, so it might introduce the deadlock problem.

A proposal is that using std::lock( https://en.cppreference.com/w/cpp/thread/lock ) instead of loop in MultiLockGuard.

@PragmaTwice would that be possible?

@git-hulk
Copy link
Member

git-hulk commented Aug 4, 2023

@mapleFU I think it won't have the deadlock for the multi-keys lock since it will sort keys first: https://github.com/apache/kvrocks/blob/unstable/src/storage/lock_manager.cc#L43

@mapleFU
Copy link
Member

mapleFU commented Aug 4, 2023

Nice I didn't find out that.

@git-hulk git-hulk merged commit 36ce907 into apache:unstable Aug 4, 2023
@enjoy-binbin enjoy-binbin deleted the fix_msetnx_issue branch August 5, 2023 07:53
PragmaTwice pushed a commit that referenced this pull request Aug 7, 2023
The previous implementation which wrote the batch per key,
we should change it to a single batch. Also use the multi-key
lock before the write operations instead of inside the loop.

This is mention in #1631, found by hulk.

Co-authored-by: mwish <1506118561@qq.com>
p1u3o pushed a commit to p1u3o/incubator-kvrocks that referenced this pull request Aug 15, 2023
The changes in apache#337 causing this issue:
```
127.0.0.1:6379> flushall
OK
127.0.0.1:6379> msetnx k v1 k v2
(integer) 1
127.0.0.1:6379> get k
"v2

127.0.0.1:6666> flushall
OK
127.0.0.1:6666> msetnx k v1 k v2
(integer) 0
127.0.0.1:6666> get k
"v1"
```

Redis allow we overriding the same key but kvrocks will
fail in this case. The way to handle it is to revert the
changes in apache#337.

So this PR is based on that, after reverting the changes
of apache#337, we can reuse the logic of MSet. And hulk mentions
that MSETNX is an exclusive command and we better to lock
those keys and then remove the exclusive flag from the command.

So we use MultiLockGuard before Exists call to lock multi
keys. And change MSet to support non-lock calls. And also
remove the exclusive flag from MSETNX command.
p1u3o pushed a commit to p1u3o/incubator-kvrocks that referenced this pull request Aug 15, 2023
The previous implementation which wrote the batch per key,
we should change it to a single batch. Also use the multi-key
lock before the write operations instead of inside the loop.

This is mention in apache#1631, found by hulk.

Co-authored-by: mwish <1506118561@qq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants