Content-Type: multipart/mixed; boundary="------------2q1mquCXfWNNc7P8gzeH0C6k"
Message-ID: <5540898f-706c-f399-2797-9e8fa58a5d81@gmail.com>
Date: Tue, 26 Sep 2023 10:00:00 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Subject: Re: BUG #17969: Assert failed in bloom_init() when
 false_positive_rate = 0.25
Content-Language: en-US
To: Tom Lane <tgl@sss.pgh.pa.us>, Tomas Vondra <tomas.vondra@enterprisedb.com>
Cc: pgsql-bugs@lists.postgresql.org, Masahiko Sawada <sawada.mshk@gmail.com>
References: <17969-a6c54de48026d694@postgresql.org>
 <836412.1686502986@sss.pgh.pa.us>
 <CAD21AoANsYnyHHSK543e1mFwuRBsVATs-Z0RnrE9thqaSsWZbA@mail.gmail.com>
 <2322240.1686882232@sss.pgh.pa.us>
From: Alexander Lakhin <exclusion@gmail.com>
In-Reply-To: <2322240.1686882232@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/5540898f-706c-f399-2797-9e8fa58a5d81%40gmail.com>
Precedence: bulk

This is a multi-part message in MIME format.
--------------2q1mquCXfWNNc7P8gzeH0C6k
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

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
--------------2q1mquCXfWNNc7P8gzeH0C6k
Content-Type: text/x-patch; charset=UTF-8;
 name="v1-01-fix-assert-in-brin-bloom.patch"
Content-Disposition: attachment;
 filename="v1-01-fix-assert-in-brin-bloom.patch"
Content-Transfer-Encoding: base64

ZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL2FjY2Vzcy9icmluL2JyaW5fYmxvb20uYyBiL3Ny
Yy9iYWNrZW5kL2FjY2Vzcy9icmluL2JyaW5fYmxvb20uYwppbmRleCA1NjUzNGNmMjljLi41
YmZhZmZlNTliIDEwMDY0NAotLS0gYS9zcmMvYmFja2VuZC9hY2Nlc3MvYnJpbi9icmluX2Js
b29tLmMKKysrIGIvc3JjL2JhY2tlbmQvYWNjZXNzL2JyaW4vYnJpbl9ibG9vbS5jCkBAIC0z
MjAsOCArMzIwLDcgQEAgYmxvb21faW5pdChpbnQgbmRpc3RpbmN0LCBkb3VibGUgZmFsc2Vf
cG9zaXRpdmVfcmF0ZSkKIAlpbnQJCQluaGFzaGVzOwkJLyogbnVtYmVyIG9mIGhhc2ggZnVu
Y3Rpb25zICovCiAKIAlBc3NlcnQobmRpc3RpbmN0ID4gMCk7Ci0JQXNzZXJ0KChmYWxzZV9w
b3NpdGl2ZV9yYXRlID49IEJMT09NX01JTl9GQUxTRV9QT1NJVElWRV9SQVRFKSAmJgotCQkg
ICAoZmFsc2VfcG9zaXRpdmVfcmF0ZSA8IEJMT09NX01BWF9GQUxTRV9QT1NJVElWRV9SQVRF
KSk7CisJQXNzZXJ0KGZhbHNlX3Bvc2l0aXZlX3JhdGUgPiAwICYmIGZhbHNlX3Bvc2l0aXZl
X3JhdGUgPCAxKTsKIAogCS8qIGNhbGN1bGF0ZSBibG9vbSBmaWx0ZXIgc2l6ZSAvIHBhcmFt
ZXRlcnMgKi8KIAlibG9vbV9maWx0ZXJfc2l6ZShuZGlzdGluY3QsIGZhbHNlX3Bvc2l0aXZl
X3JhdGUsCg==
--------------2q1mquCXfWNNc7P8gzeH0C6k
Content-Type: text/plain; charset=UTF-8;
 name="v1-01-fix-assert-in-brin-bloom.backpatch14-16"
Content-Disposition: attachment;
 filename="v1-01-fix-assert-in-brin-bloom.backpatch14-16"
Content-Transfer-Encoding: base64

ZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL2FjY2Vzcy9icmluL2JyaW5fYmxvb20uYyBiL3Ny
Yy9iYWNrZW5kL2FjY2Vzcy9icmluL2JyaW5fYmxvb20uYwppbmRleCBlNDk1M2E5ZDM3Li4x
ZWQwZDhhM2I4IDEwMDY0NAotLS0gYS9zcmMvYmFja2VuZC9hY2Nlc3MvYnJpbi9icmluX2Js
b29tLmMKKysrIGIvc3JjL2JhY2tlbmQvYWNjZXNzL2JyaW4vYnJpbl9ibG9vbS5jCkBAIC0y
NzksOCArMjc5LDcgQEAgYmxvb21faW5pdChpbnQgbmRpc3RpbmN0LCBkb3VibGUgZmFsc2Vf
cG9zaXRpdmVfcmF0ZSkKIAlkb3VibGUJCWs7CQkJCS8qIG51bWJlciBvZiBoYXNoIGZ1bmN0
aW9ucyAqLwogCiAJQXNzZXJ0KG5kaXN0aW5jdCA+IDApOwotCUFzc2VydCgoZmFsc2VfcG9z
aXRpdmVfcmF0ZSA+PSBCTE9PTV9NSU5fRkFMU0VfUE9TSVRJVkVfUkFURSkgJiYKLQkJICAg
KGZhbHNlX3Bvc2l0aXZlX3JhdGUgPCBCTE9PTV9NQVhfRkFMU0VfUE9TSVRJVkVfUkFURSkp
OworCUFzc2VydChmYWxzZV9wb3NpdGl2ZV9yYXRlID4gMCAmJiBmYWxzZV9wb3NpdGl2ZV9y
YXRlIDwgMSk7CiAKIAkvKiBzaXppbmcgYmxvb20gZmlsdGVyOiAtKG4gKiBsbihwKSkgLyAo
bG4oMikpXjIgKi8KIAluYml0cyA9IGNlaWwoLShuZGlzdGluY3QgKiBsb2coZmFsc2VfcG9z
aXRpdmVfcmF0ZSkpIC8gcG93KGxvZygyLjApLCAyKSk7Cg==

--------------2q1mquCXfWNNc7P8gzeH0C6k--