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
