redis
redis copied to clipboard
redis
Allow loading RDB with module AUX data even if module is missing.
An issue that we encounter recently is a user that loaded a module that saves AUX data. The user ended up not using the module, but the RDB that was created did contain the module AUX data. The user ended up not been able to load his data without the module (even though he is not using the module).
Notice that even if the module saves nothing on the module AUX save callback, the module AUX load will still be called when loading the RDB and will failed if the module is missing. So the problem can not be solved on the module level and requires changes in Redis.
The Solution
We believe the AUX data is used to save module metadata and it is OK to simply skip it if the module isn't loaded. This is what the PR does, if module is not loaded we just skip its AUX data. To conclude:
- Module not loaded -> skip module aux data
- Module loaded -> call the module load callback
Other Solutions to Consider
The other solutions attempt to recognize the case where the module wrote nothing on the AUX save callback and allow loading the RDB (even if the module is missing) in this case only. Note that we'll need some mechanism to read-head the next module type opcode so we can detect the EOF marker without damaging the parsing.
-
Skip module AUX data only if no data was actually written to the RDB by the module AUX save callback and the module is not loaded. If the module is loaded we can not skip calling the load callback because otherwise it can be considered a breaking change. So we have to pass the responsibility of not reading any data to the module, but the module can not know that he has nothing to read without reading something :). So we need to give the module an ability to check that there is nothing to read. We can do it with another module API,
RM_LoadHasData, that will tell the module if something was written to the AUX field. To conclude:- Aux field is empty ->
- Module not loaded -> skip
- Module loaded -> call the module load callback, module will need to use
RM_LoadHasDatato know if there is a data to read.
- Aux field not empty ->
- Module not loaded -> fail
- Module loaded -> call the module load callback.
- Aux field is empty ->
-
Break the existing API and decide that if nothing was written on the save callback then we will not call the load callback (even if the module is exists). This can be considered a breaking change but it is easier solution then (1). To conclude:
- Aux field is empty ->
- Module not loaded -> skip
- Module loaded -> skip
- Aux field not empty ->
- Module not loaded -> fail
- Module loaded -> call the module load callback.
- Aux field is empty ->
-
Same as 2 but with some datatype flag to avoid breaking change. To conclude:
- Aux field is empty ->
- Module not loaded -> skip
- Module loaded -> check datatype flag to decide whether or not to call the module load callback
- Aux field not empty ->
- Module not loaded -> fail
- Module loaded -> call the module load callback.
- Aux field is empty ->
We do believe the solution in the PR is good enough but we want to hear other opinions.
We had a discussion about this with Yossi, we concluded that it's better / safer to let the module that saved this field to declare if loading it is optional or not, so that human error forgetting to load a module on restart or a replica would not lead to loss of actual data (in case there's meaningful data in the aux field even when there is no registered data type, or no keys of that data type). This requires an RDB format change..
Sorry for chiming late. I agree that I don't think we should just "skip" module data.
Adding a change where a module can declare if auxiliary data is optional seems like a reasonable mitigation.
IMHO, the module "aux" data has no difference with module data, we should skip them both or just exit.
We can expand the original problem in this PR, imagine that a user loaded a module with module type, and did insert some module values, and then this user cannot load data without the module even they wanna discard the module's values and just keep other types of data.
To fix it, I think we can add a new configuration for users to decide whether skip the module aux and value or not if module is missing.
the difference is that for keys, the user can easily delete the module keys before saving the rdb. for aux it's not that simple, the module can't easily avoid saving it even if it knows it's gonna be empty, and it can't even just return without saving anything (when it'll be impossible for the module to load such an RDB, the callback will be called, but the module has no way to detect that it's empty).
i think the viable solution is either to delay saving the aux field headers until the module actually saved something to the rdb, in which case, if it doesn't save anything, nothing will be present and it'll not get a callback on loading.
or as suggested by Yossi, to let the module declare that the payload is optional and can be skipped. in which case the module can store a nearly empty payload, which it'll be able to read.
I still prefer a new config, it's easy to understand and use, and can save the time caused by deleting the module's keys (SCAN and DEL?).
or as suggested by Yossi, to let the module declare that the payload is optional and can be skipped. in which case the module can store a nearly empty payload, which it'll be able to read.
I don't understand, if a module declare the aux filed cannot be skipped, users cannot load RDB without the module.
I'm confused, the problem you wanna fix is loss of data or the restart failure without module?
the problem i wanna solve is a case were someone has an instance with a module which he didn't use, or use it and then stopped using it, and now he wants to restart the instance without that module. but if the module always saves some global aux field we either need to:
- allow the module to avoid saving the aux field.
- allow the module to save an empty one which can be somehow implicitly skipped (both if the module is present, or missing)
- allow the module to declare this is optional, and can be safely skipped even if not empty.
i don't think we need to solve a similar problem about module keys, users should delete them. the reason for this discussion is that for aux field there's simply no solution currently (complete dead end), specifically since modules can't avoid saving them, and if they'll choose to save an empty one, they have no way to handle it on load time.
the problem i wanna solve is a case were someone has an instance with a module which he didn't use, or use it and then stopped using it, and now he wants to restart the instance without that module.
OK, then I think the approach in this PR is enough, especially considering backward compatibility.
- allow the module to declare this is optional, and can be safely skipped even if not empty.
I don't like this method, IIUC module can still save aux field and declare the field must not be skipped, and then the restart will fail.
the problem with the current approach in this PR is that it could be that some modules save their entire data in the aux field, and silently skipping it could cause a real data loss for a user (which could be discovered when it's too late) in the event of human error of forgetting to load the module.
as noted, we could have redis delay saving the aux headers until the module actually saves something, and then if it didn't save anything, it won't get a load callback. but maybe we consider that a breaking change, since maybe some module used these callbacks as hooks to be notified of things, and didn't save anything anyway, so it'll miss the hook.
the other options are to add module APIs:
- callback to let the module decide if the aux field should be saved or not (doesn't require RDB format change).
- let the module declare that the aux field is optional in which case it can be skipped if the module is missing, even if the data isn't empty (requires rdb format change).
another option is like you said, add a config to let the user decide if it's safe to skip data that's not loadable, and can be applied on keys as well (i don't like it that much, since i don't think it should be the user's decision).
if we do want it to be a user's decision, maybe it should be an external tool (like rdb-cli) that can process the rdb, and trim things from it (could even be keys by some regex pattern). so we shift the problem to a tool rather than redis.
if we do want it to be a user's decision, maybe it should be an external tool (like rdb-cli) that can process the rdb, and trim things from it (could even be keys by some regex pattern). so we shift the problem to a tool rather than redis.
vote for this one.