MIME-Version: 1.0
References: 
 <CAH2-Wzmy7NMba9k8m_VZ-XNDZJEUQBU8TeLEeL960-rAKb-+tQ@mail.gmail.com>
 <d2d4qofb5ajg2ftvm6h56oi4utdwpzkqfjd7z2y4vod5qaub4h@ixyotvfut3mg>
 <CAH2-WznoD7vhjZNDj-5OrLp+1fjvW6ypEUwZ1=ieadefgWaTDQ@mail.gmail.com>
 <ayjpwpm5cn6ng2bgedhz3ckbjrxocbsbywhlghwxxz2p6a5tgr@jubomhsjkvcl>
 <CAH2-Wznxu+AFz-EBOG-XiRA_R3nXLp45NEiGSD3ebx3h=OKPAw@mail.gmail.com>
 <vbb4naf2tvm2tm7yoml54pzvrmn77p4nvq4awfa4wufc3hn7qx@mof5q6li3xzv>
 <CAH2-Wzn1j2a0p3OqmqrV6zADtWA_QpG82U6F9yCYG1Uschm_fA@mail.gmail.com>
 <CAH2-WzmCH+N2-H2oGSQcbn2fArbk7GXyD6rQN6kn5P=FX9R-_g@mail.gmail.com>
 <CAH2-WzkyG01682zwqyUTwV=Zq+M_qGgi1NbXwp1H-piRSfJsgQ@mail.gmail.com>
 <CAH2-Wz=HJc+QV2AZ9mUY43aKL+n+a1JQ-7OGE=MOkqSAtoKJug@mail.gmail.com>
 <t6mtqbv2mbfhjni4bvwdgoecppjmxvbyfwl6utovzv76xc2672@k3o5ryevaeqv>
 <CAH2-Wz=D4Lru9BkvqaRnFRPDaZbfTOdWcxw13zyG6GVFTtz_vw@mail.gmail.com>
 <CAH2-Wz=Vxsgas35ZzOJJW1ceqp9TJ2DFhKmXULwUAcVpfD73xA@mail.gmail.com>
 <CAH2-Wz=kMg3PNay96cHMT0LFwtxP-cQSRZTZzh1Cixxf8G=zrw@mail.gmail.com>
In-Reply-To: 
 <CAH2-Wz=kMg3PNay96cHMT0LFwtxP-cQSRZTZzh1Cixxf8G=zrw@mail.gmail.com>
From: SATYANARAYANA NARLAPURAM <satyanarlapuram@gmail.com>
Date: Sat, 28 Mar 2026 23:15:55 -0700
Message-ID: 
 <CAHg+QDdsOe_7wyN_fdeKQmGkCTGkhnkSUCSNGOFWQZZTO0CdUQ@mail.gmail.com>
Subject: Re: index prefetching
To: Peter Geoghegan <pg@bowt.ie>
Cc: Andres Freund <andres@anarazel.de>, Tomas Vondra <tomas@vondra.me>,
	Alexandre Felipe <o.alexandre.felipe@gmail.com>,
 Thomas Munro <thomas.munro@gmail.com>,
	Nazir Bilal Yavuz <byavuz81@gmail.com>, Robert Haas <robertmhaas@gmail.com>,
	Melanie Plageman <melanieplageman@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
 Georgios <gkokolatos@protonmail.com>,
	Konstantin Knizhnik <knizhnik@garret.ru>, Dilip Kumar <dilipbalaut@gmail.com>
Content-Type: multipart/alternative; boundary="000000000000629c70064e23ab29"
Archived-At: 
 <https://www.postgresql.org/message-id/CAHg%2BQDdsOe_7wyN_fdeKQmGkCTGkhnkSUCSNGOFWQZZTO0CdUQ%40mail.gmail.com>
Precedence: bulk

--000000000000629c70064e23ab29
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Peter,


On Fri, Mar 27, 2026 at 5:36=E2=80=AFPM Peter Geoghegan <pg@bowt.ie> wrote:

