Horton

Re: Confine vacuum skip logic to lazy_scan_skip

Melanie Plageman <melanieplageman@gmail.com>

View thread View on postgr.es
From: Melanie Plageman <melanieplageman@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: 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>, 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-18T14:31:03Z
Lists: pgsql-hackers 
On Sun, Feb 16, 2025 at 1:12 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Thomas Munro <thomas.munro@gmail.com> writes:
> > Thanks!  It's green again.
>
> The security team's Coverity instance complained about this patch:
>
> *** CID 1642971:  Null pointer dereferences  (FORWARD_NULL)
> /srv/coverity/git/pgsql-git/postgresql/src/backend/access/heap/vacuumlazy.c: 1295 in lazy_scan_heap()
> 1289                    buf = read_stream_next_buffer(stream, &per_buffer_data);
> 1290
> 1291                    /* The relation is exhausted. */
> 1292                    if (!BufferIsValid(buf))
> 1293                            break;
> 1294
> >>>     CID 1642971:  Null pointer dereferences  (FORWARD_NULL)
> >>>     Dereferencing null pointer "per_buffer_data".
> 1295                    blk_info = *((uint8 *) per_buffer_data);
> 1296                    CheckBufferIsPinnedOnce(buf);
> 1297                    page = BufferGetPage(buf);
> 1298                    blkno = BufferGetBlockNumber(buf);
> 1299
> 1300                    vacrel->scanned_pages++;
>
> Basically, Coverity doesn't understand that a successful call to
> read_stream_next_buffer must set per_buffer_data here.  I don't
> think there's much chance of teaching it that, so we'll just
> have to dismiss this item as "intentional, not a bug".

Is this easy to do? Like is there a list of things from coverity to ignore?

> I do have a suggestion: I think the "per_buffer_data" variable
> should be declared inside the "while (true)" loop not outside.
> That way there is no chance of a value being carried across
> iterations, so that if for some reason read_stream_next_buffer
> failed to do what we expect and did not set per_buffer_data,
> we'd be certain to get a null-pointer core dump rather than
> accessing data from a previous buffer.

Done and pushed. Thanks!

- Melanie

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.