MIME-Version: 1.0
References: 
 <CAH2-Wz=D4Lru9BkvqaRnFRPDaZbfTOdWcxw13zyG6GVFTtz_vw@mail.gmail.com>
 <CAH2-Wz=Vxsgas35ZzOJJW1ceqp9TJ2DFhKmXULwUAcVpfD73xA@mail.gmail.com>
 <CAH2-Wz=kMg3PNay96cHMT0LFwtxP-cQSRZTZzh1Cixxf8G=zrw@mail.gmail.com>
 <CAH2-WzkFRoTjD9T8ykYDzOMxzGiWFqcAkbK8B=HjfpoMdM4E8A@mail.gmail.com>
 <chsvntdxvsiyigxq4nng36gne4natvxwvsqnkvbjlpaw6bu7co@a6togdo4wbrj>
 <CAH2-Wz=nz-zr=gaXL1od_F7dcr=7d+3jEEveqY-bgcAKF6wZJQ@mail.gmail.com>
 <CAH2-Wz=t3G53xKGYEWqm_QV35ExRgT2k=qhw_VHe5oGjdFRwtA@mail.gmail.com>
 <CAH2-WzkiCK=wELiXPgriN4r7cJzGb3Xg48E9YHrFEyEPTkynOw@mail.gmail.com>
 <s25f6opuqwmz7oad467twvgxc36zmgzguhph43z4sbfkflsjnq@r6cuj7cl36lg>
 <CAH2-Wzm9r6sCDVkXHXLobQFE3WcEPf-pm=g6S93u4W9JLK2VDA@mail.gmail.com>
 <6sphk3ycctmbihlrykts7uj6mjakop6wrq2dhe3vnlmrnldz2f@uuwmkd6jjrxa>
In-Reply-To: <6sphk3ycctmbihlrykts7uj6mjakop6wrq2dhe3vnlmrnldz2f@uuwmkd6jjrxa>
From: Peter Geoghegan <pg@bowt.ie>
Date: Mon, 6 Apr 2026 01:47:43 -0400
Message-ID: 
 <CAH2-Wzn=vwK+9mv5HuNfuaPkhT+0Ou-E1_Y8miGNJZm-WxHORg@mail.gmail.com>
Subject: Re: index prefetching
To: Andres Freund <andres@anarazel.de>
Cc: 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: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAH2-Wzn%3DvwK%2B9mv5HuNfuaPkhT%2B0Ou-E1_Y8miGNJZm-WxHORg%40mail.gmail.com>
Precedence: bulk

On Sun, Apr 5, 2026 at 7:01=E2=80=AFPM Andres Freund <andres@anarazel.de> w=
rote:
> > It's unclear if you're asking (or at least suggesting) that I should
> > invent a new table_index_getnext_slot-like shim, used only during
> > index-only scans, that doesn't require passing a slot (but does
> > require getting a table slot some other way, likely at scan startup).
>
> Why would it require creating a tableam shaped slot internally?  It seems
> rather silly to store a tuple fetched to verify visibility into a slot, w=
here
> it is never used. Storing in a tuple requires an indirect things like
> IncrBufferRefCount(), which isn't that cheap.
>
> heapam_index_getnext_slot(index_only=3D>1)
> -> heapam_index_fetch_heap_item()
> -> heapam_index_fetch_tuple()

The fact is that nodeIndexOnlyscan.c calls index_fetch_heap right now,
on master, which uses a TupleTableSlot.

Yes, using a slot is suboptimal. But it's nothing new. And this is the
first time I'm hearing about this being a problem. A couple of days
before feature freeze.

> So I guess yes, I am suggesting that we have a separate
> table_index_getnext_ios() or such, which is only usable for index only sc=
ans.

What if we offered such an interface but still used a slot internally?
That way we'd have the API you're asking for, without any significant
changes to heap_hot_search_buffer. The caller doesn't need to know
about the table slot's usage.

I get that making the caller use a slot interface with a slot that
they don't exactly need is a bit hokey. We can fix that part, without
seriously risking not getting the patch into 19.

> > Actually, I don't know if we really *should* treat the selfuncs.c
> > caller any different (except as needed to avoid spurious assertion
> > failures).

> I don't think it's worth assuming that that's the code in the core
> infrstructure.

To be clear, I agree.

>         scan->heapRelation->rd_tableam->index_fetch_batch_init(scan, batc=
h, new_alloc);
>
> that's a lot of indirection to go through.
>
>
> Which is what then made me think of embedding something like a
> TableIndexFetchRoutine (with batch_init, reset, markpos, restrpos, end
> members) in IndexScanDesc. That way table_index_{fetch_reset,batch_Init,.=
..}()
> wouldn't need this level of indirection.

But that's the same level of indirection as before? Except it's once
per batch instead of once per tuple.

> Not having the same code in multiple table AMs would make it easier to wr=
ite
> them, which would be nice in itself, but I am more concerned with ending =
up
> with copies of heapam_index_getnext_scanbatch_pos() in various AMs, us fi=
nding
> problems with heapam_index_getnext_scanbatch_pos() in a minor release, an=
d
> having a harder time adjusting things due to the copied code.

I'll need to study this further.

> > What do you want to do about it? index_getnext_tid isn't adding too
> > much right now, I'd say.
>
> It does indeed not add a whole lot.

> I would move it into a tableam_util_fetch_next_tid() (or such) helper, I
> think.  That also makes it easier to get rid of the table_index_fetch_res=
et()
> and move the pgstat_count_index_tuples(scan->indexRelation, 1) into one p=
lace.

I'll put it into indexbatch.h (next to the function that calls
amgetbatch), and make sure that the amgettuple path is consistent in
terms of how it calls pgstat_count_index_tuples(), and how the
fetch/index scan gets reset.

I don't think that we should be resetting here -- even if it's the
historical behavior. I'm absolutely sure that whatever we do shouldn't
vary based on whether amgetbatch or amgettuple was used (that was not
intended).

> > > Can'tt the *2 lead to computing a xs_vm_items bigger than scan->maxit=
emsbatch?
> >
> > Yes, it can.
>
> But we prevent it from overflow by a Max()/Min() in the loop headers.

Right.

> > > I'd set scan->batchcache[i] to NULL after freeing.
> >
> > I tried that out, but I *think* it regressed pgbench SELECT by about
> > ~1.5% of total TPS (I'd need more thorough testing to confirm this).
> > Are you sure we need this?
>
> That seems surprising. But no, I don't think we need this.

I'll revisit, might have been wrong.

> It's perhaps ok, I just was a bit surprised to see the callback inside a
> function with that name.  I'd perhaps just name it
> tableam_util_release_batch() (and rename batch_free accordingly).

Will do.

> I'd probably just assert !kill_prior_tuple.

Will do.


--=20
Peter Geoghegan