Skip to content

fix zslGetRank bug in dead-code#9246

Merged
oranagra merged 1 commit intoredis:unstablefrom
oranagra:fix_skiplist_deadcode_bug
Jul 22, 2021
Merged

fix zslGetRank bug in dead-code#9246
oranagra merged 1 commit intoredis:unstablefrom
oranagra:fix_skiplist_deadcode_bug

Conversation

@oranagra
Copy link
Member

This fixes an issue with zslGetRank which will happen only if the
skiplist data stracture is added two entries with the same element name,
this can't happen in redis zsets (we use dict), but in theory this is a
bug in the underlaying skiplist code.

Fixes #3081 and #4032

This fixes an issue with zslGetRank which will happen only if the
skiplist data stracture is added two entries with the same element name,
this can't happen in redis zsets (we use dict), but in theory this is a
bug in the underlaying skiplist code.

Fixes #3081 and #4032
@oranagra oranagra requested a review from yossigo July 18, 2021 07:29
@enjoy-binbin
Copy link
Contributor

interesting...
Also in zslInsert, the rank is unsigned int, maybe it should be unsigned long

@oranagra
Copy link
Member Author

@enjoy-binbin i agree, but i also seem to remember this or something similar being discussed recently, and i can't locate it (are you aware of something?).
also, this is not strictly related to this PR, and it has a real impact (solve a "problem" with more than 2B entries in a single zset), so if we wanna change it, i suggest doing it in a different PR.

@enjoy-binbin
Copy link
Contributor

enjoy-binbin commented Jul 18, 2021

No, I just saw this pr, it was interesting, and then happened to revisit the implementation of insertion...

I created a new pr: #9249

@yangbodong22011
Copy link
Contributor

i agree, but i also seem to remember this or something similar being discussed recently, and i can't locate it (are you aware of something?).

hi, @oranagra, is here? #8043

@oranagra oranagra merged commit 9ca5e8c into redis:unstable Jul 22, 2021
@oranagra oranagra deleted the fix_skiplist_deadcode_bug branch July 22, 2021 10:40
JackieXie168 pushed a commit to JackieXie168/redis that referenced this pull request Sep 8, 2021
This fixes an issue with zslGetRank which will happen only if the
skiplist data stracture is added two entries with the same element name,
this can't happen in redis zsets (we use dict), but in theory this is a
bug in the underlaying skiplist code.

Fixes redis#3081 and redis#4032

Co-authored-by: minjian.cai <[email protected]>
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.

SkipList zslGetRank clearification

5 participants