Message-ID: <2ef04861c0f77e7ae78b703770cc2bbbac3d85e6.camel@j-davis.com>
Subject: Re: Improve WALRead() to suck data directly from WAL buffers when
 possible
From: Jeff Davis <pgsql@j-davis.com>
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.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: Fri, 03 Nov 2023 00:05:06 -0700
In-Reply-To: <CALj2ACV123Ke6wn05QCnCgJhFpcarGCv7sKM1p39t+qpUNHUgQ@mail.gmail.com>
References: 
	<CALj2ACXKKK=wbiG5_t6dGao5GoecMwRkhr7GjVBM_jg54+Na=Q@mail.gmail.com>
	 <20221212.115717.1183698540772933396.horikyota.ntt@gmail.com>
	 <CALj2ACXUbvON86vgwTkum8ab3bf1=HkMxQ5hZJZS3ZcJn8NEXQ@mail.gmail.com>
	 <CAFiTN-uCG-+axA4r02_9OYgKJU_GkJ1=tBCxq_5UvyWmWEBywA@mail.gmail.com>
	 <CALj2ACU9cfAcfVsGwUqXMace_7rfSBJ7+hXVJfVV1jnspTDGHQ@mail.gmail.com>
	 <628b07b79743e2614ab1724f516cbd41560d6551.camel@j-davis.com>
	 <20230114203403.4zpg72fw2qb34awf@awork3.anarazel.de>
	 <0d84c025e38673cb575dcf4987d050fe7b1e1a1f.camel@j-davis.com>
	 <20231011224353.cl7c2s222dw3de4j@awork3.anarazel.de>
	 <CALj2ACU2DGH-BB_187N7gfVkLdxfJ+doVTcteS0ugLn_c8mLOw@mail.gmail.com>
	 <CALj2ACVgVgA5BSSrEYO2eTMEGB=QUbcYosYm3vZ3R2=GPB6tNw@mail.gmail.com>
	 <5c6e8e60c5c156a9f51ccf850f70829e45a985a1.camel@j-davis.com>
	 <CALj2ACVjzHgQvONnTLffyBJRACPwhsf-cYG4TQ2KHKrFfFop-w@mail.gmail.com>
	 <4132fe48f831ed6f73a9eb191af5fe475384969c.camel@j-davis.com>
	 <CALj2ACV123Ke6wn05QCnCgJhFpcarGCv7sKM1p39t+qpUNHUgQ@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4-0ubuntu2 
MIME-Version: 1.0
Archived-At: <https://www.postgresql.org/message-id/2ef04861c0f77e7ae78b703770cc2bbbac3d85e6.camel%40j-davis.com>
Precedence: bulk

On Thu, 2023-11-02 at 22:38 +0530, Bharath Rupireddy wrote:
> > I suppose the question is: should reading from the WAL buffers an
> > intentional thing that the caller does explicitly by specific
> > callers?
> > Or is it an optimization that should be hidden from the caller?
> >=20
> > I tend toward the former, at least for now.
>=20
> Yes, it's an optimization that must be hidden from the caller.

As I said, I tend toward the opposite: that specific callers should
read from the buffers explicitly in the cases where it makes sense.

I don't think this is the most important point right now though, let's
sort out the other details.

> >=20
> At any given point of time, WAL buffer pages are maintained as a
> circularly sorted array in an ascending order from
> OldestInitializedPage to InitializedUpTo (new pages are inserted at
> this end).

I don't see any reference to OldestInitializedPage or anything like it,
with or without your patch. Am I missing something?

> - Read 6 pages starting from LSN 80. Nothing is read from WAL buffers
> as the page at LSN 80 doesn't exist despite other 5 pages starting
> from LSN 90 exist.

This is what I imagined a "partial hit" was: read the 5 pages starting
at 90. The caller would then need to figure out how to read the page at
LSN 80 from the segment files.

I am not saying we should support this case; perhaps it doesn't matter.
I'm just describing why that term was confusing for me.

> If a caller asks for an unflushed WAL read
> intentionally or unintentionally, XLogReadFromBuffers() reads only 4
> pages starting from LSN 150 to LSN 180 and will leave the remaining 2
> pages for the caller to deal with. This is the partial hit that can
> happen.

To me that's more like an EOF case. "Partial hit" sounds to me like the
data exists but is not available in the cache (i.e. go to the segment
files); whereas if it encountered the end, the data is not available at
all.

> >=20
> WALBufMappingLock protects both xlblocks and WAL buffer pages [1][2].
> I'm not sure how using the memory barrier, not WALBufMappingLock,
> prevents writers from replacing WAL buffer pages while readers
> reading
> the pages.

It doesn't *prevent* that case, but it does *detect* that case. We
don't want to prevent writers from replacing WAL buffers, because that
would mean we are slowing down the critical WAL writing path.

Let me explain the potential problem cases, and how the barriers
prevent them:

Potential problem 1: the page is not yet resident in the cache at the
time the memcpy begins. In this case, the first read barrier would
ensure that the page is also not yet resident at the time xlblocks[idx]
is read into endptr1, and=C2=A0we'd break out of the loop.

Potential problem 2: the page is evicted before the memcpy finishes. In
this case, the second read barrier would ensure that the page was also
evicted before xlblocks[idx] is read into endptr2, and again we'd
detect the problem and break out of the loop.

I assume here that, if xlblocks[idx] holds the endPtr of the desired
page, all of the bytes for that page are resident at that moment. I
don't think that's true right now: AdvanceXLInsertBuffers() zeroes the
old page before updating=C2=A0xlblocks[nextidx]. I think it needs something
like:

  pg_atomic_write_u64(&XLogCtl->xlblocks[nextidx], InvalidXLogRecPtr);
  pg_write_barrier();

before the MemSet.

I didn't review your latest v14 patch yet.

Regards,
	Jeff Davis