From 6d03409e0124ad7cdd119f4be2edbc1e61461c1e Mon Sep 17 00:00:00 2001 From: Binbin Date: Wed, 11 Sep 2024 12:00:08 +0800 Subject: [PATCH] Fix module RdbLoad wrongly disable the AOF (#1001) In RdbLoad, we disable AOF before emptyData and rdbLoad to prevent copy-on-write issues. After rdbLoad completes, AOF should be re-enabled, but the code incorrectly checks server.aof_state, which has been reset to AOF_OFF in stopAppendOnly. This leads to AOF not being re-enabled after being disabled. --------- Signed-off-by: Binbin --- src/module.c | 8 +++++++- tests/unit/moduleapi/rdbloadsave.tcl | 3 +++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/module.c b/src/module.c index a7c669e27..160e1b094 100644 --- a/src/module.c +++ b/src/module.c @@ -12928,6 +12928,9 @@ int VM_RdbLoad(ValkeyModuleCtx *ctx, ValkeyModuleRdbStream *stream, int flags) { disconnectSlaves(); freeReplicationBacklog(); + /* Stop and kill existing AOF rewriting fork as it is saving outdated data, + * we will re-enable it after the rdbLoad. Also killing it will prevent COW + * memory issue. */ if (server.aof_state != AOF_OFF) stopAppendOnly(); /* Kill existing RDB fork as it is saving outdated data. Also killing it @@ -12946,7 +12949,10 @@ int VM_RdbLoad(ValkeyModuleCtx *ctx, ValkeyModuleRdbStream *stream, int flags) { int ret = rdbLoad(stream->data.filename,NULL,RDBFLAGS_NONE); if (server.current_client) unprotectClient(server.current_client); - if (server.aof_state != AOF_OFF) startAppendOnly(); + + /* Here we need to decide whether to enable the AOF based on the aof_enabled, + * since the previous stopAppendOnly sets aof_state to AOF_OFF. */ + if (server.aof_enabled) startAppendOnly(); if (ret != RDB_OK) { errno = (ret == RDB_NOT_EXIST) ? ENOENT : EIO; diff --git a/tests/unit/moduleapi/rdbloadsave.tcl b/tests/unit/moduleapi/rdbloadsave.tcl index 9319c9385..131c3f321 100644 --- a/tests/unit/moduleapi/rdbloadsave.tcl +++ b/tests/unit/moduleapi/rdbloadsave.tcl @@ -80,6 +80,9 @@ start_server {tags {"modules"}} { fail "Can't find 'Killing AOF child' in recent log lines" } + # Verify that AOF is active. + assert_equal 1 [s aof_enabled] + # Verify the value in the loaded rdb assert_equal v1 [r get k]