Subject: Re: WIP: BRIN multi-range indexes
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: John Naylor <john.naylor@enterprisedb.com>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
References: <20210302234414.GA30716@alvherre.pgsql>
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
Message-ID: <4e92be8a-446b-1bab-d43d-8bd55e62e96e@enterprisedb.com>
Date: Wed, 3 Mar 2021 00:53:22 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.7.0
MIME-Version: 1.0
In-Reply-To: <20210302234414.GA30716@alvherre.pgsql>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 3/3/21 12:44 AM, Alvaro Herrera wrote:
> On 2021-Mar-03, Tomas Vondra wrote:
> 
>> 1) The 0001 patch allows passing of all scan keys to BRIN opclasses,
>> which is needed for the minmax-multi to work. But it also modifies the
>> existing opclasses (minmax and inclusion) to do this - but for those
>> opclasses it does not make much difference, I think. Initially this was
>> done because the patch did this for all opclasses, but then we added the
>> detection based on number of parameters. So I wonder if we should just
>> remove this, to make the patch a bit smaller. It'll also test the other
>> code path (for opclasses without the last parameter).
> 
> I think it makes sense to just do them all in one pass.  I think trying
> to keep the old way working just because it's how it was working
> previously does not have much benefit.  I don't think we care about the
> *patch* being small as much as the resulting *code* being as simple as
> possible (but no simpler).
> 

That's kinda my point - I agree the size of the patch is not the primary
concern, but it makes the minmax/inclusion code a bit more complicated
(because they now have to loop over the keys), with very little benefit
(there might be some speedup, but IMO it's rather negligible).

Alternatively we could simply remove the code supporting the old API
with "consistent" functions without the additional parameter. But the
idea was to seamlessly support existing opclasses / not breaking them
unnecessarily (I know we don't guarantee that in major upgrades, but as
they may not benefit from this, why break them?). It'd simplify the code
in brin.c a little bit, but the opclasses a bit more complex.

> 
>> 2) This needs bsearch_arg, but we only have that defined/used in
>> extended_statistics_internal.h - it seems silly to include that here, as
>> this has nothing to do with extended stats, so I simply added another
>> prototype (which gets linked correctly). But, I suppose a better way
>> would be to define our "portable" variant pg_bsearch_arg, next to
>> pg_qsort etc.
> 
> Yeah, I think it makes sense to move bsearch_arg to a place where it's
> more generally accesible (src/port/bsearch_arg.c I suppose), and make
> both places use that.
> 

OK, will do.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company