Horton

Re: Improve WALRead() to suck data directly from WAL buffers when possible

Andres Freund <andres@anarazel.de>

View thread
From: Andres Freund <andres@anarazel.de>
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Cc: Jeff Davis <pgsql@j-davis.com>, Dilip Kumar <dilipbalaut@gmail.com>, Kyotaro Horiguchi <horikyota.ntt@gmail.com>, pgsql-hackers@lists.postgresql.org, Nathan Bossart <nathandbossart@gmail.com>, Masahiko Sawada <sawada.mshk@gmail.com>
Date: 2023-11-09T20:58:36Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Add XLogCtl->logInsertResult

  2. Add assert to WALReadFromBuffers().

  3. Read WAL directly from WAL buffers.

  4. Additional write barrier in AdvanceXLInsertBuffer().

  5. Use 64-bit atomics for xlblocks array elements.

  6. Don't trust unvalidated xl_tot_len.

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