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

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
Cc: Amit Langote <amitlangote09(at)gmail(dot)com>, Richard Guo <guofenglinux(at)gmail(dot)com>, David Rowley <dgrowleyml(at)gmail(dot)com>, Greg Stark <stark(at)mit(dot)edu>, Julien Rouhaud <rjuju123(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: List of Bitmapset (was Re: ExecRTCheckPerms() and many prunable partitions)
Date: 2022-11-15 18:23:27
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> writes:
> On 2022-Nov-14, Tom Lane wrote:
>> For discussion's sake, here's my current version of that 0004 patch,
>> rewritten to use list-of-bitmapset as the data structure.

> I feel that there should be more commentary that explains what a
> multi-bms is. Not sure where, maybe just put it near the function that
> first appears in the file.

Right. I split the new functions out to new files multibitmapset.h/.c,
so that the file headers can carry the overall explanation. (I duplicated
the text between .h and .c, which is also true of bitmapset.h/.c, but
maybe that's overkill.)

>> * 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.

> I don't think the "ulti_" bytes add a lot, and short names are better.

Yeah, after sleeping on it I like mbms.

>> * 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?

> I agree with not adding dead code.

I concluded that the only thing that makes this an odd set of functions
to start out with is the lack of mbms_is_member; it seems asymmetric
to have mbms_add_member but not mbms_is_member. So I added that.
I'm content to let the rest grow out as needed.

>> * 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.

> Hmm ... if somebody makes a mistake, does the functionality break in
> obvious ways, or is it very hard to pinpoint what happened?

Now that Bitmapset is a full-fledged Node type, we can make use of
castNode checks to verify that the input Lists contain what we expect.
That seems probably sufficient to catch coding errors.

>> +multi_bms_add_member(List *mbms, int index1, int index2)

> Maybe s/index1/listidx/ or bitmapidx and s/index2/bitnr/ ?

Right. I used listidx and bitidx.

regards, tom lane

Attachment Content-Type Size
invent-multibitmapsets-v1.patch text/x-diff 14.4 KB

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Peter Geoghegan 2022-11-15 18:24:05 Standardizing how pg_waldump presents recovery conflict XID cutoffs
Previous Message David Christensen 2022-11-15 17:51:42 Re: [PATCH] Teach pg_waldump to extract FPIs from the WAL