Re: List of Bitmapset (was Re: ExecRTCheckPerms() and many prunable partitions)

On Mon, Nov 14, 2022 at 11:57 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Amit Langote <amitlangote09(at)gmail(dot)com> writes:
> > On Sat, Nov 12, 2022 at 1:46 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> The main thing I was wondering about in connection with that
> >> was whether to assume that there could be other future applications
> >> of the logic to perform multi-bitmapset union, intersection,
> >> etc. If so, then I'd be inclined to choose different naming and
> >> put those functions in or near to bitmapset.c. It doesn't look
> >> like Amit's code needs anything like that, but maybe somebody
> >> has an idea about other applications?
>
> > Yes, simple storage of multiple Bitmapsets in a List somewhere in a
> > parse/plan tree sounded like that would have wider enough use to add
> > proper node support for. Assuming you mean trying to generalize
> > VarAttnoSet in your patch 0004 posted at [2], I wonder if you want to
> > somehow make its indexability by varno / RT index a part of the
> > interface of the generic code you're thinking for it?
>
> For discussion's sake, here's my current version of that 0004 patch,
> rewritten to use list-of-bitmapset as the data structure. (This
> could actually be pulled out of the outer-join-vars patchset and
> committed independently, just as a minor performance improvement.
> It doesn't quite apply cleanly to HEAD, but pretty close.)
>
> As it stands, the new functions are still in util/clauses.c, but
> if we think they could be of general use it'd make sense to move them
> either to nodes/bitmapset.c or to some new file under backend/nodes.

These multi_bms_* functions sound generic enough to me, so +1 to put
them in nodes/bitmapset.c. Or even a new file if the API should
involve a dedicated struct enveloping the List as you write below.

> Some other thoughts:
>
> * The multi_bms prefix is a bit wordy, so I was thinking of shortening
> the function names to mbms_xxx. Maybe that's too brief.

FWIW, multi_bms_* naming sounds fine to me.

> * This is a pretty short list of functions so far. I'm not eager
> to build out a bunch of dead code though. Is it OK to leave it
> with just this much functionality until someone needs more?

+1

> * I'm a little hesitant about whether the API actually should be
> List-of-Bitmapset, or some dedicated struct as I had in the previous
> version of 0004. This version is way less invasive in prepjointree.c
> than that was, but the reason is there's ambiguity about what the
> forced_null_vars Lists actually contain, which feels error-prone.

Are you thinking of something like a MultiBitmapset that wraps the
multi_bms List? That sounds fine to me. Another option is to make
the generic API be List-of-Bitmapset but keep VarAttnoSet in
prepjointree.c and put the List in it. IMHO, VarAttnoSet is
definitely more self-documenting for that patch's purposes.

+ * The new member is identified by the zero-based index of the List
+ * element it should go into, and the bit number to be set therein.
+ */
+List *
+multi_bms_add_member(List *mbms, int index1, int index2)

The comment sounds a bit ambiguous, especially the ", and the bit
number to be set therein." part. If you meant to describe the
arguments, how about mentioning their names too, as in:

The new member is identified by 'index1', the zero-based index of the
List element it should go into, and 'index2' specifies the bit number
to be set therein.

+ /* Add empty elements to a, as needed */
+ while (list_length(a) < list_length(b))
+ a = lappend(a, NULL);
+ /* forboth will stop at the end of the shorter list, which is fine */

Isn't this comment unnecessary given that the while loop makes both
lists be the same length?

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com