Hoist key mutexes from every command impls to Connection::ExecuteCommand

[PragmaTwice]

Search before asking

• I had searched in the issues and found no similar issues.

Motivation

We can remove all key-level mutex locking operations in command implementation and handle them uniformly in Connection::ExecuteCommand.

In this way, we can handle transaction support of scripts, index, etc. more easily.

Solution

No response

Are you willing to submit a PR?

• I'm willing to submit a PR!

[PokIsemaine]

I am trying to move the operation of the lock key to ExecuteCommands(not ExecuteCommand), because I realized that the index might also need to be locked.

If cmd_flags contains the kCmdWrite flag, I will use attributes->ForEachKeyRange to get all the keys that need to be locked and lock these keys together with MultiLockGuard.

What I want to confirm is whether the keys obtained in this way do indeed correspond to those that need to be locked, and if there are any redundancies or omissions?

Furthermore, after some attempts, I found that the locking for certain methods depends on the method's parameters. Since the locks are now uniformly applied within ExecuteCommands, we may need to unlock them based on the inverse conditions.

rocksdb::Status String::MSet(engine::Context &ctx, const std::vector<StringPair> &pairs, uint64_t expire_ms,
                             bool lock) {
  // Data race, key string maybe overwrite by other key while didn't lock the keys here,
  // to improve the set performance
  std::optional<MultiLockGuard> guard;
  if (lock) {
    std::vector<std::string> lock_keys;
    lock_keys.reserve(pairs.size());
    for (const StringPair &pair : pairs) {
      std::string ns_key = AppendNamespacePrefix(pair.key);
      lock_keys.emplace_back(std::move(ns_key));
    }
    guard.emplace(storage_->GetLockManager(), lock_keys);
  }
  

rocksdb::Status ZSet::RangeByRank(engine::Context &ctx, const Slice &user_key, const RangeRankSpec &spec,
                                  MemberScores *mscores, uint64_t *removed_cnt) {
  if (mscores) mscores->clear();

  uint64_t cnt = 0;
  if (!removed_cnt) removed_cnt = &cnt;
  *removed_cnt = 0;

  std::string ns_key = AppendNamespacePrefix(user_key);

  std::optional<LockGuard> lock_guard;
  if (spec.with_deletion) lock_guard.emplace(storage_->GetLockManager(), ns_key);

This has raised a few questions that need to be considered:

• We do not pass the LockGuard object but instead unlock through the LockManager. When unlocking, there is no association between the two, which could lead to the LockGuard being unlocked multiple times due to repeated calls. Should we pass the LockGuard or establish some kind of association mechanism?
• Would directly unlocking individual or partial keys via the LockManager violate the requirements for the order of unlocking? I noticed that MultiLockGuard has a specific requirement for the order of unlocking

  ~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();
    }
  }

[PragmaTwice]

For MSet, it's quite easy, the lock parameter is just for reuse MSet in other functions (e.g. maybe it's already locked in other methods so it shouldn't lock again while calling MSet), if we move them to ExecuteCommands we can just remove these parameters.

For RangeByRank, you can see that for commands like ZREMRANGEBYRANK, with_deletion is true, and for commands like ZRANGE, it's false. So it doesn't affect this part, we can just check if the command is with a "write" flag.

For some more complicated commands which is an "optionally-write" command, e.g. it can be "write" in some cases (when some arguments are provided by users), we should model it via a runtime flag generator mechanism here.