Re: Revise the Asserts added to bimapset manipulation functions

On Fri, 29 Dec 2023 at 23:38, Richard Guo <guofenglinux@gmail.com> wrote:
> After applying this process to the first RestrictInfo, the bitmapset now
> becomes {t2, t3}.  Consequently, the second RestrictInfo also perceives
> {t2, t3} as its required_relids.  As a result, when attempting to remove
> it from the joininfo lists, a problem arises, because it is not in t2's
> joininfo list.

Changing the relids so they reference t2 instead of t1 requires both
bms_add_member() and bms_del_member().  The bms_add_member() will
cause the set to be reallocated with my patch so I don't see why this
case isn't covered.

> Also, here are some review comments for the v2 patch:
>
> * There is no reallocation that happens in bms_add_members(), do we need
> to call bms_copy_and_free() there?

The spec I put in the comment at the top of bitmapset.c says:

> have the code *always* reallocate the bitmapset when the
> * set *could* be reallocated as a result of the modification

Looking at bms_add_members(), it seems to me that the set *could* be
reallocated as a result of the modification, per:

if (a->nwords < b->nwords)
{
    result = bms_copy(b);
    other = a;
}

In my view, that meets the spec I outlined.

> * Do you think we can add Assert(bms_is_valid_set(a)) for bms_free()?

I did briefly have the Assert in bms_free(), but I removed it as I
didn't think it was that useful to validate the set before freeing it.
Certainly, there'd be an argument to do that, but I ended up not
putting it there. I wondered if there could be a case where we do
something like bms_int_members() which results in an empty set and
after checking for and finding the set is now empty, we call
bms_free().  If we did that then we'd Assert fail.  In reality, we use
pfree() instead of bms_free() as we already know the set is not NULL,
so it wouldn't cause a problem for that particular case. I wondered if
there might be another one that I didn't spot, so felt it was best not
to Assert there.

I imagine that if we found some case where the bms_free() Assert
failed, we'd likely just remove it rather than trying to make the set
valid before freeing it.  So it seems pretty pointless if we'd opt to
remove the Assert() at the first sign of trouble.