Message-ID: <ca8fbf983069c701ccdc934ccc5b0029130cd337.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: Thu, 25 Jan 2024 19:01:52 -0800
In-Reply-To: 
 <CALj2ACV=C1GZT9XQRm4iN1NV1T=hLA_hsGWNx2Y5-G+mSwdhNg@mail.gmail.com>
References: 
	<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>
	 <3750ff9c2a5556808827e7fc5bcc0bd16e9facf4.camel@j-davis.com>
	 <CALj2ACXy5eA-Y4Qgm3MUYZTGr_opNm2SzGtN+Zj6dbcSzqHrJA@mail.gmail.com>
	 <73df9831747b01ced949102610dc44ca6026ee1c.camel@j-davis.com>
	 <CALj2ACWPKuoUdA7r4VFKhsTZeNekyUr=ZwM6ckyvJ0qv2f6Whg@mail.gmail.com>
	 <20240122201218.bclrdkwpmuk26fl3@awork3.anarazel.de>
	 <2b36bf99e762e65db0dafbf8d338756cf5fa6ece.camel@j-davis.com>
	 <CALj2ACV=C1GZT9XQRm4iN1NV1T=hLA_hsGWNx2Y5-G+mSwdhNg@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/ca8fbf983069c701ccdc934ccc5b0029130cd337.camel%40j-davis.com>
Precedence: bulk

On Thu, 2024-01-25 at 14:35 +0530, Bharath Rupireddy wrote:
>=20
> "expecting zeros at the end" - this can't always be true as the WAL
>=20
...

> I think this needs to be discussed separately. If okay, I'll start a
> new thread.

Thank you for investigating. When the above issue is handled, I'll be
more comfortable expanding the call sites for XLogReadFromBuffers().

> > Also, if we've detected that the first requested buffer has been
> > evicted, is there any value in continuing the loop to see if more
> > recent buffers are available? For example, if the requested LSNs
> > range
> > over buffers 4, 5, and 6, and 4 has already been evicted, should we
> > try
> > to return LSN data from 5 and 6 at the proper offset in the dest
> > buffer? If so, we'd need to adjust the API so the caller knows what
> > parts of the dest buffer were filled in.
>=20
> I'd second this capability for now to keep the API simple and clear,
> but we can consider expanding it as needed.

Agreed. This case doesn't seem important; I just thought I'd ask about
it.

> If the callers use GetFlushRecPtr() to determine how far to read,
> LogwrtResult will be *reasonably* latest

It will be up-to-date enough that we'd never go through
WaitXLogInsertionsToFinish(), which is all we care about.

> As far as the current WAL readers are concerned, we don't need an
> explicit spinlock to determine LogwrtResult because all of them use
> GetFlushRecPtr() to determine how far to read. If there's any caller
> that's not updating LogwrtResult at all, we can consider reading
> LogwrtResult it ourselves in future.

So we don't actually need that path yet, right?

> 5. I think the two requirements specified at
> https://www.postgresql.org/message-id/20231109205836.zjoawdrn4q77yemv%40a=
work3.anarazel.de
> still hold with the v19j.

Agreed.

> PSA v20 patch set.

0001 is very close. I have the following suggestions:

  * Don't just return zero. If the caller is doing something we don't
expect, we want to fix the caller. I understand you'd like this to be
more like a transparent optimization, and we may do that later, but I
don't think it's a good idea to do that now.

  * There's currently no use for reading LSNs between Write and Insert,
so remove the WaitXLogInsertionsToFinish() code path. That also means
we don't need the extra emitLog parameter, so we can remove that. When
we have a use case, we can bring it all back.

If you agree, I can just make those adjustments (and do some final
checking) and commit 0001. Otherwise let me know what you think.

0002: How does the test control whether the data requested is before
the Flush pointer, the Write pointer, or the Insert pointer? What if
the walwriter comes in and moves one of those pointers before the next
statement is executed? Also, do you think a test module is required for
the basic functionality in 0001, or only when we start doing more
complex things like reading past the Flush pointer?

0003: can you explain why this is useful for wal summarizer to read
from the buffers?

Regards,
	Jeff Davis