From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Nikita Glukhov <n.gluhov@postgrespro.ru>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Nikolay Shaplov <dhyan@nataraj.su>
Cc: Oleg Bartunov <obartunov@gmail.com>
Subject: Re: [PATCH] Opclass parameters
Date: Wed, 11 Sep 2019 00:03:58 +0200
Message-ID: <20190910220358.6tj37k4v6vyaqac7@development> (raw)
In-Reply-To: <dcb740af-2a93-8785-eb26-a4273029f2fb@postgrespro.ru>
References: <20190903220254.GA22389@alvherre.pgsql>
<dcb740af-2a93-8785-eb26-a4273029f2fb@postgrespro.ru>
On Tue, Sep 10, 2019 at 04:30:41AM +0300, Nikita Glukhov wrote:
>On 04.09.2019 1:02, Alvaro Herrera wrote:
>
>>On 2019-Jun-11, Tomas Vondra wrote:
>>
>>>1) We need a better infrastructure to parse opclass parameters. For
>>>example the gtsvector_options does this:
>>I think this is part of what Nikolay's patch series was supposed to
>>address. But that one has been going way too slow. I agree we need
>>something better.
>
>API was simplified in the new version of the patches (see below).
>
>>>2) The 0001 part does this in index_opclass_options_generic:
>>>
>>> get_opclass_name(opclass, InvalidOid, &str);
>>>
>>> ereport(ERROR,
>>> (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>>> errmsg("operator class \"%s\" has no options",
>>> opclassname.data)));
>>>
>>>But that's a bit broken, because get_opclass_name() appends the opclass
>>>name to 'str', but with a space at the beginning.
>>Yeah, I think just exporting get_opclass_name from ruleutils.c is a bad
>>idea. Sounds like we need a (very small) new function in lsyscache.c
>>that does the job of extracting the opclass name, and then the ruleutils
>>function can call that one to avoid duplicated code.
>
>I decided to add new function generate_opclass_name() like existing
>generate_collation_name(), and to reuse static get_opclass_name().
>
>>Anyway, this patchset doesn't apply anymore. Somebody (maybe its
>>author this time?) please rebase.
>
>New version of rebased patches is attached:
>
>1. Opclass parameters infrastructure.
>
> API was completely refactored since the previous version:
>
> - API was generalized for all AMs. Previously, each AM should implement
> opclass options parsing/passing in its own way using its own support
> function numbers.
> Now each AMs uses 0th support function (discussable). Binary bytea values
> of parsed options are passed to support functions using special expression
> node initialized in FmgrInfo.fn_expr (see macro PG_GET_OPCLASS_OPTIONS(),
> get_fn_opclass_options(), set_fn_opclass_options).
>
I very much doubt these changes are in the right direction. Firstly, using
0 as procnum is weird - AFAICS you picked 0 because after moving it from
individual AMs to pg_amproc.h it's hard to guarantee the procnum does not
clash with other am procs. But ISTM that's more a hint that the move to
pg_amproc.h itself was a bad idea. I suggest we undo that move, instead of
trying to fix the symptoms. That is, each AM should have a custom procnum.
Also, looking at fn_expr definition in FmgrInfo, I see this
fmNodePtr fn_expr; /* expression parse tree for call, or NULL */
it seems like a rather bad idea to reuse that to pass options when it's
clearly not meant for that purpose.
> - Introduced new structure local_relopts (needs a better name, of course)
> with a set of functions for opclass/AM options definition. The parsing
> was moved into index_opclass_option(). That was necessary for mixing of
> AM- and opclass-specific options. Opclasses now extend the structure with
> AM's options adding their own options. See patch #4 for an example.
>
OK. No opinion on this change yet.
>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?
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tomas.vondra@2ndquadrant.com, n.gluhov@postgrespro.ru, alvherre@2ndquadrant.com, robertmhaas@gmail.com, tgl@sss.pgh.pa.us, dhyan@nataraj.su, obartunov@gmail.com
Subject: Re: [PATCH] Opclass parameters
In-Reply-To: <20190910220358.6tj37k4v6vyaqac7@development>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox