Allow loading RDB with module AUX data even if module is missing. #11056
Conversation
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 its not there. 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 recongnize 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.
1. 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 consdidered 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_LoadHasData` to know if there is a data to read.
* Aux field not empty ->
* Module not loaded -> fail
* Module loaded -> call the module load callback.
2. 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 its 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.
3. 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.
We do believe the solution in the PR is good enough but we want to hear other opinions.
oranagra
added
state:major-decision
|
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). |
oranagra
removed
release-notes
|
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. 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. |
|
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?).
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.
i don't think we need to solve a similar problem about module keys, users should delete them. |
OK, then I think the approach in this PR is enough, especially considering backward compatibility.
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:
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). |
vote for this one. |
As describe here: redis#11056 The issue is that when saving an RDB with module AUX data, the module AUX metadata (moduleid, when, ...) is saved to the RDB even though the module did not saved any actual data. This prevent loading the RDB in the absence of the module (although there is no actual data in the RDB that requires the module to be loaded). On redis#11056, the solution suggested was the ignore AUX data if the module is not loaded. This solution is wrong because we have data that we ignore and allow the server to start without it. Other solution that was suggested is to provide a new module API to save optional AUX fields. This solution will solve the problem but it is probably more then we actually need and we want to avoid new module API and new RDB versions if possible. The solution suggested in this PR is that module AUX will be saved on the RDB only if the module acutally saved something during `aux_save` function. For modules that saved some data on the RDB, this PR will make no difference. We also claim that any valid used case of AUX data must save some data on `aux_save` function (Even if a module wants an indication that he has no data to save, it needs to save this indication as data). So the only difference this PR makes is for modules that **never** save data on the `aux_save` function and just used it to know that a saving operation started/ended. For this purpose we have server events, using module AUX for this purpose is wrong and we believe its ok to break it. For such modules, the `aux_load` function will now **not** be called. ### Technical Details To avoid saving AUX data on RDB, we change to code to first save the AUX metadata (moduleid, when, ...) into a temporary buffer. The buffer is then flushed to the rio at the first time the module makes a write operation inside the `aux_save` function. If the module saves nothing, the entire temporary buffer is simply dropped and no data about this AUX field is saved to the RDB. This make it possible to load the RDB even in the absence of the module. Test was added to verify the fix.
As describe here: redis#11056 The issue is that when saving an RDB with module AUX data, the module AUX metadata (moduleid, when, ...) is saved to the RDB even though the module did not saved any actual data. This prevent loading the RDB in the absence of the module (although there is no actual data in the RDB that requires the module to be loaded). On redis#11056, the solution suggested was the ignore AUX data if the module is not loaded. This solution is wrong because we have data that we ignore and allow the server to start without it. Other solution that was suggested is to provide a new module API to save optional AUX fields. This solution will solve the problem but it is probably more then we actually need. The solution suggested in this PR is that module AUX will be saved on the RDB only if the module acutally saved something during `aux_save` function. To support backward compatibility, we introduce `aux_save2` callback that acts the same as `aux_save` with the tiny change of avoid saving the aux field if no data was actually saved by the module. Modules can use the new API to make sure that if they have no data to save, then it will be possible to load the created RDB even without the module. To avoid saving AUX data on RDB, we change the code to first save the AUX metadata (moduleid, when, ...) into a temporary buffer. The buffer is then flushed to the rio at the first time the module makes a write operation inside the `aux_save` function. If the module saves nothing (and `aux_save2` was used), the entire temporary buffer is simply dropped and no data about this AUX field is saved to the RDB. This make it possible to load the RDB even in the absence of the module. Test was added to verify the fix.
|
Replaced by: #11374 |
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:
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:RM_LoadHasDatato know if there is a data to read.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:
Same as 2 but with some datatype flag to avoid breaking change. To conclude:
We do believe the solution in the PR is good enough but we want to hear other opinions.