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.von...@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.von...@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
