Re: Revise the Asserts added to bimapset manipulation functions - Mailing list pgsql-hackers

From Richard Guo
Subject Re: Revise the Asserts added to bimapset manipulation functions
Date
Msg-id CAMbWs4-s+db9tzPHwOR0eQ80QHCDmus+0dYpzvYAnYr43TK6ig@mail.gmail.com
Whole thread Raw
In response to Re: Revise the Asserts added to bimapset manipulation functions  (David Rowley <dgrowleyml@gmail.com>)
Responses Re: Revise the Asserts added to bimapset manipulation functions
List pgsql-hackers

On Fri, Dec 29, 2023 at 9:15 AM David Rowley <dgrowleyml@gmail.com> wrote:
I looked into this a bit more and I just can't see why the current
code feels like it must do the reallocation in functions such as
bms_del_members().  We don't repalloc the set there, ever, so why do
we need to do it when building with REALLOCATE_BITMAPSETS?  It seems
to me the point of REALLOCATE_BITMAPSETS is to ensure that *if* we
possibly could end up reallocating the set that we *do* reallocate.

I think the argument here is whether the REALLOCATE_BITMAPSETS option is
intended to force a reallocation for every modification of a bitmapset,
or only for modifications that could potentially require the set to be
reallocated.

IIUC, the v2 patch addresses the latter scenario.  I agree that it can
help find bugs in cases where there's more than one reference to a set,
and a modification that could reallocate the bitmapset might leave the
other references being dangling pointers.

It seems to me that the former scenario also makes sense in some cases.
For instance, let's say there are two pointers in two structs, s1->p and
s2->p, both referencing the same bitmapset.  If we modify the bitmapset
via s1->p (even if no reallocation could occur), s2 would see different
content via its pointer 'p'.  Sometimes this is just wrong and could
cause problems.  If we can force a reallocation for every modification
of the bitmapset, it'd be much easier to find such bugs.

Having said that, I think the codes checked in by 71a3e8c43b and
7d58f2342b are far from perfect.  And I agree that the bms_copy_and_free
in v2 patch is a good idea, as well as the bms_is_valid_set.

Thanks
Richard

pgsql-hackers by date:

Previous
From: jian he
Date:
Subject: autoprewarm main function not tested background worker not listed in pg_stat_activity
Next
From: vignesh C
Date:
Subject: Re: pg_upgrade and logical replication