On 3/2/19 10:00 AM, Alexander Korotkov wrote:
> Hi!
>
> I'm starting to look at this patchset. In the general, I think it's
> very cool! We definitely need this.
>
> On Tue, Apr 3, 2018 at 10:51 PM Tomas Vondra
> <tomas.vondra@2ndquadrant.com> wrote:
>> 1) index parameters
>>
>> The main improvement of this version is an introduction of a couple of
>> BRIN index parameters, next to pages_per_range and autosummarize.
>>
>> a) n_distinct_per_range - used to size Bloom index
>> b) false_positive_rate - used to size Bloom index
>> c) values_per_range - number of values in the minmax-multi summary
>>
>> Until now those parameters were pretty much hard-coded, this allows easy
>> customization depending on the data set. There are some basic rules to
>> to clamp the values (e.g. not to allow ndistinct to be less than 128 or
>> more than MaxHeapTuplesPerPage * page_per_range), but that's about it.
>> I'm sure we could devise more elaborate heuristics (e.g. when building
>> index on an existing table, we could inspect table statistics first),
>> but the patch does not do that.
>>
>> One disadvantage is that those parameters are per-index.
>
> For me, the main disadvantage of this solution is that we put
> opclass-specific parameters into access method. And this is generally
> bad design. So, user can specify such parameter if even not using
> corresponding opclass, that may cause a confuse (if even we forbid
> that, it needs to be hardcoded). Also, extension opclasses can't do
> the same thing. Thus, it appears that extension opclasses are not
> first class citizens anymore. Have you take a look at opclass
> parameters patch [1]? I think it's proper solution of this problem.
> I think we should postpone this parameterization until we push opclass
> parameters patch.
>
> 1. https://www.postgresql.org/message-id/d22c3a18-31c7-1879-fc11-4c1ce2f5e5af%40postgrespro.ru
>
I've looked at that patch only very briefly so far, but I agree it's
likely a better solution than what my patch does at the moment (which I
agree is a misuse of the AM-level options). I'll take a closer look.
I agree it makes sense to re-use that infrastructure for this patch, but
I'm hesitant to rebase it on top of that patch right away. Because it
would mean this thread dependent on it, which would confuse cputube,
make it bitrot faster etc.
So I suggest we ignore this aspect of the patch for now, and let's talk
about the other bits first.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
