Horton

Re: Confine vacuum skip logic to lazy_scan_skip

Thomas Munro <thomas.munro@gmail.com>

View thread View on postgr.es
From: Thomas Munro <thomas.munro@gmail.com>
To: Melanie Plageman <melanieplageman@gmail.com>
Cc: Masahiko Sawada <sawada.mshk@gmail.com>, Tomas Vondra <tomas@vondra.me>, 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-02-14T21:50:40Z
Lists: pgsql-hackers 
On Sat, Feb 15, 2025 at 7:30 AM Melanie Plageman
<melanieplageman@gmail.com> wrote:
> Seems valgrind doesn't like this [1]. I'm looking into it.

Melanie was able to reproduce this on her local valgrind and
eventually we figured out that it's my fault.  I put code into
read_stream.c that calls wipe_mem(), thinking that that was our
standard way of scribbling 0x7f on memory that you shouldn't access
again until it's reused.  I didn't realise that wipe_mem() also tells
valgrind that the memory is now "no access".  That makes sense for
palloc/pfree because when that range is allocated again it'll clear
that.  The point is to help people discover that they have a dangling
reference to per-buffer data after they advance to the next buffer,
which wouldn't work because it's in a circular queue and could be
overwritten any time after that.

This fixes it, but is not yet my proposed change:

@@ -193,9 +193,12 @@ read_stream_get_block(ReadStream *stream, void
*per_buffer_data)
        if (blocknum != InvalidBlockNumber)
                stream->buffered_blocknum = InvalidBlockNumber;
        else
+       {
+               VALGRIND_MAKE_MEM_UNDEFINED(per_buffer_data,
stream->per_buffer_data_size);
                blocknum = stream->callback(stream,

 stream->callback_private_data,

 per_buffer_data);
+       }

Thinking about how to make it more symmetrical...

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.