Re: [PATCH] Opclass parameters

On Wed, Sep 11, 2019 at 01:44:28AM +0300, Nikita Glukhov wrote:
>On 11.09.2019 1:03, Tomas Vondra wrote:
>
>>On Tue, Sep 10, 2019 at 04:30:41AM +0300, Nikita Glukhov wrote:
>>
>>>2. New AM method amattoptions().
>>>
>>>  amattoptions() is used to specify per-column AM-specific options.
>>>  The example is signature length for bloom indexes (patch #3).
>>>
>>
>>I'm somewhat confused how am I supposed to use this, considering the
>>patch
>>set only defines this for the contrib/bloom index AM. So let's say I want
>>to create a custom BRIN opclass with per-attribute options (like the two
>>BRIN opclasses I work on in the other thread). Clearly, I can't tweak the
>>IndexAmRoutine from the extension. ISTM the patch series should
>>modify all
>>existing index AMs to have a valid amattoptions() implementation, calling
>>the new amproc if defined.
>>
>>Or what is the correct way to define custom opclass for existing index AM
>>(e.g. BRIN) with attribute options?
>>
>Per-attribute opclass options are implemented independently from per-attribute
>AM options. amattoptions() is optional and needs to be defined only if AM has
>per-attribute options.

OK, thanks for the explanation - so the per-attribute opclass options will
work even when the AM does not have amattoptions() defined. Is there any
practical reason why not to just define everything as opclass options and
get rid of the amattoptions() entirely?

> amproc #0 is called regardless of whether amattoptions
>is defined or not. That was the main reason why uniform procnum 0 was
>picked.
>

I still think using procnum 0 and passing the data through fn_expr are not
the right solution. Firstly, traditionally the amprocs are either required
or optional, with required procs having low procnums and optional starting
at 11 or so. The 0 breaks this, because it's optional but it contradicts
the procnum rule. Also, what happens if we need to add another optional
amproc defined for all AMs? Surely we won't use -1.

IMHO we should keep AM-specific procnum and pass it somehow to the AM
machinery.

FWIW there seems to be a bug in identify_opfamily_groups() which does
this:

/* Ignore strategy numbers outside supported range */
if (oprform->amopstrategy > 0 && oprform->amopstrategy < 64)
thisgroup->operatorset |= ((uint64) 1) << oprform->amopstrategy;

but then identify_opfamily_groups() computes allfuncs without any such
restriction, i.e. it includes procnum 0. And then it fails on this check

if (thisgroup->functionset != allfuncs) {...}

None of the built-in brin opclasses defines the new amproc, so the code
does not hit this issue. I only noticed this with the opclasses added in
the other thread.

As for the fn_expr, I still think this seems like a misuse of a field
which was intended for something else. I wonder if it might be breaking
some exising code - either in code or in some extension. It seems quite
possible.

It just seems like we're inventing a new way to novel way to pass data
into a function while we already have parameters for that purpose. Adding
parameters may require care so as not to break existing opclasses, but it
seems like the right approach.

>You should simply define function like that and use it as amproc #0:
>
>Datum
>brin_bloom_options(PG_FUNCTION_ARGS)
>{
> local_relopts *relopts = (local_relopts *) PG_GETARG_POINTER(0);
> BloomOptions *blopts = NULL;
>
> extend_local_reloptions(relopts, blopts, sizeof(*blopts));
>
> add_local_real_reloption(relopts, "n_distinct_per_range", "desc",
> -0.1, -1.0, INT_MAX, &blopts->nDistinctPerRange);
>
> add_local_real_reloption(relopts, "false_positive_rate", "desc",
> 0.01, 0.001, 1.0, &blopts->falsePositiveRate);
>
> PG_RETURN_VOID();
>}
>

OK, this did the trick. Thanks. I don't have a clear opinion on the API,
but it certainly looks like an improvement.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services