fix some bug about sds and quicklist #4674

yebaoshan · 2018-02-09T02:46:31Z

No description provided.

sundb · 2021-06-30T13:56:11Z

@yebaoshan I see that there is a lot of commented code in the code, can you improve it?

sundb · 2021-07-13T07:25:10Z

src/quicklist.c

@@ -854,7 +857,7 @@ REDIS_STATIC void _quicklistInsert(quicklist *quicklist, quicklistEntry *entry,
        full = 1;
    }

-    if (after && (entry->offset == node->count)) {
+    if (full && after && (entry->offset == node->count - 1)) {


ping @oranagra, (entry->offset == node->count - 1)
This is a minor error, and does not cause any side effects.
Like to #9113.

sundb · 2021-07-13T07:26:03Z

src/quicklist.c

        new_node->count++;
+        quicklistNodeUpdateSz(new_node);


This is also a minor error, but it's dead code, maybe we can delete it.

Here the size of the new quicklistNode inserted will be 0.

oranagra · 2021-07-13T09:53:47Z

This PR is a bit sloppy (it hurts my eyes 😃).
title is very broad ("some bug") and no description.
combined with the fact it's not a trivial one line fix, makes it harder to review.

so @yebaoshan if you're still here, i invite you to improve, but it looks like you moved away by now.

@sundb thank you for looking into it 💯
can you please save me the effort and tell me what exactly i should focus on, or if it's just a small subset of the changes here, just open a new focused PR?
sadly i'm not too familiar with quicklists, and also quite busy.
did you mean that this fix is overlapping with the other PR, or fixes a similar problem (i.e. we need to merge both)?
p.s. i do think we need to fix bugs in dead code, it might come to life one day..

sundb · 2021-07-13T10:03:57Z

@oranagra These two pr do not overlap, but the result is the same.
#4674 causes at_tail to always be 0, which will eventually go to quicklist.c#L938
##9113 When inserting the top/bottom of the quicklist, a new quicklistNode should have been created, but it ended up going to quicklist.c#L938

In my two comments above, I think these two changes are OK, the changes to the sds I feel are unnecessary, they are just optimization and not bugs.

sundb · 2021-07-13T10:06:02Z

src/quicklist.c

@@ -841,8 +843,9 @@ REDIS_STATIC void _quicklistInsert(quicklist *quicklist, quicklistEntry *entry,
        D("No node given!");
        new_node = quicklistCreateNode();
        new_node->zl = ziplistPush(ziplistNew(), value, sz, ZIPLIST_HEAD);
-        __quicklistInsertNode(quicklist, NULL, new_node, after);
+        __quicklistInsertNode(quicklist, quicklist->tail, new_node, after);


In the old code, this line of code would create a wild pointer.

sundb · 2021-07-13T10:08:12Z

src/quicklist.c

        new_node->count++;
+        quicklistNodeUpdateSz(new_node);


Here the size of the new quicklistNode inserted will be 0.

sundb · 2021-07-13T10:22:20Z

src/quicklist.c

@@ -378,6 +378,8 @@ REDIS_STATIC void __quicklistInsertNode(quicklist *quicklist,
        quicklist->head = quicklist->tail = new_node;
    }

+    /* Should try to compress new_node, it may be not at head or tail */
+    quicklistCompress(quicklist, new_node);


In the existing code, old_node will never be NULL, so new_node will be compressed in the following quicklistCompress(quicklist, old_node); code, so this change is not necessary.

Some background: This fixes a problem that used to be dead code till now, but became alive (only in the unit tests, not in redis) when #9113 got merged. The problem it fixes doesn't actually cause any significant harm, but that PR also added a test that fails verification because of that. This test was merged with that problem due to human error, we didn't run it on the last modified version before merging. The fix in this PR existed in #8641 (closed because it's just dead code) and #4674 (still pending but has other changes in it). Now to the actual fix: On quicklist insertion, if the insertion offset is -1 or `-(quicklist->count)`, we can insert into the head of the next node rather than the tail of the current node. this is especially important when the current node is full, and adding anything to it will cause it to be split (or be over it's fill limit setting). The bug was that the code attempted to determine that we're adding to the tail of the current node by matching `offset == node->count` when in fact it should have been `offset == node->count-1` (so it never entered that `if`). and also that since we take negative offsets too, we can also match `-1`. same applies for the head, i.e. `0` and `-count`. The bug will cause the code to attempt inserting into the current node (thinking we have to insert into the middle of the node rather than head or tail), and in case the current node is full it'll have to be split (something that also happens in valid cases). On top of that, since it calls _quicklistSplitNode with an edge case, it'll actually split the node in a way that all the entries fall into one split, and 0 into the other, and then still insert the new entry into the first one, causing it to be populated beyond it's intended fill limit. This problem does not create any bug in redis, because the existing code does not iterate from tail to head, and the offset never has a negative value when insert. The other change this PR makes in the test code is just for some coverage, insertion at index 0 is tested a lot, so it's nice to test some negative offsets too.

…is#9311) Some background: This fixes a problem that used to be dead code till now, but became alive (only in the unit tests, not in redis) when redis#9113 got merged. The problem it fixes doesn't actually cause any significant harm, but that PR also added a test that fails verification because of that. This test was merged with that problem due to human error, we didn't run it on the last modified version before merging. The fix in this PR existed in redis#8641 (closed because it's just dead code) and redis#4674 (still pending but has other changes in it). Now to the actual fix: On quicklist insertion, if the insertion offset is -1 or `-(quicklist->count)`, we can insert into the head of the next node rather than the tail of the current node. this is especially important when the current node is full, and adding anything to it will cause it to be split (or be over it's fill limit setting). The bug was that the code attempted to determine that we're adding to the tail of the current node by matching `offset == node->count` when in fact it should have been `offset == node->count-1` (so it never entered that `if`). and also that since we take negative offsets too, we can also match `-1`. same applies for the head, i.e. `0` and `-count`. The bug will cause the code to attempt inserting into the current node (thinking we have to insert into the middle of the node rather than head or tail), and in case the current node is full it'll have to be split (something that also happens in valid cases). On top of that, since it calls _quicklistSplitNode with an edge case, it'll actually split the node in a way that all the entries fall into one split, and 0 into the other, and then still insert the new entry into the first one, causing it to be populated beyond it's intended fill limit. This problem does not create any bug in redis, because the existing code does not iterate from tail to head, and the offset never has a negative value when insert. The other change this PR makes in the test code is just for some coverage, insertion at index 0 is tested a lot, so it's nice to test some negative offsets too.

CLAassistant · 2024-03-24T23:29:11Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

yebaoshan seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

fix some bug about sds and quicklist

a7d234c

sundb reviewed Jul 13, 2021

View reviewed changes

sundb mentioned this pull request Aug 3, 2021

Fix head and tail check with negative offset in _quicklistInsert #9311

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix some bug about sds and quicklist #4674

fix some bug about sds and quicklist #4674

yebaoshan commented Feb 9, 2018

sundb commented Jun 30, 2021

sundb Jul 13, 2021

sundb Jul 13, 2021

sundb Jul 13, 2021

oranagra commented Jul 13, 2021

sundb commented Jul 13, 2021

sundb Jul 13, 2021

sundb Jul 13, 2021

sundb Jul 13, 2021

CLAassistant commented Mar 24, 2024

fix some bug about sds and quicklist #4674

Are you sure you want to change the base?

fix some bug about sds and quicklist #4674

Conversation

yebaoshan commented Feb 9, 2018

sundb commented Jun 30, 2021

sundb Jul 13, 2021

Choose a reason for hiding this comment

sundb Jul 13, 2021

Choose a reason for hiding this comment

sundb Jul 13, 2021

Choose a reason for hiding this comment

oranagra commented Jul 13, 2021

sundb commented Jul 13, 2021

sundb Jul 13, 2021

Choose a reason for hiding this comment

sundb Jul 13, 2021

Choose a reason for hiding this comment

sundb Jul 13, 2021

Choose a reason for hiding this comment

CLAassistant commented Mar 24, 2024