Message-ID: <3750ff9c2a5556808827e7fc5bcc0bd16e9facf4.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>, Andres
 Freund <andres@anarazel.de>
Cc: 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: Thu, 07 Dec 2023 16:34:57 -0800
In-Reply-To: <CALj2ACVfFMfqD5oLzZSQQZWfXiJqd-NdX0_317veP6FuB31QWA@mail.gmail.com>
References: <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>
	 <20231104004348.zhxzk2p266i3r2z2@awork3.anarazel.de>
	 <CALj2ACUSydTHPcucftm4XdiJ4osH8zeNENyADd-zfDzEMf6akA@mail.gmail.com>
	 <20231109205836.zjoawdrn4q77yemv@awork3.anarazel.de>
	 <CALj2ACXo-_boM3h-PJuvfuvFzP1nv2wcZBoZN_YbRbQBGQYAUg@mail.gmail.com>
	 <CALj2ACVfFMfqD5oLzZSQQZWfXiJqd-NdX0_317veP6FuB31QWA@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/3750ff9c2a5556808827e7fc5bcc0bd16e9facf4.camel%40j-davis.com>
Precedence: bulk

On Thu, 2023-12-07 at 15:59 +0530, Bharath Rupireddy wrote:
> In the attached v17 patch

0001 could impact performance could be impacted in a few ways:

   * There's one additional write barrier inside
     AdvanceXLInsertBuffer()
   * AdvanceXLInsertBuffer() already holds WALBufMappingLock, so
     the atomic access inside of it is somewhat redundant
   * On some platforms, the XLogCtlData structure size will change

The patch has been out for a while and nobody seems concerned about
those things, and they look fine to me, so I assume these are not real
problems. I just wanted to highlight them.

Also, the description and the comments seem off. The patch does two
things: (a) make it possible to read a page without a lock, which means
we need to mark with InvalidXLogRecPtr while it's being initialized;
and (b) use 64-bit atomics to make it safer (or at least more
readable).

(a) feels like the most important thing, and it's a hard requirement
for the rest of the work, right?

(b) seems like an implementation choice, and I agree with it on
readability grounds.

Also:

+  * But it means that when we do this
+  * unlocked read, we might see a value that appears to be ahead of
the
+  * page we're looking for. Don't PANIC on that, until we've verified
the
+  * value while holding the lock.

Is that still true even without a torn read?

The code for 0001 itself looks good. These are minor concerns and I am
inclined to commit something like it fairly soon.

Regards,
	Jeff Davis