Re: BUG #17969: Assert failed in bloom_init() when false_positive_rate = 0.25

Hi Alexander,

This change makes sense to me. But I have a question here, because we
already have a limitation in rel option level and that will make sure the
parameter is in 0.0001 - 0.25 range, this CHECK seems like a little
redundant for me especially it does not match the constraint in rel option.
Maybe I missed some context here, but could we just remove this CHECK?

Thanks,
Shihao

On Mon, Oct 23, 2023 at 4:53 PM Alexander Lakhin <exclusion(at)gmail(dot)com>
wrote:

> 16.06.2023 05:23, Tom Lane wrote:
> > Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> writes:
> >> It seems that the minimum false positive rate also doesn't work:
> >> postgres(1:3419179)=# create table t (a int);
> >> CREATE TABLE
> >> postgres(1:3419179)=# create index t_idx on t using brin (a
> >> int4_bloom_ops (false_positive_rate = 0.0001));
> >> CREATE INDEX
> >> postgres(1:3419179)=# insert into t values (1);
> >> 2023-06-16 11:14:01.349 JST [3419179] ERROR: the bloom filter is too
> >> large (8924 > 8144)
> >> 2023-06-16 11:14:01.349 JST [3419179] STATEMENT: insert into t values
> (1);
> >> ERROR: the bloom filter is too large (8924 > 8144)
> > Hmm, but it would work with a larger page size, right? If so
> > I'm disinclined to move the minimum. What I don't like about
> > the above is that the failure doesn't occur until you actually
> > insert an index entry. Is there a way to check for this at
> > CREATE INDEX time?
>
> With a larger page size, say, 32 kB, you'll get the similar error:
> ERROR: the bloom filter is too large (35856 > 32720)
>
> but, if you define also a smaller n_distinct_per_range value explicitly.
> e. g.:
> CREATE INDEX idx ON tbl USING brin (i int4_bloom_ops(false_positive_rate =
> 0.00010, n_distinct_per_range = 100));
> false_positive_rate = 0.00010 will do.
>
> So I see an issue only with false_positive_rate.
> And I think the assertion that false_positive_rate is a valid probability
> would be good.
> The patch is attached.
>
> Tomas, I saw that you refactored that place (introduced
> bloom_filter_size())
> in 2b8b2852b, could you also consider checking for invalid parameter values
> at CREATE INDEX time, as Tom suggested above?
>
> Best regards,
> Alexander