Date: Fri, 10 Jul 2020 14:09:14 +0200
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
Cc: Alexander Korotkov <a.korotkov@postgrespro.ru>,
	Michael Paquier <michael@paquier.xyz>,
	Alvaro Herrera <alvherre@2ndquadrant.com>,
	Tom Lane <tgl@sss.pgh.pa.us>, Andres Freund <andres@anarazel.de>,
	Mark Dilger <hornschnorter@gmail.com>,
	pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: WIP: BRIN multi-range indexes
Message-ID: <20200710120914.6kydpi3kfr2icpfq@development>
References: <20190926190148.piwdqnfc2brargqq@development>
 <20191201015502.GF2355@paquier.xyz>
 <20200402022904.xssjyfupdaj2ejyp@development>
 <CAPpHfdsaLSaWvmvVO-tz_t7PmeueWctCFq9URzYW1cReUdzKAA@mail.gmail.com>
 <20200405155317.rpmkqdj7far7mwkx@development>
 <CAPpHfduRd9sZy5M9NYwbvgSLMt4+X104+7J1pvUimq+w66n54A@mail.gmail.com>
 <20200405165954.m2odnthc7sbdctwg@development>
 <CAPpHfdtkV=fE-_9B-K_RZaxexksDwRUp6GvfiXRa0DcreRwtQg@mail.gmail.com>
 <20200703005737.dw5i2uwd6i3gv5lf@development>
 <CA+fd4k48FaU3SHJjOi2DDfvZAYtz4Yih=MF9zGcdPbn2SeRJdw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: 
 <CA+fd4k48FaU3SHJjOi2DDfvZAYtz4Yih=MF9zGcdPbn2SeRJdw@mail.gmail.com>
Precedence: bulk

On Fri, Jul 10, 2020 at 06:01:58PM +0900, Masahiko Sawada wrote:
>On Fri, 3 Jul 2020 at 09:58, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote:
>>
>> On Sun, Apr 05, 2020 at 08:01:50PM +0300, Alexander Korotkov wrote:
>> >On Sun, Apr 5, 2020 at 8:00 PM Tomas Vondra
>> ><tomas.vondra@2ndquadrant.com> wrote:
>> ...
>> >> >
>> >> >Assuming we're not going to get 0001-0003 into v13, I'm not so
>> >> >inclined to rush on these three as well.  But you're willing to commit
>> >> >them, you can count round of review on me.
>> >> >
>> >>
>> >> I have no intention to get 0001-0003 committed. I think those changes
>> >> are beneficial on their own, but the primary reason was to support the
>> >> new opclasses (which require those changes). And those parts are not
>> >> going to make it into v13 ...
>> >
>> >OK, no problem.
>> >Let's do this for v14.
>> >
>>
>> Hi Alexander,
>>
>> Are you still interested in reviewing those patches? I'll take a look at
>> 0001-0003 to check that your previous feedback was addressed. Do you
>> have any comments about 0004 / 0005, which I think are the more
>> interesting parts of this series?
>>
>>
>> Attached is a rebased version - I realized I forgot to include 0005 in
>> the last update, for some reason.
>>
>
>I've done a quick test with this patch set. I wonder if we can improve
>brin_page_items() SQL function in pageinspect as well. Currently,
>brin_page_items() is hard-coded to support only normal brin indexes.
>When we pass brin-bloom or brin-multi-range to that function the
>binary values are shown in 'value' column but it seems not helpful for
>users. For instance, here is an output of brin_page_items() with a
>brin-multi-range index:
>
>postgres(1:12801)=# select * from brin_page_items(get_raw_page('mul',
>2), 'mul');
>-[ RECORD 1 ]----------------------------------------------------------------------------------------------------------------------
>-----------------------------------------------------------------------------------------------------------------------------------
>----------------------------
>itemoffset  | 1
>blknum      | 0
>attnum      | 1
>allnulls    | f
>hasnulls    | f
>placeholder | f
>value       | {\x010000001b0000002000000001000000e5700000e6700000e7700000e8700000e9700000ea700000eb700000ec700000ed700000ee700000ef
>700000f0700000f1700000f2700000f3700000f4700000f5700000f6700000f7700000f8700000f9700000fa700000fb700000fc700000fd700000fe700000ff700
>00000710000}
>

Hmm. I'm not sure we can do much better, without making the function
much more complicated. I mean, even with regular BRIN indexes we don't
really know if the value is plain min/max, right?


>Also, I got an assertion failure when setting false_positive_rate reloption:
>
>postgres(1:12448)=# create index blm on t using brin (c int4_bloom_ops
>(false_positive_rate = 1));
>TRAP: FailedAssertion("(false_positive_rate > 0) &&
>(false_positive_rate < 1.0)", File: "brin_bloom.c", Line: 300)
>
>I'll look at the code in depth and let you know if I find a problem.
>

Yeah, the assert should say (f_p_r <= 1.0).

But I'm not convinced we should allow values up to 1.0, really. The
f_p_r is the fraction of the table that will get matched always, so 1.0
would mean we get to scan the whole table. Seems kinda pointless. So
maybe we should cap it to something like 0.1 or so, but I agree the
value seems kinda arbitrary.


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services