> On Sun, Mar 22, 2026 at 9:14=E2=80=AFPM Peter Geoghegan <pg@bowt.ie> wrot=
e:
> > V17 is attached. This addresses most of your feedback, but defers
> > dealing with the trickier parts for now.
>
> Attached is v18, which addresses the tricky issues I skipped in v17.
>
> Highlights:
>
> * New commit (the first) that adds a heapam_indexscan.c file, and
> moves all of the relevant existing heapam_handler.c functions over to
> this new file. This commit is strictly mechanical, and has no
> functional impact.
>
> * Another new commit (the second) adds the new slot-based index scan
> interface, and pushes the VM lookups that currently take place in
> nodeIndexonlyScan.c into heapam. This includes the instrumentation
> changes that were previously in their own small patch. It also
> includes the VISITED_PAGES_LIMIT stuff -- there's no longer any commit
> that breaks VISITED_PAGES_LIMIT, nor is there a later commit that
> subsequently restores the functionality.
>
> * We now extract one key piece of infrastructure that other table AMs
> can now reuse to manage their own index scan batches: a new inline
> function called tableam_util_fetch_next_batch now appears as an
> indexbatch.h inline function.
>
> This function (which appeared under a different name in v17) manages
> deciding whether to call amgetbatch (performing the amgetbatch call
> when needed) or to simply return the next batch in line (because such
> a batch already exists/is already loaded in the ring buffer from
> earlier). It also detects and handles cross-batch changes in scan
> direction, and sets/reads the fields that track when a batch is
> definitely the last one in the given scan direction.
>
> I tried to go further by also placing the bulk of the code in
> heapam_index_getnext_scanbatch_pos in indexbatch.h, but ultimately
> concluded that it wasn't worth it. There isn't much code in
> heapam_index_getnext_scanbatch_pos to generalize. Some of that code
> coordinates quite directly with the heapam read stream callback.
>
> * Added an "isGuarded" field to IndexScanBatchData. That way we can
> assert that any batch loaded within the read stream callback is
> already unguarded/has no batch index page buffer pin still held. The
> field isn't just for assertions, though. We also rely on it in one or
> two places during release builds, which seemed clearer to me. For
> example, we no longer require that amunguardbatch routines be
> idempotent, which seems like a minor improvement.
>
> * Other work is broken out into its own commit, to better highlight
> issues that aren't strictly related to the amgetbatch changes.
> Specifically, a new commit adds the "don't unpin on rescan" behavior.
> These changes, which first appeared in the main amgetbatch commit, aim
> to minimize regressions with cached workloads (this was discussed
> briefly upthread, plus Andres and I discussed it over IM some weeks
> back).
>
> * Replaced what was previously an assert with an equivalent
> pg_assume(), within heapam_index_getnext_scanbatch_pos -- which is a
> very hot function.  We now "pg_assume(batchringbuf->headBatch =3D=3D
> scanPos->batch)", allowing the compiler to exploit this existing
> invariant. This seems to help the compiler generate better code,
> presumably by allowing more efficient register allocation. I haven't
> looked at the disassembly just yet, but it helps with certain
> microbenchmarks enough to matter.
>
> * The "Add UnlockBufferGetLSN utility function" commit now uses an
> atomic operation (in the common case where it actually drops the index
> page's pin), just like UnlockReleaseBuffer itself following Andres'
> commit f39cb8c011062d65e146c1e9d1aae221e96d8320 from today.
>
> I completed this item quickly, after pulling from master a little
> while ago. The expectation for UnlockBufferGetLSN is that it be at
> least as good as a BufferGetLSNAtomic followed by a
> UnlockReleaseBuffer, so IMV it made sense to include this last minute
> addition in v18.
>
> * The "don't wait" patches are no longer included, since Andres
> committed them to the master branch just now. Thanks to both you
> (Andres) and Melanie for taking care of this very thorny issue!
>
> I believe I have now acted on all of Andres' feedback, with one minor
> exception: Andres disliked how we use 'if (scan->xs_heapfetch)' to
> determine whether to use the scan's batch cache (we don't want to use
> it at all if the scan is ending). I just ran out of steam, so this
> version doesn't address the problem. But I'm still tracking it as an
> open item (I don't disagree with Andres, but this problem is slightly
> awkward). Note that I *did* create a helper function here, so we at
> least avoid having 2 copies of the loop that looks for a free cache
> batch slot.
>

I am still reviewing / understanding the patch, a couple quick comments
based on my review.

1. Looks like off-by-one in the for loop in the
patch v18-0015-WIP-aio-io_uring-Use-IO-size-not-IO-queue-to-tri.patch

