Skip to content

Obliterate STRALGO! add LCS (which only works on keys)#9799

Merged
oranagra merged 1 commit intoredis:unstablefrom
guybe7:no_stralgo
Nov 18, 2021
Merged

Obliterate STRALGO! add LCS (which only works on keys)#9799
oranagra merged 1 commit intoredis:unstablefrom
guybe7:no_stralgo

Conversation

@guybe7
Copy link
Copy Markdown
Collaborator

@guybe7 guybe7 commented Nov 17, 2021 •
edited by oranagra
Loading

Fix #9744

Drop the STRALGO command, now LCS is a command of its own and it only works on keys (not input strings).
The motivation is that STRALGO's syntax was really messed-up...

  • assumes all (future) string algorithms will take similar arguments
  • mixes command that takes keys and one that doesn't in the same command.
  • make it nearly impossible to expose the right key spec in COMMAND INFO (issues cluster clients)
  • hard for cluster clients to determine the key names (firstkey, lastkey, etc)
  • hard for ACL / flags (is it a read command?)

This is a breaking change.

@guybe7
No more STRALGO!
4d7817f 
Fix redis#9744

Drop the STRALGO command, now LCS is a command of its own.
The motivation is that STRALGO's syntax was really messed-up...
- assumes all (future) string algorithms will take similar arguments
- mix command take takes keys and one that doesn't in the same command.
- make it nearly impossible to expose the right key spec in COMMAND INFO (issues cluster clients)

This is a breaking change.
@guybe7
Copy link
Copy Markdown
Collaborator Author

guybe7 commented Nov 17, 2021

best PR ever

@yossigo yossigo added breaking-change This change can potentially break existing application release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus labels Nov 17, 2021
@yossigo yossigo requested a review from a team November 17, 2021 16:57
@oranagra oranagra changed the title No more STRALGO! Obliterate STRALGO! add LCS (which only works on keys) Nov 17, 2021
@oranagra oranagra added the approval-needed Waiting for core team approval to be merged label Nov 17, 2021
madolson
madolson approved these changes Nov 18, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@madolson madolson left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Fine, I still stand behind this shouldn't be a Redis command. EDIT: I mean that we should just delete STRALGO and not replace it. I doubt there is any real usage of it in production workloads.

@oranagra oranagra merged commit af74898 into redis:unstable Nov 18, 2021
@sundb
Copy link
Copy Markdown
Collaborator

sundb commented Nov 19, 2021

It seems that corrupt-dump-fuzzer fail due to the change(the last command is all LCS).
https://github.com/sundb/redis/runs/4259577358?check_suite_focus=true
https://github.com/sundb/redis/runs/4259577222?check_suite_focus=true

@oranagra
Copy link
Copy Markdown
Member

oranagra commented Nov 20, 2021

i dug into it and this is what i found (hope no one else wastes his time on this)

  1. till now the fuzzer was unable to produce valid LCS commands, so it didn't really test it.. (one more justification to the change we made. STRALGO was wrong).
  2. it only fails on the daily CI, and not on the per PR CI, because the failure is rare, and the daily CI uses --accurate, which changes the fuzzer to run for 10 minutes instead of 10 seconds.
  3. the failure is due to the following sequence SETRANGE _int 423324 1450173551 and LCS _int _int
  4. when i run this sequence on my PC, LCS returns ERR Insufficient memory (this is because it now uses ztrymalloc
  5. but on GH Actions, it doesn't fail allocating, and instead it seems to get OOM killed by the kernel.

maybe we should make another PR to completely obliterate LCS from redis (for being a memory hog)
i.e. easy to be used to evict all the data, or get OOM killed.

@oranagra
Copy link
Copy Markdown
Member

oranagra commented Nov 20, 2021

following a discussion with @madolson the idea came up to limit the transient memory allocation of LCS to proto-max-bulk-len.
It's not rally related to is, and there could be systems in which this is too high (order of magnitude higher than maxmemory?), and arguably there may be a case where the limit is too low for someone, but I suppose such users would be able to tune their limit to higher, and also maybe it's not a realistic usage anyway.
@yossigo WDYT?

@oranagra oranagra mentioned this pull request Nov 21, 2021
Merged
@oranagra
Copy link
Copy Markdown
Member

oranagra commented Nov 21, 2021

handled in #9817

oranagra added a commit that referenced this pull request Nov 21, 2021
@oranagra
Prevent LCS from allocating temp memory over proto-max-bulk-len (#9817)
1417648 
LCS can allocate immense amount of memory (sizes of two inputs multiplied by each other).
In the past this caused some possible security issues due to overflows, which we solved
and also added use of `trymalloc` to return "Insufficient memory" instead of OOM panic zmalloc.

But in case overcommit is enabled, it could be that we won't get the OOM panic, and zmalloc
will succeed, and then we can get OOM killed by the kernel.

The solution here is to prevent LCS from allocating transient memory that's bigger than
`proto-max-bulk-len` config.
This config is not directly related to transient memory, but using a hard coded value ad well as
introducing a specific config seems wrong.

This comes to solve an error in the corrupt-dump-fuzzer test that started in the daily CI see #9799
hwware pushed a commit to hwware/redis that referenced this pull request Dec 20, 2021
@oranagra @hwware
Prevent LCS from allocating temp memory over proto-max-bulk-len (redi…
2828676 
…s#9817)

LCS can allocate immense amount of memory (sizes of two inputs multiplied by each other).
In the past this caused some possible security issues due to overflows, which we solved
and also added use of `trymalloc` to return "Insufficient memory" instead of OOM panic zmalloc.

But in case overcommit is enabled, it could be that we won't get the OOM panic, and zmalloc
will succeed, and then we can get OOM killed by the kernel.

The solution here is to prevent LCS from allocating transient memory that's bigger than
`proto-max-bulk-len` config.
This config is not directly related to transient memory, but using a hard coded value ad well as
introducing a specific config seems wrong.

This comes to solve an error in the corrupt-dump-fuzzer test that started in the daily CI see redis#9799
@oranagra oranagra mentioned this pull request Mar 22, 2022
Closed
@enjoy-binbin enjoy-binbin mentioned this pull request Mar 28, 2022
Merged
@uki00a uki00a mentioned this pull request Oct 20, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yossigo yossigo yossigo approved these changes

@oranagra oranagra oranagra approved these changes

@itamarhaber itamarhaber Awaiting requested review from itamarhaber

+2 more reviewers

@soloestoy soloestoy soloestoy approved these changes

@madolson madolson madolson approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

approval-needed Waiting for core team approval to be merged breaking-change This change can potentially break existing application release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus

Projects

Redis 7.0
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[CHANGE] STRALGO LCS doesn't make sense

6 participants

@guybe7 @sundb @oranagra @yossigo @soloestoy @madolson