From: Tom Lane <tgl@sss.pgh.pa.us>
To: Thomas Munro <thomas.munro@gmail.com>
cc: Noah Misch <noah@leadboat.com>,
        Melanie Plageman <melanieplageman@gmail.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+hUKGKN3oy0bN_3yv8hd78a4+M1tJC9z7mD8+f+yA+GeoFUwQ@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>
Comments: In-reply-to Thomas Munro <thomas.munro@gmail.com>
	message dated "Wed, 24 Jul 2024 17:40:12 +1200"
MIME-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Content-ID: <798101.1761149528.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Wed, 22 Oct 2025 12:12:08 -0400
Message-ID: <798102.1761149528@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/798102.1761149528%40sss.pgh.pa.us>
Precedence: bulk

[ seizing on this old commit as being most closely related to the issue ]

Thomas Munro <thomas.munro@gmail.com> writes:
> On Tue, Jul 16, 2024 at 1:52=E2=80=AFPM Noah Misch <noah@leadboat.com> w=
rote:
>> On Mon, Jul 15, 2024 at 03:26:32PM +1200, Thomas Munro wrote:
>> That's reasonable.  radixtree already forbids mutations concurrent with
>> iteration, so there's no new concurrency hazard.  One alternative is
>> per_buffer_data big enough for MaxOffsetNumber, but that might thrash c=
aches
>> measurably.  That patch is good to go apart from these trivialities:

> Thanks!  I have pushed that patch, without those changes you didn't like=
.

The security team recently updated our Coverity instance to the latest
version, and it's started complaining as follows:


*** CID 1667418:         Memory - corruptions  (OVERRUN)
/srv/coverity/git/pgsql-git/postgresql/src/backend/access/heap/vacuumlazy.=
c: 2812             in lazy_vacuum_heap_rel()
2806              * already have the correct page pinned anyway.
2807              */
2808             visibilitymap_pin(vacrel->rel, blkno, &vmbuffer);
2809     =

2810             /* We need a non-cleanup exclusive lock to mark dead_item=
s unused */
2811             LockBuffer(buf, BUFFER_LOCK_EXCLUSIVE);
>>>     CID 1667418:         Memory - corruptions  (OVERRUN)
>>>     Overrunning callee's array of size 291 by passing argument "num_of=
fsets" (which evaluates to 2048) in call to "lazy_vacuum_heap_page".
2812             lazy_vacuum_heap_page(vacrel, blkno, buf, offsets,
2813                                   num_offsets, vmbuffer);
2814     =

2815             /* Now that we've vacuumed the page, record its available=
 space */
2816             page =3D BufferGetPage(buf);
2817             freespace =3D PageGetHeapFreeSpace(page);


The reason it thinks that num_offsets could be as much as 2048 is
presumably the code a little bit above this:

        OffsetNumber offsets[MaxOffsetNumber];
        ...
        num_offsets =3D TidStoreGetBlockOffsets(iter_result, offsets, leng=
thof(offsets));
        Assert(num_offsets <=3D lengthof(offsets));

However, lazy_vacuum_heap_page blindly assumes that the passed value
will be no more than MaxHeapTuplesPerPage.  It seems like we ought
to get these two functions in sync, either both using MaxOffsetNumber
or both using MaxHeapTuplesPerPage for their array sizes.

It looks to me like MaxHeapTuplesPerPage should be sufficient.
Also, after reading TidStoreGetBlockOffsets I wonder if we
should replace that Assert with

        num_offsets =3D Min(num_offsets, lengthof(offsets));

Thoughts?

			regards, tom lane