Re: Revise the Asserts added to bimapset manipulation functions

On Thu, 28 Dec 2023 at 23:21, David Rowley <dgrowleyml@gmail.com> wrote:
> then instead of having to do:
>
> #ifdef REALLOCATE_BITMAPSETS
> a = (Bitmapset *) palloc(BITMAPSET_SIZE(tmp->nwords));
> memcpy(a, tmp, BITMAPSET_SIZE(tmp->nwords));
> pfree(tmp);
> #endif
>
> all over the place.  Couldn't we do:
>
> #ifdef REALLOCATE_BITMAPSETS
>      return bms_clone_and_free(a);
> #else
>      return a;
> #endif

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.

There's also a few cases where you could argue that the
REALLOCATE_BITMAPSETS code has introduced bugs.  For example,
bms_del_members(), bms_join() and bms_int_members() are meant to
guarantee that their left input (both inputs in the case of
bms_join()) are recycled. Compiling with REALLOCATE_BITMAPSETS
invalidates that, so it seems about as likely that building with
REALLOCATE_BITMAPSETS could cause bugs rather than find bugs.

I've hacked a bit on this to fix these problems and also added some
documentation to try to offer future people modifying bitmapset.c some
guidance on if they need to care about REALLOCATE_BITMAPSETS.

I also don't think "hangling" is a word. So I've adjusted the comment
in pg_config_manual.h to fix that.

David