Re: Confine vacuum skip logic to lazy_scan_skip

On Thu, Oct 30, 2025 at 7:14 AM John Naylor <johncnaylorls(at)gmail(dot)com> wrote:
>
> On Wed, Oct 22, 2025 at 11:12 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > The reason it thinks that num_offsets could be as much as 2048 is
> > presumably the code a little bit above this:
> >
> > OffsetNumber offsets[MaxOffsetNumber];
> > ...
> > num_offsets = TidStoreGetBlockOffsets(iter_result, offsets, lengthof(offsets));
> > Assert(num_offsets <= lengthof(offsets));
> >
> > However, lazy_vacuum_heap_page blindly assumes that the passed value
> > will be no more than MaxHeapTuplesPerPage. It seems like we ought
> > to get these two functions in sync, either both using MaxOffsetNumber
> > or both using MaxHeapTuplesPerPage for their array sizes.
> >
> > It looks to me like MaxHeapTuplesPerPage should be sufficient.
>
> Seems right.

Yes, it makes sense to me to make offsets size MaxHeapTuplesPerPage,
if that is what is being suggested. Doesn't hurt to take up a bit less
stack space too.

> > Also, after reading TidStoreGetBlockOffsets I wonder if we
> > should replace that Assert with
> >
> > num_offsets = Min(num_offsets, lengthof(offsets));
> >
> > Thoughts?
>
> Not sure. That changes the posture from "can't happen" to "shouldn't
> happen, but if it does, don't cause a disaster". Even with the latter,
> the assert still seems appropriate for catching developer mistakes.

You are suggesting keeping the assert and this line after it?

num_offsets = Min(num_offsets, lengthof(offsets));

The current contract of TidStoreGetBlockOffsets() is that it won't
return a value larger than max_offsets passed in, so it is a good idea
to have an assert in case it changes. But, if we take the minimum,
then is the assert there to keep developers from changing
TidStoreGetBlockOffsets() from behaving differently? I don't know if I
like that, but I don't feel strongly enough to object. Anyway, I think
we should add the line Tom suggested.

- Melanie