Unverified Commit 625bdaf3 authored by chenyang8094's avatar chenyang8094 Committed by GitHub
Browse files

Fix auto-aof-rewrite-percentage based AOFRW trigger after restart (#10550)



The `auto-aof-rewrite-percentage` config defines at what growth percentage
an automatic AOF rewrite is triggered.
This normally works OK since the size of the AOF file at the end of a rewrite
is stored in `server.aof_rewrite_base_size`.
However, on startup, redis used to store the entire size of the AOF file into that
variable, resulting in a wrong automatic AOF rewrite trigger (could have been
triggered much later than desired).
This issue would only affect the first AOFRW after startup, after that future AOFRW
would have been triggered correctly.
This bug existed in all previous versions of Redis.

This PR unifies the meaning of `server.aof_rewrite_base_size`, which only represents
the size of BASE AOF.
Note that after an AOFRW this size includes the size of the incremental file (all the
commands that executed during rewrite), so that auto-aof-rewrite-percentage is the
ratio from the size of the AOF after rewrite.
However, on startup, it is complicated to know that size, and we compromised on
taking just the size of the base file, this means that the first rewrite after startup can
happen a little bit too soon.
Co-authored-by: default avatarOran Agra <oran@redislabs.com>
Co-authored-by: default avataryoav-steinberg <yoav@redislabs.com>
parent 451531f1
......@@ -1558,7 +1558,7 @@ int loadAppendOnlyFiles(aofManifest *am) {
serverAssert(am != NULL);
int status, ret = C_OK;
long long start;
off_t total_size = 0;
off_t total_size = 0, base_size = 0;
sds aof_name;
int total_num, aof_num = 0, last_file;
......@@ -1607,6 +1607,7 @@ int loadAppendOnlyFiles(aofManifest *am) {
serverAssert(am->base_aof_info->file_type == AOF_FILE_TYPE_BASE);
aof_name = (char*)am->base_aof_info->file_name;
updateLoadingFileName(aof_name);
base_size = getAppendOnlyFileSize(aof_name, NULL);
last_file = ++aof_num == total_num;
start = ustime();
ret = loadSingleAppendOnlyFile(aof_name);
......@@ -1659,7 +1660,16 @@ int loadAppendOnlyFiles(aofManifest *am) {
}
server.aof_current_size = total_size;
server.aof_rewrite_base_size = server.aof_current_size;
/* Ideally, the aof_rewrite_base_size variable should hold the size of the
* AOF when the last rewrite ended, this should include the size of the
* incremental file that was created during the rewrite since otherwise we
* risk the next automatic rewrite to happen too soon (or immediately if
* auto-aof-rewrite-percentage is low). However, since we do not persist
* aof_rewrite_base_size information anywhere, we initialize it on restart
* to the size of BASE AOF file. This might cause the first AOFRW to be
* executed early, but that shouldn't be a problem since everything will be
* fine after the first AOFRW. */
server.aof_rewrite_base_size = base_size;
server.aof_fsync_offset = server.aof_current_size;
cleanup:
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment