Re: WIP: BRIN multi-range indexes - Mailing list pgsql-hackers

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