Horton

Re: Confine vacuum skip logic to lazy_scan_skip

Tom Lane <tgl@sss.pgh.pa.us>

View thread View on postgr.es
From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andres Freund <andres@anarazel.de>
Cc: Ranier Vilela <ranier.vf@gmail.com>, Melanie Plageman <melanieplageman@gmail.com>, Thomas Munro <thomas.munro@gmail.com>, Masahiko Sawada <sawada.mshk@gmail.com>, Tomas Vondra <tomas@vondra.me>, Noah Misch <noah@leadboat.com>, vignesh C <vignesh21@gmail.com>, 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-02-27T18:08:47Z
Lists: pgsql-hackers 
Andres Freund <andres@anarazel.de> writes:
> Ah, no, it isn't. But I still think the coverity alert and the patch don't
> make sense, as per the below:

Coverity's alert makes perfect sense if you posit that Coverity
doesn't assume that this read_stream_next_buffer call will
only be applied to a stream that has per_buffer_data_size > 0.
(Even if it did understand that, I wouldn't assume that it's
smart enough to see that the fast path will never be taken.)

I wonder if it'd be a good idea to add something like

		Assert(stream->distance == 1);
		Assert(stream->pending_read_nblocks == 0);
		Assert(stream->per_buffer_data_size == 0);
+		Assert(per_buffer_data == NULL);

in read_stream_next_buffer.  I doubt that this will shut Coverity
up, but it would help to catch caller coding errors, i.e. passing
a per_buffer_data pointer when there's no per-buffer data.

On the whole I doubt we can get rid of this warning without some
significant redesign of the read_stream API, and I don't think
it's worth the trouble.  Coverity is a tool not a requirement.
I'm content to just dismiss the warning.

			regards, tom lane

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.