Subject: Re: WIP: BRIN multi-range indexes
To: Nikita Glukhov <n.gluhov@postgrespro.ru>,
 Alexander Korotkov <a.korotkov@postgrespro.ru>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Andres Freund <andres@anarazel.de>,
 Mark Dilger <hornschnorter@gmail.com>,
 Alvaro Herrera <alvherre@alvh.no-ip.org>,
 pgsql-hackers <pgsql-hackers@postgresql.org>
References: <20180123200537.gfx5hxcp5k5hktpe@alvherre.pgsql>
 <821c4c1d-592e-8916-e323-04607b11777e@2ndquadrant.com>
 <c32f9f46-e0ca-0c02-45ec-718dd381f4be@2ndquadrant.com>
 <104E8BB8-35FF-4AA8-9871-6AFF04E4CAF3@gmail.com>
 <0afde2fc-e160-004f-8fe2-c65302ac3760@2ndquadrant.com>
 <22674.1517874015@sss.pgh.pa.us>
 <3e924a4c-6ca0-cc10-aebb-5004fc002800@2ndquadrant.com>
 <c2523129-a970-97f2-4705-a3b65e21b32f@2ndquadrant.com>
 <20180302035538.gkqgtcv6kmn2cklw@alap3.anarazel.de>
 <18526.1519963686@sss.pgh.pa.us>
 <1c1c1ced-1f79-a0ee-e5cd-858ca3eb9d74@2ndquadrant.com>
 <CAPpHfdsUSu+tEo6Kaj_iXvrhzhGOZ2pkYEn_aWj9b4M2vHGEhg@mail.gmail.com>
 <acfc8aa3-1cbf-b9de-77ba-556ddd84a094@2ndquadrant.com>
 <CAPpHfduD_30yZ4H8CE9BHHedci-qUpuhX941mkV0jR7Nd3AJWA@mail.gmail.com>
 <49cb668f-d6f9-3493-681d-7d40b715ef64@postgrespro.ru>
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Message-ID: <90be37f9-07ea-0f90-4bc6-6bdb68f4f190@2ndquadrant.com>
Date: Tue, 12 Mar 2019 18:14:53 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.5.1
MIME-Version: 1.0
In-Reply-To: <49cb668f-d6f9-3493-681d-7d40b715ef64@postgrespro.ru>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


Hi Nikita,

Thanks for looking at the patch.

On 3/12/19 11:33 AM, Nikita Glukhov wrote:
> Hi!
> 
> I have looked at this patch set too, but so far only at first two 
> infrastructure patches.
> 
> First of all, I agree that opclass parameters patch is needed here.
> 

OK.

> 
> 0001. Pass all keys to BRIN consistent function at once.
> 
> I think that changing the signature of consistent function is bad, because then
> the authors of existing BRIN opclasses will need to maintain two variants of
> the function for different version of PosgreSQL.  Moreover, we can easily
> distinguish two variants by the number of parameters.  So I returned back a
> call to old 3-argument variant of consistent() in bringetbitmap().  Also I
> fixed brinvalidate() adding support for new 4-argument variant, and fixed
> catalog entries for brin_minmax_consistent() and brin_inclusion_consistent()
> which remained 3-argument.  And also I removed unneeded indentation shift in
> these two functions, which makes it difficult to compare changes, by extracting
> subroutines minmax_consistent_key() and inclusion_consistent_key().
> 

Hmmm. I admit I rather dislike functions that change the signature based
on the number of arguments, for some reason. But maybe it's better than
changing the consistent function. Not sure.

> 
> 0002. Move IS NOT NULL checks to bringetbitmap()
> 
> I believe that removing duplicate code is always good.  But in this case it
> seems a bit inconsistent to refactor only bringetbitmap().  I think we can't
> guarantee that existing opclasses work with null flags in add_value() and
> union() in the expected way.
> 
> So I refactored the work with BrinValues flags in other places in patch 0003.
> I added flag BrinOpcInfp.oi_regular_nulls which enables regular processing of
> NULLs before calling of support functions.  Now support functions don't need to
> care about bv_hasnulls at all. add_value(), for example, works now only with
> non-NULL values.
> 

That seems like unnecessary complexity to me. We can't really guarantee
much about opclasses in extensions anyway. I don't know if there's some
sort of precedent but IMHO it's reasonable to expect the opclasses to be
updated accordingly.

> Patches 0002 and 0003 should be merged, I put 0003 in a separate patch just 
> for ease of review.
> 

Thanks.

> 
> 0004. BRIN bloom indexes
> 0005. BRIN multi-range minmax indexes
> 
> I have not looked carefully at these packs yet, but fixed only catalog entries
> and removed NULLs processing according to patch 0003.  I also noticed that the
> following functions contain a lot of duplicated code, which needs to be
> extracted into common subroutine:
> inclusion_get_procinfo()
> bloom_get_procinfo()
> minmax_multi_get_procinfo()
> 

Yes. The reason for the duplicate code is that initially this was
submitted as two separate patches, so there was no obvious need for
sharing code.

regards

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