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

fix wrong data type conversion in zrangeResultBeginStore #13148

Merged
merged 4 commits into from Mar 19, 2024

Conversation

lyq2333
Copy link
Contributor

@lyq2333 lyq2333 commented Mar 15, 2024

In beginResultEmission, -1 means the result length is not known in advance. But after #12185, if we pass -1 to zrangeResultBeginStore, it will convert to SIZE_MAX in zsetTypeCreate and try to dictExpand. Although dictExpand won't succeed because the size overflows, I think we'd better to avoid this wrong conversion.

This bug can be triggered when the source of zrangestore doesn't exist or we use zrangestore command with byscore or bylex.
The impact is that dst keys will be converted to use skiplist instead of listpack.

@lyq2333
Copy link
Contributor Author

lyq2333 commented Mar 15, 2024

@enjoy-binbin Hi, could you please help review this PR? Thanks a lot.

Copy link
Collaborator

@enjoy-binbin enjoy-binbin left a comment

Choose a reason for hiding this comment

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

LGTM, please add a comment in zsetTypeCreate to mention the -1 thing, and please also try to find a place to add a test:

zadd src 1 a
zrangestore dst src 0 -1
assert_encoding listpack dst

please also merge upstream/unstable to solve the failing CI

src/t_zset.c Outdated Show resolved Hide resolved
@lyq2333
Copy link
Contributor Author

lyq2333 commented Mar 18, 2024

LGTM, please add a comment in zsetTypeCreate to mention the -1 thing, and please also try to find a place to add a test:

zadd src 1 a
zrangestore dst src 0 -1
assert_encoding listpack dst

@enjoy-binbin Sorry, I made a mistake in the top comment. This bug can be triggered when the source of zrangestore doesn't exist, not the destination. So It's just an intermediate state.

I add the tests for zrangestore with byscore or bylex which can also trigger it.

Copy link
Collaborator

@enjoy-binbin enjoy-binbin left a comment

Choose a reason for hiding this comment

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

thanks.

@oranagra the impact of this is that some dst keys will be converted to use skiplist

@oranagra oranagra merged commit bad33f8 into redis:unstable Mar 19, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants