Re: [PATCH] Opclass parameters - Mailing list pgsql-hackers
From | Tomas Vondra |
---|---|
Subject | Re: [PATCH] Opclass parameters |
Date | |
Msg-id | 20190910220358.6tj37k4v6vyaqac7@development Whole thread Raw |
In response to | Re: [PATCH] Opclass parameters (Nikita Glukhov <n.gluhov@postgrespro.ru>) |
Responses |
Re: [PATCH] Opclass parameters
Re: [PATCH] Opclass parameters |
List | pgsql-hackers |
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
pgsql-hackers by date: