Horton

Re: Confine vacuum skip logic to lazy_scan_skip

John Naylor <johncnaylorls@gmail.com>

View thread View on postgr.es
From: John Naylor <johncnaylorls@gmail.com>
To: Melanie Plageman <melanieplageman@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Thomas Munro <thomas.munro@gmail.com>, Noah Misch <noah@leadboat.com>, vignesh C <vignesh21@gmail.com>, Andres Freund <andres@anarazel.de>, Pg Hackers <pgsql-hackers@postgresql.org>, Heikki Linnakangas <hlinnaka@iki.fi>, Nazir Bilal Yavuz <byavuz81@gmail.com>, Robert Haas <robertmhaas@gmail.com>, "Andrey M. Borodin" <x4mmm@yandex-team.ru>
Date: 2025-11-04T09:27:08Z
Lists: pgsql-hackers 
On Mon, Nov 3, 2025 at 10:59 PM Melanie Plageman
<melanieplageman@gmail.com> wrote:
>
> > 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));

My "not sure" was referring to this line.

> 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.

I suspect the contract is the way it is in order to enable the assert to work.

> 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.

This line seems strange to me (and maybe even stranger to have both
the min and the assert), but maybe I don't understand Tom's rationale
well enough. Do we need it to silence Coverity?

-- 
John Naylor
Amazon Web Services

Commits

  1. Fix explicit valgrind interaction in read_stream.c.

  2. Reduce scope of heap vacuum per_buffer_data

  3. Use streaming read I/O in VACUUM's third phase

  4. Use streaming read I/O in VACUUM's first phase

  5. Convert heap_vac_scan_next_block() boolean parameters to flags

  6. Refactor tidstore.c iterator buffering.

  7. Increase default vacuum_buffer_usage_limit to 2MB.

  8. Remove unneeded vacuum_delay_point from heap_vac_scan_get_next_block

  9. Confine vacuum skip logic to lazy_scan_skip()

  10. Set all_visible_according_to_vm correctly with DISABLE_PAGE_SKIPPING

  11. Tighten up VACUUM's approach to setting VM bits.