From e32518d6553e4bd9d7742926b65fdc3fb18a0da6 Mon Sep 17 00:00:00 2001 From: Binbin Date: Sat, 27 Jul 2024 18:38:23 +0800 Subject: [PATCH] Fix unexpected behavior when turning appendonly on and off within a transaction (#826) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If we do `config set appendonly yes` and `config set appendonly no` in a multi, there are some unexpected behavior. When doing appendonly yes, we will schedule a AOFRW, and when we are doding appendonly no, we will call stopAppendOnly to stop it. In stopAppendOnly, the aof_fd is -1 since the aof is not start yet and the fsync and close will take the -1 and call it, so they will all fail with EBADF. And stopAppendOnly will emit a server log, the close(-1) should be no problem but it is still an undefined behavior. This PR also adds a log `Background append only file rewriting scheduled.` to bgrewriteaofCommand when it was scheduled. And adds a log in stopAppendOnly when a scheduled AOF is canceled, it will print `AOF was disabled but there is a scheduled AOF background, cancel it.` Signed-off-by: Binbin Co-authored-by: Viktor Söderqvist --- src/aof.c | 20 +++++++++++++------- tests/integration/aof.tcl | 17 +++++++++++++++++ 2 files changed, 30 insertions(+), 7 deletions(-) diff --git a/src/aof.c b/src/aof.c index 1a47d9c68..d43616c88 100644 --- a/src/aof.c +++ b/src/aof.c @@ -931,18 +931,23 @@ void killAppendOnlyChild(void) { * at runtime using the CONFIG command. */ void stopAppendOnly(void) { serverAssert(server.aof_state != AOF_OFF); - flushAppendOnlyFile(1); - if (valkey_fsync(server.aof_fd) == -1) { - serverLog(LL_WARNING, "Fail to fsync the AOF file: %s", strerror(errno)); - } else { - server.aof_last_fsync = server.mstime; + if (server.aof_fd != -1) { + flushAppendOnlyFile(1); + if (valkey_fsync(server.aof_fd) == -1) { + serverLog(LL_WARNING, "Fail to fsync the AOF file: %s", strerror(errno)); + } else { + server.aof_last_fsync = server.mstime; + } + close(server.aof_fd); } - close(server.aof_fd); server.aof_fd = -1; server.aof_selected_db = -1; server.aof_state = AOF_OFF; - server.aof_rewrite_scheduled = 0; + if (server.aof_rewrite_scheduled) { + server.aof_rewrite_scheduled = 0; + serverLog(LL_NOTICE, "AOF was disabled but there is a scheduled AOF background, cancel it."); + } server.aof_last_incr_size = 0; server.aof_last_incr_fsync_offset = 0; server.fsynced_reploff = -1; @@ -2453,6 +2458,7 @@ void bgrewriteaofCommand(client *c) { /* When manually triggering AOFRW we reset the count * so that it can be executed immediately. */ server.stat_aofrw_consecutive_failures = 0; + serverLog(LL_NOTICE, "Background append only file rewriting scheduled."); addReplyStatus(c, "Background append only file rewriting scheduled"); } else if (rewriteAppendOnlyFileBackground() == C_OK) { addReplyStatus(c, "Background append only file rewriting started"); diff --git a/tests/integration/aof.tcl b/tests/integration/aof.tcl index 6a074fbef..72fae9915 100644 --- a/tests/integration/aof.tcl +++ b/tests/integration/aof.tcl @@ -655,4 +655,21 @@ tags {"aof external:skip"} { } } } + + start_server {} { + # This test is just a coverage test, it does not check anything. + test {Turning appendonly on and off within a transaction} { + r config set appendonly no + r multi + r config set appendonly yes + r config set appendonly no + r exec + + r config set appendonly yes + r multi + r config set appendonly no + r config set appendonly yes + r exec + } + } }