Hi Melanie, I remember I ever reviewed this patch. But when I revisit v7, I just got a confusion to clarify.
> On Nov 19, 2025, at 03:13, Melanie Plageman <[email protected]> wrote: > > On Mon, Nov 3, 2025 at 3:06 PM Melanie Plageman > <[email protected]> wrote: >> >> I found an incorrect assert in CleanVictimBuffer() that was tripping >> in CI. Attached v6 is updated with it removed. > > v7 rebased over recent changes in bufmgr.c > > - Melanie > <v7-0001-Refactor-goto-into-for-loop-in-GetVictimBuffer.patch><v7-0002-Split-FlushBuffer-into-two-parts.patch><v7-0003-Eagerly-flush-bulkwrite-strategy-ring.patch> 0001 only changes “goto" to “for”, thus it's supposed no logic change. In the old code: ``` if (strategy != NULL) { XLogRecPtr lsn; /* Read the LSN while holding buffer header lock */ buf_state = LockBufHdr(buf_hdr); lsn = BufferGetLSN(buf_hdr); UnlockBufHdr(buf_hdr); if (XLogNeedsFlush(lsn) && StrategyRejectBuffer(strategy, buf_hdr, from_ring)) { LWLockRelease(content_lock); UnpinBuffer(buf_hdr); goto again; } } ``` It only retries when XLogNeedsFlush(lsn) is true. In the patch, everything are merged into StrategyRejectBuffer(): ``` if (StrategyRejectBuffer(strategy, buf_hdr, from_ring)) { LWLockRelease(content_lock); UnpinBuffer(buf_hdr); continue; } ``` When StrategyRejectBuffer(strategy, buf_hdr, from_ring) is true, retry happens. However, look into the function: ``` bool StrategyRejectBuffer(BufferAccessStrategy strategy, BufferDesc *buf, bool from_ring) { XLogRecPtr lsn; if (!strategy) return false; /* We only do this in bulkread mode */ if (strategy->btype != BAS_BULKREAD) return false; /* Don't muck with behavior of normal buffer-replacement strategy */ if (!from_ring || strategy->buffers[strategy->current] != BufferDescriptorGetBuffer(buf)) return false; LockBufHdr(buf); lsn = BufferGetLSN(buf); UnlockBufHdr(buf); if (XLogNeedsFlush(lsn)) return false; ``` When XLogNeedsFlush(lsn) is true, StrategyRejectBuffer returns false, thus no retry will happen, which is different from the old logic, is that an intentional change? Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/
