Re: [PoC] Improve dead tuple storage for lazy vacuum

On Mon, Mar 25, 2024 at 1:53 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> John Naylor <johncnaylorls(at)gmail(dot)com> writes:
> > Done. I pushed this with a few last-minute cosmetic adjustments. This
> > has been a very long time coming, but we're finally in the home
> > stretch!

Thank you for the report.

>
> I'm not sure why it took a couple weeks for Coverity to notice
> ee1b30f12, but it saw it today, and it's not happy:

Hmm, I've also done Coverity Scan in development but I wasn't able to
see this one for some reason...

>
> /srv/coverity/git/pgsql-git/postgresql/src/include/lib/radixtree.h: 1621 in local_ts_extend_down()
> 1615 node = child;
> 1616 shift -= RT_SPAN;
> 1617 }
> 1618
> 1619 /* Reserve slot for the value. */
> 1620 n4 = (RT_NODE_4 *) node.local;
> >>> CID 1594658: Integer handling issues (BAD_SHIFT)
> >>> In expression "key >> shift", shifting by a negative amount has undefined behavior. The shift amount, "shift", is as little as -7.
> 1621 n4->chunks[0] = RT_GET_KEY_CHUNK(key, shift);
> 1622 n4->base.count = 1;
> 1623
> 1624 return &n4->children[0];
> 1625 }
> 1626
>
> I think the point here is that if you start with an arbitrary
> non-negative shift value, the preceding loop may in fact decrement it
> down to something less than zero before exiting, in which case we
> would indeed have trouble. I suspect that the code is making
> undocumented assumptions about the possible initial values of shift.
> Maybe some Asserts would be good? Also, if we're effectively assuming
> that shift must be exactly zero here, why not let the compiler
> hard-code that?
>
> - n4->chunks[0] = RT_GET_KEY_CHUNK(key, shift);
> + n4->chunks[0] = RT_GET_KEY_CHUNK(key, 0);

Sounds like a good solution. I've attached the patch for that.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com