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 XTRIM or XADD with LIMIT may delete more entries than Count. #9048

Merged
merged 4 commits into from Jun 7, 2021

Conversation

huangzhw
Copy link
Collaborator

@huangzhw huangzhw commented Jun 4, 2021

The decision to stop trimming due to LIMIT in XADD and XTRIM was after the limit was reached.
i.e. the code was deleting at least that count of records (from the LIMIT argument's perspective, not the MAXLEN),
instead of up to that count of records.
see #9046

src/t_stream.c Outdated Show resolved Hide resolved
tests/unit/type/stream.tcl Outdated Show resolved Hide resolved
@oranagra
Copy link
Member

oranagra commented Jun 6, 2021

so you're saying it's an "off by one" bug. i.e. checking the limit after deletion rather than before. right?

but note that what we documented is:

The ~ argument between the MAXLEN strategy and the threshold means that the user is requesting to trim the stream so its length is at least the threshold, but possibly slightly more

so are you sure there's a bug? or maybe we just lost our way...

@huangzhw
Copy link
Collaborator Author

huangzhw commented Jun 6, 2021

Checking the limit is still before deletion. For example, limit is 10, deletion is 9, the lenght of next node to delete is 5. The old code will delete the 5 node, so deletion is 14. The new code will not delete the node, so deletion is still 9.

@oranagra
Copy link
Member

oranagra commented Jun 6, 2021

ohh, never mind. i mixed the documentation of MAXLEN with the LIMIT feature (two different features)

@oranagra oranagra added this to To do in 6.2 Backport via automation Jun 6, 2021
@oranagra oranagra added the release-notes indication that this issue needs to be mentioned in the release notes label Jun 6, 2021
@oranagra
Copy link
Member

oranagra commented Jun 6, 2021

i've edited the top comment, let me know (or fix it), if wrong.

@oranagra oranagra added the state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten label Jun 6, 2021
@huangzhw
Copy link
Collaborator Author

huangzhw commented Jun 6, 2021

The old code might still delete less records than count, if the records to delete is less than required. So whether should make it more accurate.

@huangzhw huangzhw changed the title Fix XTRIM or XADD with LIMIT may delete more entries than limit. Fix XTRIM or XADD with LIMIT may delete more entries than Count. Jun 6, 2021
@oranagra oranagra merged commit eaa7a7b into redis:unstable Jun 7, 2021
@huangzhw huangzhw deleted the stream-xtrim-limit branch June 7, 2021 11:49
oranagra pushed a commit to oranagra/redis that referenced this pull request Jul 20, 2021
…is#9048)

The decision to stop trimming due to LIMIT in XADD and XTRIM was after the limit was reached.
i.e. the code was deleting **at least** that count of records (from the LIMIT argument's perspective, not the MAXLEN),
instead of **up to** that count of records.
see redis#9046

(cherry picked from commit eaa7a7b)
@oranagra oranagra mentioned this pull request Jul 21, 2021
oranagra pushed a commit that referenced this pull request Jul 21, 2021
The decision to stop trimming due to LIMIT in XADD and XTRIM was after the limit was reached.
i.e. the code was deleting **at least** that count of records (from the LIMIT argument's perspective, not the MAXLEN),
instead of **up to** that count of records.
see #9046

(cherry picked from commit eaa7a7b)
@oranagra oranagra moved this from To Do to Done in 6.2 Backport Jul 22, 2021
JackieXie168 pushed a commit to JackieXie168/redis that referenced this pull request Sep 8, 2021
…is#9048)

The decision to stop trimming due to LIMIT in XADD and XTRIM was after the limit was reached.
i.e. the code was deleting **at least** that count of records (from the LIMIT argument's perspective, not the MAXLEN),
instead of **up to** that count of records.
see redis#9046
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes indication that this issue needs to be mentioned in the release notes state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants