Re: WIP: BRIN multi-range indexes

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