Re: Revise the Asserts added to bimapset manipulation functions

On Fri, Dec 29, 2023 at 5:22 PM David Rowley <dgrowleyml(at)gmail(dot)com> wrote:

> On Fri, 29 Dec 2023 at 21:07, Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
> > It seems to me that the former scenario also makes sense in some cases.
> > For instance, let's say there are two pointers in two structs, s1->p and
> > s2->p, both referencing the same bitmapset. If we modify the bitmapset
> > via s1->p (even if no reallocation could occur), s2 would see different
> > content via its pointer 'p'.
>
> That statement simply isn't true. If there's no reallocation then
> both pointers point to the same memory and, therefore have the same
> content, not different content. In the absence of a reallocation,
> then the only time s1->p and s2->p could differ is if s1->p became an
> empty set as a result of the modification.

Sorry I didn't make myself clear. By "different content via s2->p" I
mean different content than what came before, not than s1->p. There was
issue caused by such modifications reported before in [1]. In that
case, the problematic query is

explain (costs off)
select * from t t1
inner join t t2 on t1.a = t2.a
left join t t3 on t1.b > 1 and t1.b < 2;

The 'required_relids' in the two RestrictInfos for 't1.b > 1' and 't1.b
< 2' both reference the same bitmapset. The content of this bitmapset
is {t1, t3}. However, as we have decided to remove the t1/t2 join by
eliminating t1, we need to substitute the Vars of t1 with the Vars of
t2. To achieve this, we remove each of the two RestrictInfos from the
joininfo lists it is in and perform the necessary replacements.

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.

Just to clarify, I am not objecting to your v2 patch. I just want to
make sure what is our purpose in introducing REALLOCATE_BITMAPSETS. I'd
like to confirm whether our intention aligns with the former scenario or
the latter one that I mentioned upthread.

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?

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

[1]
https://www.postgresql.org/message-id/flat/CAMbWs4_wJthNtYBL%2BSsebpgF-5L2r5zFFk6xYbS0A78GKOTFHw%40mail.gmail.com

Thanks
Richard