From ab7c8979ff3e7efef64ffea0b61577e3290bd3e9 Mon Sep 17 00:00:00 2001 From: Dilip Kumar Date: Fri, 12 Sep 2025 08:37:15 +0530 Subject: [PATCH v4] Make XLogFlush() and XLogNeedsFlush() decision more consistent XLogFlush() and XLogNeedsFlush() previously used inconsistent criteria for determining the WAL flush status. XLogFlush() used XLogInsertAllowed() to determine whether to perform WAL flush or just update minRecoveryPoint. In contrast, XLogNeedsFlush() relied on checking RecoveryInProgress() to identify whether to check against the minRecoveryPoint or the current flush location. This difference introduced a logical inconsistency. For example, during an end-of-recovery checkpoint, the checkpointer was allowed to flush the WAL. However, XLogNeedsFlush() determined its need for a flush based on minRecoveryPoint because RecoveryInProgress() was true, leading to a reliance on the minRecoveryPoint when it was not applicable. This commit resolves the inconsistency by having XLogNeedsFlush() also base its decision on the result of XLogInsertAllowed(). To further ensure consistency, XLogFlush() adds an assertion that XLogNeedsFlush() returns false after XLogFlush() has completed its job. The inconsistency described above was not observed as a live bug, but this patch hardens the logic to prevent potential issues. --- src/backend/access/transam/xlog.c | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 0baf0ac6160..34d9d762210 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -2938,6 +2938,13 @@ XLogFlush(XLogRecPtr record) "xlog flush request %X/%08X is not satisfied --- flushed only to %X/%08X", LSN_FORMAT_ARGS(record), LSN_FORMAT_ARGS(LogwrtResult.Flush)); + + /* + * Cross-check XLogNeedsFlush(). Some of the checks of XLogFlush() and + * XLogNeedsFlush() are duplicated, and this assertion ensures that these + * remain consistent. + */ + Assert(!XLogNeedsFlush(record)); } /* @@ -3103,9 +3110,13 @@ XLogBackgroundFlush(void) /* * Test whether XLOG data has been flushed up to (at least) the given position. + * or whether the minimum recovery point is updated past the given position. + * + * Returns true if a flush is still needed or if the minimum recovery point + * must be updated. * - * Returns true if a flush is still needed. (It may be that someone else - * is already in process of flushing that far, however.) + * It is possible that someone else is already in the process of flushing that + * far or updating the minimum recovery point that far. */ bool XLogNeedsFlush(XLogRecPtr record) @@ -3114,8 +3125,16 @@ XLogNeedsFlush(XLogRecPtr record) * During recovery, we don't flush WAL but update minRecoveryPoint * instead. So "needs flush" is taken to mean whether minRecoveryPoint * would need to be updated. + * + * The XLogFlush() will perform a physical WAL flush if XLogInsertAllowed() + * is true (e.g., during the end-of-recovery checkpoint) otherwise it will + * update the minRecoveryPoint. + * + * For consistency with XLogFlush(), we adopt its logic by using + * !XLogInsertAllowed() to determine if the check should be against + * minRecoveryPoint or LogwrtResult.Flush. */ - if (RecoveryInProgress()) + if (!XLogInsertAllowed()) { /* * An invalid minRecoveryPoint means that we need to recover all the -- 2.49.0