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

[BUG] Remove empty tag values #2269

Merged
merged 6 commits into from
Oct 5, 2021
Merged

[BUG] Remove empty tag values #2269

merged 6 commits into from
Oct 5, 2021

Conversation

ashtul
Copy link
Contributor

@ashtul ashtul commented Oct 4, 2021

The fix updates ForkGC to remove empty tag values from the trie. This caused a leak.
This bug affected indexes where TAG fields are updated with new values while the old, deleted values are left in the trie.

src/fork_gc.c Outdated Show resolved Hide resolved
src/fork_gc.c Outdated
uint64_t *id) {
if (FGC_recvBuffer(fgc, (void **)fieldName, fieldNameLen) != REDISMODULE_OK) {
static FGCError recvNumericTagHeader(ForkGC *fgc, tagNumHeader *header, int type) {
size_t tagValLen;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you use this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, but we cannot pass NULL to FGC_recvBuffer.

src/fork_gc.c Outdated Show resolved Hide resolved
src/fork_gc.c Outdated Show resolved Hide resolved
Copy link
Collaborator

@MeirShpilraien MeirShpilraien left a comment

Choose a reason for hiding this comment

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

👍
Some small comments

Copy link
Contributor Author

@ashtul ashtul left a comment

Choose a reason for hiding this comment

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

@MeirShpilraien made requested changes

src/fork_gc.c Outdated
uint64_t *id) {
if (FGC_recvBuffer(fgc, (void **)fieldName, fieldNameLen) != REDISMODULE_OK) {
static FGCError recvNumericTagHeader(ForkGC *fgc, tagNumHeader *header, int type) {
size_t tagValLen;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, but we cannot pass NULL to FGC_recvBuffer.

src/fork_gc.c Outdated Show resolved Hide resolved
src/fork_gc.c Outdated Show resolved Hide resolved
IndexIterator **its;
uint32_t uid;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We do not use this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MeirShpilraien
Since I use TagIndex with TagIndex_OpenIndex to retrieve the inverted index of the tag value.

Copy link
Collaborator

@MeirShpilraien MeirShpilraien left a comment

Choose a reason for hiding this comment

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

Last comment

@ashtul ashtul merged commit 4aa6f9b into master Oct 5, 2021
@ashtul ashtul deleted the ariel_empty_tag_gc_fix branch October 5, 2021 18:50
ashtul pushed a commit that referenced this pull request Oct 5, 2021
* [BUG] Remove empty tag values

* Remove comment

* test cursor

* Per Meir review

* clean up

* init ptr

(cherry picked from commit 4aa6f9b)
ashtul pushed a commit that referenced this pull request Oct 6, 2021
* [BUG] Remove empty tag values (#2269)

* [BUG] Remove empty tag values

* Remove comment

* test cursor

* Per Meir review

* clean up

* init ptr

(cherry picked from commit 4aa6f9b)

* fix tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants