-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix(string_family): Add RemoveDoc for the SET command #5306
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(string_family): Add RemoveDoc for the SET command #5306
Conversation
fixes dragonflydb#5215 Signed-off-by: Stepan Bagritsevich <stefan@dragonflydb.io>
|
@BagritsevichStepan |
Done |
vyavdoshenko
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@romange
Could you please take a look at this PR as well?
I see the potential issue. In my opinion, this issue can be bigger than this specific issue. We have to track the search index in all possible places, briefly speaking, all mutable operations. I am afraid that the same issue can cause the same or similar crash during the change of a different pair of types.
Personally, I don't know how to generalize the approach and avoid similar issues in the future. Maybe I am paranoid, correct me if I am wrong.
|
@vyavdoshenko I spoke about it today and we already have an issue for this #5014 . I have a solution |
src/server/string_family.cc
Outdated
| db_slice.SetMCFlag(op_args_.db_cntx.db_index, it->first.AsRef(), params.memcache_flags); | ||
|
|
||
| // We need to remove the key from search indices, because we are overwriting it to OBJ_STRING | ||
| shard->search_indices()->RemoveDoc(key, db_cntx, prime_value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i do not like this unconditional approach. please check before if prime_value.ObjType() is indexable (i.e. either hset or json). You can introduce a one-liner utility function for better readability (add it to family_utils.h).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know, we have this problem in several places (check for example rename command). Let's do it in another PR, I will fix other commands as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, please do not forget
|
most of our write commands are not blind writes so the issue is not wide to begin with. |
|
I know for example zunionstore command, that also changes the key type |
|
And there is the same bug in the |
Signed-off-by: Stepan Bagritsevich <stefan@dragonflydb.io>
3bc3020 to
29706a8
Compare
fixes #5215
The problem was that the
StringFamily::Setcommand could change the key's type. If the previous type wasHSETorJSON, the key was indexed. WhenStringFamily::Setis called on such a key, it changes the type from HSET/JSON to STRING, but we forgot to remove the key from the indexes.This PR adds a call to
RemoveDocinsideStringFamily::Setto properly clean up the old index entry.A snippet that reproduces the issue has been added to the tests, along with explanatory comments: