On Thu, Dec 4, 2025 at 6:41 PM Yilin Zhang <[email protected]> wrote: > > On 28/11/2025 02:15, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote: > > I've made a few minor adjustments to the test patch. > > The updated version is attached. > > Hi, > I was reading your code and had a question about the new code you added in > the main() function of pg_recvlogical.c: > if (outfd != -1 && strcmp(outfile, "-") != 0) > OutputFsync(feGetCurrentTimestamp()); > In the stream loop, the StreamLogicalLog() function already contains similar > code: > if (outfd != -1 && > feTimestampDifferenceExceeds(output_last_fsync, now, > fsync_interval)) > { > if (!OutputFsync(now)) > goto error; > } > > If the outfile becomes unwritable due to external reasons, would the error > reporting here be redundant with the error handling in StreamLogicalLog()?
Are you suggesting that the existing code checks the return value of OutputFsync(), but since it never returns false, that check is unnecessary and can be removed? If so, I agree. The attached 0004 patch does that. Regards, -- Fujii Masao
From a6ed81df5fc042b1b3684c24a4b4ae3acfb10576 Mon Sep 17 00:00:00 2001 From: Fujii Masao <[email protected]> Date: Thu, 25 Dec 2025 19:41:03 +0900 Subject: [PATCH v3 4/4] pg_recvlogical: remove unnecessary OutputFsync() return value checks. Commit 1e2fddfa33d changed OutputFsync() so that it always returns true. However, pg_recvlogical.c still contained checks of its boolean return value, which are now redundant. This commit removes those checks and changes the type of return value of OutputFsync() to void, simplifying the code. --- src/bin/pg_basebackup/pg_recvlogical.c | 23 +++++++---------------- 1 file changed, 7 insertions(+), 16 deletions(-) diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c index 0354173bcbc..60e77406b77 100644 --- a/src/bin/pg_basebackup/pg_recvlogical.c +++ b/src/bin/pg_basebackup/pg_recvlogical.c @@ -184,7 +184,7 @@ disconnect_atexit(void) PQfinish(conn); } -static bool +static void OutputFsync(TimestampTz now) { output_last_fsync = now; @@ -199,21 +199,19 @@ OutputFsync(TimestampTz now) startpos = output_fsync_lsn; if (fsync_interval <= 0) - return true; + return; if (!output_needs_fsync) - return true; + return; output_needs_fsync = false; /* can only fsync if it's a regular file */ if (!output_isfile) - return true; + return; if (fsync(outfd) != 0) pg_fatal("could not fsync file \"%s\": %m", outfile); - - return true; } /* @@ -314,10 +312,7 @@ StreamLogicalLog(void) if (outfd != -1 && feTimestampDifferenceExceeds(output_last_fsync, now, fsync_interval)) - { - if (!OutputFsync(now)) - goto error; - } + OutputFsync(now); if (standby_message_timeout > 0 && feTimestampDifferenceExceeds(last_status, now, @@ -334,8 +329,7 @@ StreamLogicalLog(void) if (outfd != -1 && output_reopen && strcmp(outfile, "-") != 0) { now = feGetCurrentTimestamp(); - if (!OutputFsync(now)) - goto error; + OutputFsync(now); close(outfd); outfd = -1; } @@ -654,9 +648,7 @@ StreamLogicalLog(void) { TimestampTz t = feGetCurrentTimestamp(); - /* no need to jump to error on failure here, we're finishing anyway */ OutputFsync(t); - if (close(outfd) != 0) pg_log_error("could not close file \"%s\": %m", outfile); } @@ -1065,8 +1057,7 @@ static bool flushAndSendFeedback(PGconn *conn, TimestampTz *now) { /* flush data to disk, so that we send a recent flush pointer */ - if (!OutputFsync(*now)) - return false; + OutputFsync(*now); *now = feGetCurrentTimestamp(); if (!sendFeedback(conn, *now, true, false)) return false; -- 2.51.2
