Horton

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

Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>

View thread
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
To: Jeff Davis <pgsql@j-davis.com>
Cc: Andres Freund <andres@anarazel.de>, 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-04T15:25:00Z
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.

Attachments

On Sat, Nov 4, 2023 at 1:17 AM Jeff Davis <pgsql@j-davis.com> wrote:
>
> > > I think it needs something like:
> > >
> > >   pg_atomic_write_u64(&XLogCtl->xlblocks[nextidx],
> > > InvalidXLogRecPtr);
> > >   pg_write_barrier();
> > >
> > > before the MemSet.
> >
> > I think it works. First, xlblocks needs to be turned to an array of
> > 64-bit atomics and then the above change.
>
> Does anyone see a reason we shouldn't move to atomics here?
>
> >
> >         pg_write_barrier();
> >
> >         *((volatile XLogRecPtr *) &XLogCtl->xlblocks[nextidx]) =
> > NewPageEndPtr;
>
> I am confused why the "volatile" is required on that line (not from
> your patch). I sent a separate message about that:
>
> https://www.postgresql.org/message-id/784f72ac09061fe5eaa5335cc347340c367c73ac.camel@j-davis.com
>
> > I think the 3 things that helps read from WAL buffers without
> > WALBufMappingLock are: 1) couple of the read barriers in
> > XLogReadFromBuffers, 2) atomically initializing xlblocks[idx] to
> > InvalidXLogRecPtr plus a write barrier in AdvanceXLInsertBuffer(), 3)
> > the following sanity check to see if the read page is valid in
> > XLogReadFromBuffers(). If it sounds sensible, I'll work towards
> > coding
> > it up. Thoughts?
>
> I like it. I think it will ultimately be a fairly simple loop. And by
> moving to atomics, we won't need the delicate comment in
> GetXLogBuffer().

I'm attaching the v15 patch set implementing the above ideas. Please
have a look.

-- 
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com