Re: Revise the Asserts added to bimapset manipulation functions

On Wed, 27 Dec 2023 at 22:30, Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
> The Asserts added to bitmapset.c by commits 71a3e8c43b and 7d58f2342b
> contain some duplicates, such as in bms_difference, bms_is_subset,
> bms_subset_compare, bms_int_members and bms_join. For instance,

I'm just learning of these changes now. I wonder why that wasn't
done more like:

#ifdef REALLOCATE_BITMAPSETS
static Bitmapset *
bms_clone_and_free(Bitmapset *a)
{
Bitmapset *c = bms_copy(a);

bms_free(a);
return c;
}
#endif

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 think it's best to leave at least that and not hide the behaviour
inside a macro.

It would also be good if REALLOCATE_BITMAPSETS was documented in
bitmapset.c to offer some guidance to people modifying the code so
they know under what circumstances they need to return a copy. There
are no comments that offer any indication of what the intentions are
with this :-( What's written in pg_config_manual.h isn't going to
help anyone that's modifying bitmapset.c

> While removing those duplicates, I think we can add checks in the new
> Asserts to ensure that Bitmapsets should not contain trailing zero
> words, as the old Asserts did. That makes the Asserts in the form of:
>
> Assert(a == NULL || (IsA(a, Bitmapset) && a->words[a->nwords - 1] != 0));
>
> I think we can define a new macro for this form and use it to check that
> a Bitmapset is valid.

I think that's an improvement. I did have a function for this in [1],
but per [2], Tom wasn't a fan. I likely shouldn't have bothered with
the loop there. It seems fine just to ensure the final word isn't 0.

David

[1] https://postgr.es/m/CAApHDvr5O41MuUjw0DQKqmAnv7QqvmLqXReEd5o4nXTzWp8-%2Bw%40mail.gmail.com
[2] https://postgr.es/m/2686153.1677881312%40sss.pgh.pa.us