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.
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().
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.
Patches 0002 and 0003 should be merged, I put 0003 in a separate patch just
for ease of review.
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()
Attached patches with all my changes.
--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
