From: Tom Lane <tgl@sss.pgh.pa.us>
To: Melanie Plageman <melanieplageman@gmail.com>
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>
Subject: Re: Confine vacuum skip logic to lazy_scan_skip
In-reply-to: 
 <CA+hUKGL8bAsQNj-bB_qxFFzoQW+RMZ3P3zK2SVa6b08G55BdRg@mail.gmail.com>
References: 
 <CAAKRu_ZuYsf4MNGzm7_KvG0SKsu-EtYRhFHy2x3q0heX8KLMTg@mail.gmail.com>
 <CAAKRu_bQgf4e9OveCQX++zTEJLKbZbqLeapA7SNkNuMjxcCO1w@mail.gmail.com>
 <20240306234733.nd4a636colxkgq2e@liskov>
 <CAAKRu_aiaf+CufVA5Rrksz4Lw2fvZ3M-OgdBZ8WpzvYoQY96MQ@mail.gmail.com>
 <CA+hUKGKHb3i8Wy72VCKZGA2B5djoj7tAzYAzbeS=Gwr_SdhgRw@mail.gmail.com>
 <CAAKRu_byDppRvNJ+p5kq-SmXZDQuL6L8N6D06pHbO6WnBnanWQ@mail.gmail.com>
 <CA+hUKGLY4Q4ZY4f1rvnFtv6+PkjNf8MejdPkcju3Qii9DYqqcQ@mail.gmail.com>
 <CAAKRu_bbkmwAzSBgnezancgJeXrQZXy4G4kBTd+5=cr86H5yew@mail.gmail.com>
 <20240707144944.58.nmisch@google.com>
 <CA+hUKGK3sB-T+Ao8EBCOu5M1MsCMPnPpNyXuQGkQYHU4WC35eA@mail.gmail.com>
 <20240716015226.60.nmisch@google.com>
 <CA+hUKGKN3oy0bN_3yv8hd78a4+M1tJC9z7mD8+f+yA+GeoFUwQ@mail.gmail.com>
 <eb657bfe-ba96-4aef-82f9-b952724de863@vondra.me>
 <CAAKRu_aLwANZpxHc0tC-6OT0OQT4TftDGkKAO5yigMUOv_Tcsw@mail.gmail.com>
 <85696b8e-f1bf-459e-ba97-5608c644c185@vondra.me>
 <CAAKRu_ZoOSZHexy=7YZ9W0CfeSHc1tGx7VCperZHWBxxz0gxzw@mail.gmail.com> <CA!
 AKRu_bmx33jTqATP5GKNFYwAg02a9dDtk4U_ciEjgBHZSVkOQ@mail.gmail.com>
 <CAD21AoDu_a0bECcOuhev08J_VUUR_3wL2fBM1D7_49Vy7A1XqA@mail.gmail.com>
 <CAAKRu_YP=-Mu9rG+q8CBGFMsurMMv-8T-=is80RMr0kR+VbAfA@mail.gmail.com>
 <CAD21AoDdx87ur-YA6qz3mKO0C7-wnQJW+45H6eEkDSP_rNB8hA@mail.gmail.com>
 <CAAKRu_Zbay2UeANcg-X455Q10nZJ_XH31Qph2wM4qDrZVa28JQ@mail.gmail.com>
 <CAAKRu_bR4ghLfsgDNnHm0gK2SOcG6qicmeyaPG2y0Ppjz1G6cw@mail.gmail.com>
 <CAAKRu_ac9DMqN2sSxLLK-D6m4V=2=uKufQUrmv82mQjch4F67A@mail.gmail.com>
 <CA+hUKG+g6aXpi2FEHqeLOzE+xYw=OV+-N5jhOEnnV+F0USM9xA@mail.gmail.com>
 <CA+hUKGJ84L0yFD2S05WeCOXmBgBcYb2S5wMmzyohcMJeCwgksw@mail.gmail.com>
 <3915749.1739574230@sss.pgh.pa.us>
 <CA+hUKGKgHs9xjOZrjjuaEoYSxBvQiYOgDDQiQ6yTSzqFwx6d2Q@mail.gmail.com>
 <CAAKRu_Z35kymRA5ru1TeAZtnOd46o7iDCyJgF9P8TH5geKy46A@mail.gmail.com>
 <CA+hUKGL8bAsQNj-bB_qxFFzoQW+RMZ3P3zK2SVa6b08G55BdRg@mail.gmail.com>
Comments: In-reply-to Thomas Munro <thomas.munro@gmail.com>
	message dated "Sat, 15 Feb 2025 14:23:08 +1300"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <626103.1739729538.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Sun, 16 Feb 2025 13:12:18 -0500
Message-ID: <626104.1739729538@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/626104.1739729538%40sss.pgh.pa.us>
Precedence: bulk

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 =3D 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 =3D *((uint8 *) per_buffer_data);
1296     		CheckBufferIsPinnedOnce(buf);
1297     		page =3D BufferGetPage(buf);
1298     		blkno =3D 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".  However,
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.

			regards, tom lane