Add FLUSHEVERYTHING command that also flushes the Redis Functions. #9923
Conversation
The issue this PR comes to solve is the ability to flush the entire dataset (including functions). Flushing the functions on FLUSHALL command can be dangerous because FLUSHALL can be called from within a function and cause the function structures to be freed while the function is running. In addition, Disallow FLUSHALL inside a function will differentiate functions and eval. The only option left is to introducing FLUSHEVERYTHING command that will flush the entire dataset, including functions.
oranagra
left a comment
There was a problem hiding this comment.
@redis/core-team please take a look as share your thoughts
src/db.c
Outdated
| void flusheverythingCommand(client *c) { | ||
| if (flushCommandsInternal(c) == C_ERR) return; /* Error was already sent to the client */ | ||
| /* Clear functions */ | ||
| functionsCtxClear(functionsCtxGetCurrent()); |
There was a problem hiding this comment.
i suppose we want to flush the EVAL scripts too? in which case maybe we wanna move that into flushAllDataAndResetRDB and control it via some flag?
or alternatively make a dedicated function, and use it also when rdbLoadRio fails?
There was a problem hiding this comment.
Thanks, pushed a fix and also updated the top comment.
The issue this PR comes to solve is the ability to flush the entire dataset (including functions). Flushing the functions on FLUSHALL command can be dangerous because FLUSHALL can be called from within a function and cause the function structures to be freed while the function is running. In addition, Disallow FLUSHALL inside a function will differentiate functions and eval. The only option left is to introducing FLUSHEVERYTHING command that will flush the entire dataset, including functions.
I don't think it would be that hard to work around this, e.g. if called from withing a function, we can postpone the freeing of the function until the function has returned. |
Not sure it will be that simple and not sure it worth it because if we make |
|
I'm not sure.. i do think it's a bit odd to add another flush command where the user can just issue two (flush both data and scripts separately). I'm not yet sure which one is better.. |
|
I think having If anyone feels strongly about flushing more with a single command, I think adding extra arguments to The other topic here is flushing from inside a function - I agree with @zuiderkwast we should just delay that and flush when exiting. I guess we'll be blocking the ability to create a function from within a function right? |
|
I also really don't like flusheverything either because it's unclear what it does compared to FLUSHALL, and would also prefer to add a separate command that just handles flushing of functions. I'm indifferent to adding any type of command that flushes data ands functions, I know we logically are grouping them together but I would expect people to not understand that. For example, someone from the relational world might view functions like schema and would view that independently from data. I would vote for having just |
|
I am Ok with FUNCTION FLUSH. @oranagra @zuiderkwast WDYT? If you agree I will modify the PR. |
|
Ok, let it be.. My main concern is that I think the user should have a single command that does the same thing as what the replica does before loading the rdb file, I.e. A reset of the entire data (function are part of the data). I agree that FLUSHEVRERTHING is confusing and it's hard to understand what's the difference from FLUSHALL, but even if we don't add that new command, it could be confusing, people will wonder if FLUSHALL resets function or not.. |
|
FUNCTION FLUSH 👍 FLUSHALL deletes all keys (like FLUSHDB). ALL refers to the DB numbers here. Simple. EVAL scripts cache is not data. Users shouldn't assume anything about when it's cleared IMO. Is pubsub data? (I think messages are data and channels are client state, but) one could imagine that FLUSHEVERYTHING cancels ongoing subscriptions... or even unblock clients blocked on keys, delete ACL users, LATENCY RESET, SLOWLOG RESET, CONFIG RESETSTAT, MEMORY PURGE, unload modules... |
|
Ok, we seem to have a consensus on scrapping FLUSHEVERYTHING and adding FUNCTION FLUSH. |
|
Will open a new PR and refer to this one, closing. |
The issue this PR comes to solve is the ability to flush the entire data set (including functions which was introduce on #9780).
Why flushing Redis functions can not be added to
FLUSHALL?Flushing the functions on
FLUSHALLcommand can be dangerous becauseFLUSHALLcan be called from within a function and cause the function structures to be freed while the function is running. In addition, DisallowFLUSHALLinside a function will differentiate functions and eval.The solution
Introducing
FLUSHEVERYTHINGcommand that flushes the entire data set, including functions and eval cache. This command is not allowed inside a script (function or eval). The command arguments is just like theFLUSHALLcommand (syncorasync).