- remove the option to disable AOF preamble RDB this adds a lot of code to redis and modules
- delete all the code in rewriteAppendOnlyFileRio and deprecate modules aof_rewrite callback
- ~always generate base rdb (even when starting empty)~ (done in #10102)
- disallow BGREWRITEAOF command when appendonly is off?
details: https://github.com/redis/redis/pull/9539#issuecomment-966244053
Comment From: oranagra
For the record. we discussed these in a core-team meeting. we concluded that that we wanna proceed with the intent to generate an empty preamble / rdb when the server starts empty.
however, we concluded that we don't want to delete the code in rewriteAppendOnlyFileRio and deprecate the preamble config yet. one of the reasons is that there are users and tools that wish to convert and RDB to commands, and some of these may want to use the module api for aofrw in order to convert the module keys to commands. once we delete the AOFRW code, the module API becomes dead code (only used for compatibility with old redis), and module developers can't test it even if they wanted to.
Comment From: chenyang8094
So, I understand that we gave up these plans, right?
Comment From: oranagra
as far as i'm aware, we only gave up the first two bullets on the top, we do wanna implement the 3rd, and maybe the 4th too.
Comment From: yossigo
@oranagra This is what I understand and support as well.
Always having a base RDB simplifies and streamlines persistence.
Same goes for disabling BGREWRITEAOF, unless anyone comes up with a good argument for maintaining support for it.
Comment From: oranagra
discussed this again in the core-team meeting. decided to proceed with items 3 and 4 in the top list
Comment From: chenyang8094
I think this needs more detailed description, or I will mention a draft separately when I have time.
Comment From: oranagra
number 4 seems quite straight forward.
as for 3, i imagine just doing a foreground rdbSave on startup (only if preamble is enabled, we're configured to persist to AOF, and there's no pre-existing AOF we loaded from).
maybe when implementing it we'll face some dilemmas.
Comment From: chenyang8094
several questions:
1. why “always generate base rdb (even when starting empty)” ? Just to avoid the situation where there is only incr file and no base file?
2. If aof-use-rdb-preamble is set to no, do we still have to do this? Or force an empty AOF format BASE?
3. Do we need to force create base if AOF is not turned on (loading RDB) ?
4. If we want to force create a base, why not trigger the AOFRW in the background directly instead of doing it in the foreground? Just to avoid the cost of a fork?
5. why “disallow BGREWRITEAOF command when appendonly is off” ? Just to avoid the situation where there is only base file and no incr file? This is a breaking change, will it affect existing users.
Comment From: oranagra
i don't recall if this was already specified or not, so i'll just respond instead of trying to find it.
the main reasons for always generating a base file are: 1. when preamble is on, the vast majority of cases where someone will look at the aof folder (or aof file, pre redis 7), it'll have an rdb base. so the case where it's missing is an anomaly and some users / systems may assume it never happens and fail if / when they see it. 2. some complex modules are loaded with a configuration at startup, and they persist that configuration into the RDB aux fields to be available for the replica and when recovering from persistence. without this info, the persistence may be incomplete or unusable. such a module would want the RDB aux fields to always be available in the AOF.
This feature of generating a base file when starting up empty isn't needed if preamble is disabled IMHO.
regarding disallowing BGREWRITEAOF, i think it complicates redis (IIRC we saw 2 bugs recently because of that feature), and i don't see why that feature is needed. so i thought it's a good idea to remove it.
Comment From: chenyang8094
@oranagra Do you mean that when redis starts and finds that base does not exist, it generates an RDB in the foreground and then loads it?
IIRC the previous AOF mechanism did not do this? The preamble is written to the AOF only after the first AOFRW.
Comment From: oranagra
I meant that if redis starts configured for appendonly yes, and there's no AOF file loaded (redis starts up empty, and configured to persist to AOF), then i'd like it to do a foreground RDB save (with the AOF flag in it's header) to generate a base file, and put it in the manifest.
since that base file is empty, there's no need to load it IMHO, but i don't mind loading it if you feel that's better.
Comment From: oranagra
yes, the previous AOF (with preamble rdb header) didn't have that mechanism, but in theory it could be added there too (if we wanted to "improve" redis 6.2)
Comment From: chenyang8094
If we only have a INCR aof when redis starts, do we need to force create a BASE ?
Comment From: oranagra
If we only have a INCR aof when redis starts, do we need to force create a BASE ?
no, only create an empty base file if nothing was loaded on startup. IMHO there's no need to load it. same as we don't load the RDB file we just generated after SAVE or BGSAVE, and don't load the AOF after an AOFRW (it should have the same content as what we already have, which is an empty redis, with some modules and their configuration).
Comment From: chenyang8094
I got to know some Chinese cloud companies. They allow customers to use BGREWRITEAOF even AOF is off to generate an AOF file which will be used for backup, clone and replay instance , analysis (depending on AOF readability) , use to troubleshoot. I think the breaking change of disable BGREWRITEAOF is the fatal blow for these cloud services.
Comment From: soloestoy
I think it's better to allow BGREWRITEAOF when appendonly is off, and @chenyang8094 has already gave the examples above.
If you want more detailed examples, here is a document we (Alibaba cloud) help people to use aof files to migrate data, https://www.alibabacloud.com/help/en/doc-detail/26357.html , in this doc we suggest users open aof to migrate data, but as I know some users still like close appendonly and just use BGREWRITEAOF, because they don't want to consume too much IO and fork.
Comment From: chenyang8094
Another if you want: https://support.huaweicloud.com/intl/en-us/migration-dcs/dcs-migration-0312010.html
Comment From: oranagra
Thank you. Indeed these documents don't mention BGREWRITEAOF, but i got the point. I suppose these documents are a little bit old (they don't mention you need to disable preamble rdb, so they're already kinda broken). I suppose that if someone would have wrote such a document today, it'll recommend using RedisShake or a similar tool (redis-rdb-tools --protocol). I suppose that starting redis 7.0, if this document would have been updated, it should mention both disabling preamble, but also digging into the aof folder and manifest, to find the base file that was created.
so considering all of that, do you really think blocking BGREWRITEAOF in that scenario would be a breaking change in 7.0?
p.s. i still didn't give up the desire to completely delete the rewriteAppendOnlyFileRio logic and deprecate the ability to disable preamble some day (i think it complicates redis).
Comment From: chenyang8094
@oranagra May be you right, but i think we can't give up compatibility just because a thing will cause complexity, and at least so far I haven't found how complicated it is and what problems it will cause, if there is, please point it out, I'll be happy to solve it this matter.
In addition, we have found that there are many cloud vendors and users using this method in reality, can we really ignore them directly? We can tolerate multiple redis sharing the same dir, we can accept that they upgrade redis 7.0 without changing their backup script, we can accept that appendfilename contains spaces, why can't we accept users to use BGREWRITEAOF when AOF is off？
So, do we have a unified standard for judging whether a feature is retained? That is, keeping compatibility first, reducing redis complexity second?
Comment From: chenyang8094
Also,IIRC, redis has a limit on the size of a single RESP or inputbuf (maybe 500MB?). Some tools will obtain AOF or RDB from a redis, then parse and replay them to another redis. At this time, if we have a key that exceeds 500MB, if the RDB is replayed, the corresponding single restore cmd may exceed 500MB , which will cause the replay to fail. On the contrary, these data structures will be split into multiple commands when rewriting AOF (for example, the hash will be split into multiple hmset), which can solve the problem of replaying bigkey.
So, removing rewriteAppendOnlyFileRio will make us lose the possibility of dealing with such problems.
Comment From: oranagra
there's obviously no standard way to decide, we just try to do what we think is best.
and this feature (BGREWRITEAOF when AOF is off), isn't adding a huge overhead, just some small random edge cases that surface from time to time (the rewriteAppendOnlyFileRio mechanism is a huge overhead though).
I suppose it's just that i wasn't aware of the use cases you're mentioning now, which is why i thought we can chuck it. but now that i am aware of it, and reading the links you sent, i think they're already broken, and will become even more broken in 7.0 due to the manifest thing.
so the advise in these links doesn't seem very useful, and maybe we should come up with a different solution for these use cases in the future. like making some librdb.so out of rdb.c, and add some feature in redis-cli that can convert an RDB file into command stream and send it to some target.
After this discussion, I guess i don't mind closing this issue and leaving that thing as is. but i must say i don't see a bright future for this "feature", the use case for it is better served in other ways.
Comment From: oranagra
regarding your last post about proto-max-bulk-len and client-query-buffer-limit and the issue with a huge RESTORE command. that's a known issue and the roadmap has other solution for it. RESP3 has a chunked feature that should allow sending an RDB payload to RESTORE command in a cut-through way (without buffering to collect the full size in advance), and redis should be able to process the RESTORE command in the background in a non-blocking way. (maybe a feature for redis 8). meanwhile, indeed the only way around that is to convert the rdb file to primitive commands, which is what many tools do.
Comment From: chenyang8094
Wow, great to see this feature in RESP3.
It is indeed feasible to let these replay tools convert RDB into original commands, but for some complex data structures such as redis stream or other redis modules, these tools are powerless or very difficult. These problems are all I encountered in maintaining RedisShake.
So I'm just saying what I know, lest you miss something.
Comment From: oranagra
i maintained a similar tool in Redis Labs, i'm well aware of the limitations of redis and the complications of converting RDB file to RESTORE commands.
I don't think this justifies rewriteAppendOnlyFileRio in redis, and i do have a plan to solve these in different ways (RESP3 chunks, as well as librdb.so), but that's still far ahead.
Comment From: ShooterIT
disallow BGREWRITEAOF command when appendonly is off?
I wish we don't break this change, it is charismatic for redis to provide many methods to use freely.
IIRC, some users actually use this feature 😂, AOF is easy to inject to another running redis server, such as by nc , but RDB is needed to parse, enabling AOF may cause performance loss, so they execute BGREWRITEAOF when needed.
I also agree AOF is a convenient method to backup, actually we ever used this way instead of RDB since RDB didn't support incrementally fsync before, calling fsync at the last causes much heavy disk load which influences redis main thread and other writing disk processes.
Comment From: oranagra
ok... i'm ok with dropping this change from 7.0. But as i noted in my previous messages, i don't see a bright future for this feature. I do think that other recent changes we made (preamble, and multi-part) are already breaking it in some way. and i do think that this mechanism will be completely removed from redis in some future version, while other mechanisms will be added to cover these use cases in a better way. @redis/core-team it was something we agreed on a call, so FYI on dropping it.