Re: WIP: BRIN multi-range indexes

On Sun, Apr 05, 2020 at 07:33:40PM +0300, Alexander Korotkov wrote:
>On Sun, Apr 5, 2020 at 6:53 PM Tomas Vondra
><tomas.vondra@2ndquadrant.com> wrote:
>> On Sun, Apr 05, 2020 at 06:29:15PM +0300, Alexander Korotkov wrote:
>> >On Thu, Apr 2, 2020 at 5:29 AM Tomas Vondra
>> ><tomas.vondra@2ndquadrant.com> wrote:
>> >> On Sun, Dec 01, 2019 at 10:55:02AM +0900, Michael Paquier wrote:
>> >> >On Thu, Sep 26, 2019 at 09:01:48PM +0200, Tomas Vondra wrote:
>> >> >> Yeah, the opclass params patches got broken by 773df883e adding enum
>> >> >> reloptions. The breakage is somewhat extensive so I'll leave it up to
>> >> >> Nikita to fix it in [1]. Until that happens, apply the patches on
>> >> >> top of caba97a9d9 for review.
>> >> >
>> >> >This has been close to two months now, so I have the patch as RwF.
>> >> >Feel free to update if you think that's incorrect.
>> >>
>> >> I see the opclass parameters patch got committed a couple days ago, so
>> >> I've rebased the patch series on top of it. The pach was marked RwF
>> >> since 2019-11, so I'll add it to the next CF.
>> >
>> >I think this patchset was marked RwF mainly because slow progress on
>> >opclass parameters.  Now we got opclass parameters committed, and I
>> >think this patchset is in a pretty good shape.  Moreover, opclass
>> >parameters patch comes with very small examples.  This patchset would
>> >be great showcase for opclass parameters.
>> >
>> >I'd like to give this patchset a chance for v13.  I'm going to make
>> >another pass trough this patchset.  If I wouldn't find serious issues,
>> >I'm going to commit it.  Any objections?
>> >
>>
>> I'm an author of the patchset and I'd love to see it committed, but I
>> think that might be a bit too rushed and unfair (considering it was not
>> included in the current CF at all).
>>
>> I think the code is correct and I'm not aware of any bugs, but I'm not
>> sure there was sufficient discussion about things like costing, choosing
>> parameter values (e.g.  number of values in the multi-minmax or bloom
>> filter parameters).
>
>Ok!
>
>> That being said, I think the first couple of patches (that modify how
>> BRIN deals with multi-key scans and IS NULL clauses) are simple enough
>> and non-controversial, so maybe we could get 0001-0003 committed, and
>> leave the bloom/multi-minmax opclasses for v14.
>
>Regarding 0001-0003 I've couple of notes:
>1) They should revise BRIN extensibility documentation section.
>2) I think 0002 and 0003 should be merged.  NULL ScanKeys should be
>still passed to consistent function when oi_regular_nulls == false.
>
>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 ...

regards

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