Incorrect logic in XLogNeedsFlush() - Mailing list pgsql-hackers

Hi,

If you call XLogNeedsFlush() immediately after calling XLogFlush() in
FlushBuffer(), it can return true.

With this diff:

diff --git a/src/backend/storage/buffer/bufmgr.c
b/src/backend/storage/buffer/bufmgr.c
index 350cc0402aa..91c3fe99d6e 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -4342,7 +4342,11 @@ FlushBuffer(BufferDesc *buf, SMgrRelation reln,
IOObject io_object,
     * skip the flush if the buffer isn't permanent.
     */
    if (buf_state & BM_PERMANENT)
+   {
        XLogFlush(recptr);
+       if(XLogNeedsFlush(recptr))
+           elog(ERROR, "should be flushed and isn't");
+   }

recovery/015_promotion_pages errors out.

During an end-of-recovery checkpoint, XLogNeedsFlush() should compare
the provided LSN to the flush pointer and not the min recovery point.

This only errors out during an end-of-recovery checkpoint after a
crash when the ControlFile->minRecoveryPoint has not been correctly
updated. In this case, LocalMinRecoveryPoint is initialized to an
incorrect value potentially causing XLogNeedsFlush() to incorrectly
return true.

This trivial diff makes the test pass:

@@ -3115,7 +3125,7 @@ XLogNeedsFlush(XLogRecPtr record)
     * instead. So "needs flush" is taken to mean whether minRecoveryPoint
     * would need to be updated.
     */
-   if (RecoveryInProgress())
+   if (RecoveryInProgress() && !XLogInsertAllowed())
    {

However, since all of this code is new to me, there are some remaining
things I don't understand:

Why is it okay for other processes than the startup process to
initialize LocalMinRecoveryPoint from ControlFile->minRecoveryPoint
during crash recovery?

All of the comments and the code seem to indicate that this is allowed.

Couldn't it just as likely be invalid for them (e.g. checkpointer
here) as it could for the startup process? Like if we crashed before
updating it?

Besides this question, I find the use of the global variable
InRecovery as a proxy for RecoveryInProgress() + "I'm the startup
process" to be confusing. It seems like UpdateMinRecoveryPoint() and
XLogNeedsFlush() could just test MyBackendType == B_STARTUP and make
it explicit.

- Melanie