@@ -730,7 +711,39 @@ pgaio_uring_sq_from_io(PgAioHandle *ioh, struct
io_uring_sqe *sqe)
  ioh->op_data.read.iov_length,
  ioh->op_data.read.offset);

+ for (int i =3D 0; i <=3D ioh->op_data.read.iov_length; i++, iov++)
+ io_size +=3D iov->iov_len;

2. NIT comment: _bt_endpoint return type changed to IndexScanBatch from
bool in the patch
v18-0005-Add-interfaces-that-enable-index-prefetching.patch.
But the places it is returning false would be  good to replace with NULL.
Though false is treat as 0/NULL and no compiler errors, it improves the
readability of the code.

 if (!BufferIsValid(btfirstbatch->buf))
{
/*
* Empty index. Lock the whole relation, as nothing finer to lock
* exists.
*/
PredicateLockRelation(rel, scan->xs_snapshot);
_bt_parallel_done(scan);
return false;
}

Thanks,
Satya

--000000000000629c70064e23ab29
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><div dir=3D"ltr"><div>Hi Peter,</div><div><br></div><br></=
div><br><div class=3D"gmail_quote gmail_quote_container"><div dir=3D"ltr" c=
lass=3D"gmail_attr">On Fri, Mar 27, 2026 at 5:36=E2=80=AFPM Peter Geoghegan=
 &lt;<a href=3D"mailto:pg@bowt.ie">pg@bowt.ie</a>&gt; wrote:<br></div><bloc=
kquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:=
1px solid rgb(204,204,204);padding-left:1ex">On Sun, Mar 22, 2026 at 9:14=
=E2=80=AFPM Peter Geoghegan &lt;<a href=3D"mailto:pg@bowt.ie" target=3D"_bl=
ank">pg@bowt.ie</a>&gt; wrote:<br>
&gt; V17 is attached. This addresses most of your feedback, but defers<br>
&gt; dealing with the trickier parts for now.<br>
<br>
Attached is v18, which addresses the tricky issues I skipped in v17.<br>
<br>
Highlights:<br>
<br>
* New commit (the first) that adds a heapam_indexscan.c file, and<br>
moves all of the relevant existing heapam_handler.c functions over to<br>
this new file. This commit is strictly mechanical, and has no<br>
functional impact.<br>
<br>
* Another new commit (the second) adds the new slot-based index scan<br>
interface, and pushes the VM lookups that currently take place in<br>
nodeIndexonlyScan.c into heapam. This includes the instrumentation<br>
changes that were previously in their own small patch. It also<br>
includes the VISITED_PAGES_LIMIT stuff -- there&#39;s no longer any commit<=
br>
that breaks VISITED_PAGES_LIMIT, nor is there a later commit that<br>
subsequently restores the functionality.<br>
<br>
* We now extract one key piece of infrastructure that other table AMs<br>
can now reuse to manage their own index scan batches: a new inline<br>
function called tableam_util_fetch_next_batch now appears as an<br>
indexbatch.h inline function.<br>
<br>
This function (which appeared under a different name in v17) manages<br>
deciding whether to call amgetbatch (performing the amgetbatch call<br>
when needed) or to simply return the next batch in line (because such<br>
a batch already exists/is already loaded in the ring buffer from<br>
earlier). It also detects and handles cross-batch changes in scan<br>
direction, and sets/reads the fields that track when a batch is<br>
definitely the last one in the given scan direction.<br>
<br>
I tried to go further by also placing the bulk of the code in<br>
heapam_index_getnext_scanbatch_pos in indexbatch.h, but ultimately<br>
concluded that it wasn&#39;t worth it. There isn&#39;t much code in<br>
heapam_index_getnext_scanbatch_pos to generalize. Some of that code<br>
coordinates quite directly with the heapam read stream callback.<br>
<br>
* Added an &quot;isGuarded&quot; field to IndexScanBatchData. That way we c=
an<br>
assert that any batch loaded within the read stream callback is<br>
already unguarded/has no batch index page buffer pin still held. The<br>
field isn&#39;t just for assertions, though. We also rely on it in one or<b=
r>
two places during release builds, which seemed clearer to me. For<br>
example, we no longer require that amunguardbatch routines be<br>
idempotent, which seems like a minor improvement.<br>
<br>
* Other work is broken out into its own commit, to better highlight<br>
issues that aren&#39;t strictly related to the amgetbatch changes.<br>
Specifically, a new commit adds the &quot;don&#39;t unpin on rescan&quot; b=
ehavior.<br>
These changes, which first appeared in the main amgetbatch commit, aim<br>
to minimize regressions with cached workloads (this was discussed<br>
briefly upthread, plus Andres and I discussed it over IM some weeks<br>
back).<br>
<br>
* Replaced what was previously an assert with an equivalent<br>
pg_assume(), within heapam_index_getnext_scanbatch_pos -- which is a<br>
very hot function.=C2=A0 We now &quot;pg_assume(batchringbuf-&gt;headBatch =
=3D=3D<br>
scanPos-&gt;batch)&quot;, allowing the compiler to exploit this existing<br=
>
invariant. This seems to help the compiler generate better code,<br>
presumably by allowing more efficient register allocation. I haven&#39;t<br=
>
looked at the disassembly just yet, but it helps with certain<br>
microbenchmarks enough to matter.<br>
<br>
* The &quot;Add UnlockBufferGetLSN utility function&quot; commit now uses a=
n<br>
atomic operation (in the common case where it actually drops the index<br>
page&#39;s pin), just like UnlockReleaseBuffer itself following Andres&#39;=
<br>
commit f39cb8c011062d65e146c1e9d1aae221e96d8320 from today.<br>
<br>
I completed this item quickly, after pulling from master a little<br>
while ago. The expectation for UnlockBufferGetLSN is that it be at<br>
least as good as a BufferGetLSNAtomic followed by a<br>
UnlockReleaseBuffer, so IMV it made sense to include this last minute<br>
addition in v18.<br>
<br>
* The &quot;don&#39;t wait&quot; patches are no longer included, since Andr=
es<br>
committed them to the master branch just now. Thanks to both you<br>
(Andres) and Melanie for taking care of this very thorny issue!<br>
<br>
I believe I have now acted on all of Andres&#39; feedback, with one minor<b=
r>
exception: Andres disliked how we use &#39;if (scan-&gt;xs_heapfetch)&#39; =
to<br>
determine whether to use the scan&#39;s batch cache (we don&#39;t want to u=
se<br>
it at all if the scan is ending). I just ran out of steam, so this<br>
version doesn&#39;t address the problem. But I&#39;m still tracking it as a=
n<br>
open item (I don&#39;t disagree with Andres, but this problem is slightly<b=
r>
awkward). Note that I *did* create a helper function here, so we at<br>
least avoid having 2 copies of the loop that looks for a free cache<br>
batch slot.<br></blockquote><div><br></div><div>I am still reviewing / unde=
rstanding the patch, a couple quick comments based on my review.</div><div>=
<br></div><div>1. Looks like off-by-one in the for loop in the patch=C2=A0v=
18-0015-WIP-aio-io_uring-Use-IO-size-not-IO-queue-to-tri.patch</div><div><b=
r></div>@@ -730,7 +711,39 @@ pgaio_uring_sq_from_io(PgAioHandle *ioh, struc=
t io_uring_sqe *sqe)<br>=C2=A0									ioh-&gt;op_data.read.iov_length,<br>=
=C2=A0									ioh-&gt;op_data.read.offset);<br>=C2=A0<br>+				for (int i =
=3D 0; i &lt;=3D ioh-&gt;op_data.read.iov_length; i++, iov++)<br><div>+				=
	io_size +=3D iov-&gt;iov_len;=C2=A0</div><div><br></div><div>2. NIT commen=
t: _bt_endpoint return type changed to IndexScanBatch from bool in the patc=
h v18-0005-Add-interfaces-that-enable-index-prefetching.patch.</div><div>Bu=
t the places it is returning false would be=C2=A0 good to replace with NULL=
. Though false is treat as 0/NULL and no compiler errors, it improves the r=
eadability of the code.</div><div><br></div><div>=C2=A0if (!BufferIsValid(b=
tfirstbatch-&gt;buf))<br>	{<br>		/*<br>		 * Empty index. Lock the whole rel=
ation, as nothing finer to lock<br>		 * exists.<br>		 */<br>		PredicateLock=
Relation(rel, scan-&gt;xs_snapshot);<br>		_bt_parallel_done(scan);<br>		ret=
urn false;<br>	}</div><div><br></div><div>Thanks,</div><div>Satya</div><div=
><br></div><div><br></div></div></div>

--000000000000629c70064e23ab29--