On Tue, Mar 26, 2024 at 02:16:03PM -0400, Tom Lane wrote:
> I did a little experimentation using the attached quick-hack C
> function, and came to the conclusion that setting up the bloom filter
> costs more or less as much as inserting 1000 or so OIDs the dumb way.
> So we definitely want a threshold that's not much less than that.
Thanks for doing this.
> So I'm now content with choosing a threshold of 1000 or 1024 or so.
Cool.
> As for the bloom filter size, I see that bloom_create does
>
> bitset_bytes = Min(bloom_work_mem * UINT64CONST(1024), total_elems * 2);
> bitset_bytes = Max(1024 * 1024, bitset_bytes);
>
> which means that any total_elems input less than 512K is disregarded
> altogether. So I'm not sold on your "ROLES_LIST_BLOOM_THRESHOLD * 10"
> value. Maybe it doesn't matter though.
Yeah, I wasn't sure how much to worry about this. I figured that we might
as well set it to a reasonable estimate based on the description of the
parameter. This description claims that the filter should work well if
this is off by a factor of 5 or more, and 50x the threshold sounded like it
ought to be good enough for anyone, so that's how I landed on 10x. But as
you point out, this value will be disregarded altogether, and it will
continue to be ignored unless the filter implementation changes, which
seems unlikely. If you have a different value in mind that you would
rather use, I'm fine with changing it.
> I do not like, even a little bit, your use of a static variable to
> hold the bloom filter pointer. That code will misbehave horribly
> if we throw an error partway through the role-accumulation loop;
> the next call will try to carry on using the old filter, which would
> be wrong even if it still existed which it likely won't. It's not
> that much worse notationally to keep it as a local variable, as I
> did in the attached.
Ah, yes, that's no good. I fixed this in the new version.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com