On Wed, 27 Dec 2023 at 22:30, Richard Guo <guofengli...@gmail.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
