Hi, On 2023-11-08 13:10:34 +0530, Bharath Rupireddy wrote: > > > + /* > > > + * The fact that we acquire WALBufMappingLock while reading > > > the WAL > > > + * buffer page itself guarantees that no one else > > > initializes it or > > > + * makes it ready for next use in AdvanceXLInsertBuffer(). > > > However, we > > > + * need to ensure that we are not reading a page that just > > > got > > > + * initialized. For this, we look at the needed page header. > > > + */ > > > + phdr = (XLogPageHeader) page; > > > + > > > + /* Return, if WAL buffer page doesn't look valid. */ > > > + if (!(phdr->xlp_magic == XLOG_PAGE_MAGIC && > > > + phdr->xlp_pageaddr == (ptr - (ptr % XLOG_BLCKSZ)) > > > && > > > + phdr->xlp_tli == tli)) > > > + break; > > > > I don't think this code should ever encounter a page where this is not the > > case? We particularly shouldn't do so silently, seems that could hide all > > kinds of problems. > > I think it's possible to read a "just got initialized" page with the > new approach to read WAL buffer pages without WALBufMappingLock if the > page is read right after it is initialized and xlblocks is filled in > AdvanceXLInsertBuffer() but before actual WAL is written.
I think the code needs to make sure that *never* happens. That seems unrelated to holding or not holding WALBufMappingLock. Even if the page header is already valid, I don't think it's ok to just read/parse WAL data that's concurrently being modified. We can never allow WAL being read that's past XLogBytePosToRecPtr(XLogCtl->Insert->CurrBytePos) as it does not exist. And if the to-be-read LSN is between XLogCtl->LogwrtResult->Write and XLogBytePosToRecPtr(Insert->CurrBytePos) we need to call WaitXLogInsertionsToFinish() before copying the data. Greetings, Andres Freund
