From: Alexander Lakhin <exclusion@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
To: Tomas Vondra <tomas.vondra@enterprisedb.com>
Cc: pgsql-bugs@lists.postgresql.org, Masahiko Sawada <sawada.mshk@gmail.com>
Subject: Re: BUG #17969: Assert failed in bloom_init() when false_positive_rate = 0.25
Date: Tue, 26 Sep 2023 10:00:00 +0300
Message-ID: <5540898f-706c-f399-2797-9e8fa58a5d81@gmail.com> (raw)
In-Reply-To: <2322240.1686882232@sss.pgh.pa.us>
References: <17969-a6c54de48026d694@postgresql.org>
<836412.1686502986@sss.pgh.pa.us>
<CAD21AoANsYnyHHSK543e1mFwuRBsVATs-Z0RnrE9thqaSsWZbA@mail.gmail.com>
<2322240.1686882232@sss.pgh.pa.us>
16.06.2023 05:23, Tom Lane wrote:
> Masahiko Sawada <sawada.mshk@gmail.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
diff --git a/src/backend/access/brin/brin_bloom.c b/src/backend/access/brin/brin_bloom.cindex e4953a9d37..1ed0d8a3b8 100644--- a/src/backend/access/brin/brin_bloom.c+++ b/src/backend/access/brin/brin_bloom.c@@ -279,8 +279,7 @@ bloom_init(int ndistinct, double false_positive_rate)
double k; /* number of hash functions */
Assert(ndistinct > 0);
- Assert((false_positive_rate >= BLOOM_MIN_FALSE_POSITIVE_RATE) &&- (false_positive_rate < BLOOM_MAX_FALSE_POSITIVE_RATE));+ Assert(false_positive_rate > 0 && false_positive_rate < 1);
/* sizing bloom filter: -(n * ln(p)) / (ln(2))^2 */
nbits = ceil(-(ndistinct * log(false_positive_rate)) / pow(log(2.0), 2));
Attachments:
[text/x-patch] v1-01-fix-assert-in-brin-bloom.patch (655B, ../5540898f-706c-f399-2797-9e8fa58a5d81@gmail.com/2-v1-01-fix-assert-in-brin-bloom.patch)
download | inline diff:diff --git a/src/backend/access/brin/brin_bloom.c b/src/backend/access/brin/brin_bloom.cindex 56534cf29c..5bfaffe59b 100644--- a/src/backend/access/brin/brin_bloom.c+++ b/src/backend/access/brin/brin_bloom.c@@ -320,8 +320,7 @@ bloom_init(int ndistinct, double false_positive_rate)
int nhashes; /* number of hash functions */
Assert(ndistinct > 0);
- Assert((false_positive_rate >= BLOOM_MIN_FALSE_POSITIVE_RATE) &&- (false_positive_rate < BLOOM_MAX_FALSE_POSITIVE_RATE));+ Assert(false_positive_rate > 0 && false_positive_rate < 1);
/* calculate bloom filter size / parameters */
bloom_filter_size(ndistinct, false_positive_rate,
[text/plain] v1-01-fix-assert-in-brin-bloom.backpatch14-16 (682B, ../5540898f-706c-f399-2797-9e8fa58a5d81@gmail.com/3-v1-01-fix-assert-in-brin-bloom.backpatch14-16)
download | inline diff:diff --git a/src/backend/access/brin/brin_bloom.c b/src/backend/access/brin/brin_bloom.cindex e4953a9d37..1ed0d8a3b8 100644--- a/src/backend/access/brin/brin_bloom.c+++ b/src/backend/access/brin/brin_bloom.c@@ -279,8 +279,7 @@ bloom_init(int ndistinct, double false_positive_rate)
double k; /* number of hash functions */
Assert(ndistinct > 0);
- Assert((false_positive_rate >= BLOOM_MIN_FALSE_POSITIVE_RATE) &&- (false_positive_rate < BLOOM_MAX_FALSE_POSITIVE_RATE));+ Assert(false_positive_rate > 0 && false_positive_rate < 1);
/* sizing bloom filter: -(n * ln(p)) / (ln(2))^2 */
nbits = ceil(-(ndistinct * log(false_positive_rate)) / pow(log(2.0), 2));
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: exclusion@gmail.com, tgl@sss.pgh.pa.us, tomas.vondra@enterprisedb.com, sawada.mshk@gmail.com
Subject: Re: BUG #17969: Assert failed in bloom_init() when false_positive_rate = 0.25
In-Reply-To: <5540898f-706c-f399-2797-9e8fa58a5d81@gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox