Introduce MDel do the batch delete and use MultiLockGuard guarantee atomicity

[enjoy-binbin]

Previously, the DEL/UNLINK commands looped through each key to
delete a single key. We need to change to batch delete (write),
and also the original implementation did not lock the keys and
will break atomicity.

This PR introduces a new Mdel function, which uses MultiLockGuard
to lock all the keys to guarantee atomicity, and use batch to do
the final delete (write).

Now it will be only used in DEL / UNLINKE commands. Noticed that
our DEL / UNLINK test coverage was not sufficient, added some simple
coverage tests to cover it.

Fixes for DEL / UNLINK issues in #1692.

unrelated change: in Exists for loop Get, return an execution error
when !s.ok() && !s.IsNotFound() without continuing the operation
(because it will be some internal RocksDB error).

[git-hulk]

We may have the deadlock in ZSet::InterStore(as well as other similar APIs) if the dst key was inside the source key list since the OverWrite function will also lock the dst key: https://github.com/apache/kvrocks/blob/unstable/src/types/redis_zset.cc#L635

This issue was found by TSAN in CI: https://github.com/apache/kvrocks/actions/runs/6022265431/job/16338859035?pr=1712

[torwig]

@git-hulk Good point.

[torwig]

I guess we should lock all the keys including the destination inside the ZSet::InterStore and similar methods (right now we lock the destination inside the Overwite method). Will it work?

[git-hulk]

I guess we should lock all the keys including the destination inside the ZSet::InterStore and similar methods (right now we lock the destination inside the Overwite method). Will it work?

Yes, I think it can resolve this issue.

[git-hulk]

I submit another issue to track this since it's related but NOT introduced by this PR. #1715

[enjoy-binbin]

Or can it be ok without locking the dst key? since we are doing a overwrite

[torwig]

@enjoy-binbin I think we need to serialize write operations on the key to avoid "lost updates". However, you made me rethink the previous sentence because we deal with overwriting :)

[git-hulk]

Is it possible to lock the dst key only and use the read snapshot like SDIFFSTORE/SINTERSTORE?

[enjoy-binbin]

Is it possible to lock the dst key only and use the read snapshot like SDIFFSTORE/SINTERSTORE?

@git-hulk can you describe this a bit more? (for example posting a pseudo code)

[git-hulk]

Is it possible to lock the dst key only and use the read snapshot like SDIFFSTORE/SINTERSTORE?

@git-hulk can you describe this a bit more? (for example posting a pseudo code)

Sure. For now, SDIFFSTORE is implemented in two steps:

• The first step is to calculate the diff results without locking source keys
• Then overwrite the dst key with locking the dst key

Please refer https://github.com/apache/kvrocks/blob/unstable/src/types/redis_set.cc#L433

[enjoy-binbin]

actually now we will lock the source keys. it was added in #1700, maybe we should remove the source keys locks?

rocksdb::Status Set::Diff(const std::vector<Slice> &keys, std::vector<std::string> *members) {
  std::vector<std::string> lock_keys;
  lock_keys.reserve(keys.size());
  for (const auto key : keys) {
    std::string ns_key = AppendNamespacePrefix(key);
    lock_keys.emplace_back(std::move(ns_key));
  }
  MultiLockGuard guard(storage_->GetLockManager(), lock_keys);

edit: ohh, i think i got your point. maybe we should split InterStore to two function, one do the inter and the other one do the store, just like the Set apis. I wanted to do it (adding a ZINTER command) before, but because we already had a pending #1517

[git-hulk]

Is it possible to lock the dst key only and use the read snapshot like SDIFFSTORE/SINTERSTORE?

@git-hulk can you describe this a bit more? (for example posting a pseudo code)

Sure. For now, SDIFFSTORE is implemented in two steps:

• The first step is to calculated the diff result without locking source keys
• Then overwrite the dst key with locking the dst key

actually now we will lock the source keys. it was added in #1700, maybe we should remove the source keys locks?

rocksdb::Status Set::Diff(const std::vector<Slice> &keys, std::vector<std::string> *members) {
  std::vector<std::string> lock_keys;
  lock_keys.reserve(keys.size());
  for (const auto key : keys) {
    std::string ns_key = AppendNamespacePrefix(key);
    lock_keys.emplace_back(std::move(ns_key));
  }
  MultiLockGuard guard(storage_->GetLockManager(), lock_keys);

edit: ohh, i think i got your point. maybe we should split InterStore to two function, one do the inter and the other one do the store, just like the Set apis. I wanted to do it (adding a ZINTER command) before, but because we already had a pending #1517

Great, thank you!

[enjoy-binbin]

i pushed a commit c51975b

split InterStore to two function, one do the inter and the other one do the store, just like the Set apis. to see if the CI can pass

[enjoy-binbin]

i see the CI is passed, so the split change can fix the deadlock, i am going to submit another PR to make the change, am revert the change in this PR (after we merge the new PR see #1726, we can re-merge it)

[PragmaTwice]

lock-order-inversion found in TSan

https://github.com/apache/kvrocks/actions/runs/6062050794/job/16447962572?pr=1712

[mapleFU]

General LGTM. Just some nits.