From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Nikita Glukhov <n.gluhov@postgrespro.ru>
To: Alexander Korotkov <a.korotkov@postgrespro.ru>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Andres Freund <andres@anarazel.de>
Cc: Mark Dilger <hornschnorter@gmail.com>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: WIP: BRIN multi-range indexes
Date: Tue, 12 Mar 2019 18:14:53 +0100
Message-ID: <90be37f9-07ea-0f90-4bc6-6bdb68f4f190@2ndquadrant.com> (raw)
In-Reply-To: <49cb668f-d6f9-3493-681d-7d40b715ef64@postgrespro.ru>
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>
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tomas.vondra@2ndquadrant.com, n.gluhov@postgrespro.ru, a.korotkov@postgrespro.ru, tgl@sss.pgh.pa.us, andres@anarazel.de, hornschnorter@gmail.com, alvherre@alvh.no-ip.org
Subject: Re: WIP: BRIN multi-range indexes
In-Reply-To: <90be37f9-07ea-0f90-4bc6-6bdb68f4f190@2ndquadrant.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox