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

Use MultiLockGuard to guarantee atomicity for multiple keys commands #1700

Merged
merged 2 commits into from
Aug 26, 2023

Conversation

enjoy-binbin
Copy link
Member

@enjoy-binbin enjoy-binbin commented Aug 25, 2023

In kvrocks, for multiple keys commands, we may break atomicity.
such as ZUNION, ZUNIONSTORE, ZINTERSTORE, SUNION, SUNIONSTORE,
SINTER, SINTERSTORE, SDIFF and SDIFFSTORE.

For SDIFFSTORE destination key1 [key2 ...] command, kvrocks will
read key1, key2 ... orderly without any lock, and then restore the
diff into destination key. So maybe some keys may be changed when
we read them orderly, that breaks atomicity but redis can guarantee
atomicity.

In this PR, we are using MultiLockGuard to lock these keys before
we read or write, so it is impossible to change these keys. This change
affects all commands listed previously.

Fixes #1692.

In kvrocks, for multiple keys commands, we may break atomicity.
such as ZUNION, ZUNIONSTORE, ZINTERSTORE, SUNION, SUNIONSTORE,
SINTER, SINTERSTORE, SDIFF and SDIFFSTORE.

For SDIFFSTORE destination key1 [key2 ...] command, kvrocks will
read key1, key2 ... orderly without any lock, and then restore the
diff into destination key. So maybe some keys may be changed when
we read them orderly, that breaks atomicity but redis can guarantee
atomicity.

In this PR, we are using MultiLockGuard to lock these keys before
we read or write, so it is impossible to change these keys.

Fixes 1692.
torwig
torwig previously approved these changes Aug 25, 2023
Copy link
Contributor

@torwig torwig left a comment

Choose a reason for hiding this comment

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

LGTM

mapleFU
mapleFU previously approved these changes Aug 25, 2023
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.

Rest looks good to me, except a nit. lock_keys.emplace_back(ns_key); can add a move here.

src/types/redis_set.cc Outdated Show resolved Hide resolved
Co-authored-by: mwish <maplewish117@gmail.com>
@enjoy-binbin enjoy-binbin dismissed stale reviews from mapleFU and torwig via 4b9a0e7 August 25, 2023 08:25
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.

Thanks!

(by the way, the code below provide the same ability, you can also use this if you like)

lock_keys.emplace_back(AppendNamespacePrefix(key));

@git-hulk git-hulk merged commit c9b1e8e into apache:unstable Aug 26, 2023
26 checks passed
@enjoy-binbin enjoy-binbin deleted the keys_lock branch August 26, 2023 05:59
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.

Kvrocks can't guarantee atomicity for multiple keys commands
4 participants