Subject: Revise the Asserts added to bimapset manipulation functions

From: Richard Guo <[email-masked]>
Date: Wed, 27 Dec 2023 17:30:01 +0800

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,

@@ -953,6 +1033,15 @@ bms_int_members(Bitmapset *a, const Bitmapset *b)
        int                     lastnonzero;
        int                     shortlen;
        int                     i;
+#ifdef REALLOCATE_BITMAPSETS
+       Bitmapset  *tmp = a;
+#endif
+
+       Assert(a == NULL || IsA(a, Bitmapset));
+       Assert(b == NULL || IsA(b, Bitmapset));
+
+       Assert(a == NULL || IsA(a, Bitmapset));
+       Assert(b == NULL || IsA(b, Bitmapset));

Sorry that I failed to notice those duplicates when reviewing the
patchset, mainly because they were introduced in different patches.

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.

In passing, I prefer to move the Asserts to the beginning of functions,
just for paranoia's sake.

Hence, proposed patch attached.

Thanks
Richard

---

From: David Rowley <[email-masked]>
Date: Thu, 28 Dec 2023 23:21:50 +1300

> 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

> I think we can define a new macro for this form and use it to check that

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