fix: bitop do not add dst key if result is empty #3751

kostasrim · 2024-09-20T14:17:19Z

There are two bugs:

Bug 1:

bitop not does_not_exist tmp2
get does_not_exist

Should return nill and not ""

Bug 2:

setup master and replica and then:

on master do: hset foo bar val -> bitop not foo bar -> get foo returns WRONGTYPE
on replica do GET foo returns ""

Now state_of(master) != state_of(replica)

Bug 3:

hset foo bar val -> bitop not foo bar -> get foo returns WRONGTYPE

when get foo should return nill, that is bitop acts as a blind update similar to set command

resolves #3528

Signed-off-by: kostas <kostas@dragonflydb.io>

adiholden · 2024-09-22T11:27:32Z

src/server/bitops_family.cc

@@ -343,8 +343,14 @@ std::string ElementAccess::Value() const {

 void ElementAccess::Commit(std::string_view new_value) const {
  if (shard_) {
-    element_iter_->second.SetString(new_value);
-    post_updater_.Run();
+    if (new_value.empty() && IsNewEntry()) {


I think we should delete it even if its not a new entry
set x y
bitop not x key
get x
should also return nil

I also added a test and confirmed the behaviour with Valkey. good catch

adiholden · 2024-09-22T11:36:15Z

src/server/bitops_family.cc

@@ -1159,7 +1165,7 @@ void BitOp(CmdArgList args, ConnectionContext* cntx) {
  cntx->transaction->Execute(std::move(shard_bitop), false);  // we still have more work to do
  // All result from each shard
  const auto joined_results = CombineResultOp(result_set, op);
-  // Second phase - save to targe key if successful
+  // Second phase - save to target key if successful


check the replication journal code line 1185. need to fix this also

Wow. I blindly did not bother with the journal because I thought that we replicate the same command always. But in this context, we do not. What we do instead is we take the result of the operation and call SET directly which is a very nice optimization since we don't now need to run the command on the replica but rather do an update on the result directly.

This is a nice pattern/optimization to know. +1

adiholden · 2024-09-23T09:50:40Z

src/server/bitops_family.cc

-    post_updater_.Run();
+    if (new_value.empty()) {
+      // No need to run, it was a new entry and it got removed
+      post_updater_.Cancel();


I think you should run the post updater here as this might be not a new entry

yep +1. I wish we could run it unconditionally though (but we can't because we have an assertion within it :(

adiholden · 2024-09-23T10:01:01Z

src/server/bitops_family.cc

@@ -1176,7 +1182,15 @@ void BitOp(CmdArgList args, ConnectionContext* cntx) {
        }

        if (shard->journal()) {
-          RecordJournal(t->GetOpArgs(shard), "SET", {dest_key, op_result});
+          if (op_result.empty()) {


also if find_res == OpStatus::OK

my question here would be if the result is not ok why do we RecordJournal ?

we should not

we had another bug then even before my changes 😛

setup master and replica and then:

on master do: hset foo bar val -> bitop not foo bar -> get foo returns WRONGTYPE

on replica do GET foo returns ""

Now state_of(master) != state_of(replica)

I added this on the issue + supplied the fix + added a replication test case so we don't break it again

romange · 2024-09-23T10:08:04Z

As a general remark - I suggest adding comments around points that raised discussions in the PR so that future selves could understand the intent

Signed-off-by: kostas <kostas@dragonflydb.io>

kostasrim · 2024-09-23T11:05:41Z

As a general remark - I suggest adding comments around points that raised discussions in the PR so that future selves could understand the intent

yep +1. I added a comment + the tests

adiholden · 2024-09-23T11:18:43Z

tests/dragonfly/replication_test.py

+    await wait_available_async(c_replica)
+
+    c_master = master.client()
+    await c_master.execute_command(f"HSET foo bar val")


I would add this as a subtest case of def test_rewrites

you will check there that del foo was sent

hmm I did not know that bitops acts as a blind update. I tested this with Valkey, so if the operation returns WRONG_TYPE we should still delete the key. I will add this to the logic + comment + move this to rewrite

So the fix above is also incorrect if (find_res == OpStatus::OK)!

I will add a unit for this as well

adiholden · 2024-09-23T11:21:19Z

tests/dragonfly/replication_test.py

+    await c_master.execute_command(f"BITOP not foo 1")
+
+    with pytest.raises(redis.exceptions.ResponseError):
+        await c_replica.execute_command(f"GET foo")


also this is not very robust because you dont know if replica already got the journal change or not, it could return error because foo is not a string and not because it was deleted

adiholden · 2024-09-24T08:30:48Z

tests/dragonfly/replication_test.py

@@ -673,6 +673,18 @@ async def check_expire(key):
        await skip_cmd()
        # Check BITOP turns into SET
        await check("BITOP OR kdest k1 k2", r"SET kdest 1100")
+        c_master = master.client()


why do you need this line

adiholden · 2024-09-24T08:31:32Z

tests/dragonfly/replication_test.py

+        await c_master.execute_command(f"HSET foo bar val")
+        await skip_cmd()
+        await check("BITOP NOT foo tmp", r"DEL foo")
+        await c_master.execute_command(f"DEL foo")


does foo exists in master at this point? why do you need del foo?

fix: bitop do not add dst key if result is empty

f598f28

Signed-off-by: kostas <kostas@dragonflydb.io>

kostasrim requested a review from adiholden September 20, 2024 14:17

kostasrim self-assigned this Sep 20, 2024

adiholden reviewed Sep 22, 2024

View reviewed changes

address comments

b69eba7

kostasrim requested a review from adiholden September 23, 2024 09:21

adiholden reviewed Sep 23, 2024

View reviewed changes

fixes

0f39c92

Signed-off-by: kostas <kostas@dragonflydb.io>

kostasrim requested a review from adiholden September 23, 2024 11:01

remove unused from pytest

8c8f7d0

adiholden reviewed Sep 23, 2024

View reviewed changes

kostasrim added 3 commits September 23, 2024 16:23

fixes

01480bf

remove not used code

ab9cc44

add info

572c16b

kostasrim requested a review from adiholden September 23, 2024 13:28

complete missing comment

bcfa23b

adiholden reviewed Sep 24, 2024

View reviewed changes

comments

f1cb237

kostasrim requested a review from adiholden September 24, 2024 08:42

adiholden approved these changes Sep 24, 2024

View reviewed changes

kostasrim merged commit 105c2bd into main Sep 25, 2024
12 checks passed

kostasrim deleted the kpr2 branch September 25, 2024 06:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: bitop do not add dst key if result is empty #3751

fix: bitop do not add dst key if result is empty #3751

kostasrim commented Sep 20, 2024 •

edited

Loading

adiholden Sep 22, 2024

kostasrim Sep 23, 2024

adiholden Sep 22, 2024

kostasrim Sep 23, 2024 •

edited

Loading

adiholden Sep 23, 2024

kostasrim Sep 23, 2024

adiholden Sep 23, 2024

kostasrim Sep 23, 2024

adiholden Sep 23, 2024

kostasrim Sep 23, 2024 •

edited

Loading

kostasrim Sep 23, 2024 •

edited

Loading

kostasrim Sep 23, 2024

romange commented Sep 23, 2024

kostasrim commented Sep 23, 2024

adiholden Sep 23, 2024

adiholden Sep 23, 2024

kostasrim Sep 23, 2024

kostasrim Sep 23, 2024 •

edited

Loading

adiholden Sep 23, 2024

adiholden Sep 24, 2024

adiholden Sep 24, 2024

fix: bitop do not add dst key if result is empty #3751

fix: bitop do not add dst key if result is empty #3751

Conversation

kostasrim commented Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kostasrim Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kostasrim Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

kostasrim Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romange commented Sep 23, 2024

kostasrim commented Sep 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kostasrim Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kostasrim commented Sep 20, 2024 •

edited

Loading

kostasrim Sep 23, 2024 •

edited

Loading

kostasrim Sep 23, 2024 •

edited

Loading

kostasrim Sep 23, 2024 •

edited

Loading

kostasrim Sep 23, 2024 •

edited

Loading