Date: Wed, 11 Sep 2019 00:14:01 +0200
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Nikita Glukhov <n.gluhov@postgrespro.ru>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>,
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>,
	Robert Haas <robertmhaas@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>,
	Nikolay Shaplov <dhyan@nataraj.su>,
	Oleg Bartunov <obartunov@gmail.com>
Subject: Re: [PATCH] Opclass parameters
Message-ID: <20190910221401.ebw6kgsb4ziqtqmg@development>
References: <20190903220254.GA22389@alvherre.pgsql>
 <dcb740af-2a93-8785-eb26-a4273029f2fb@postgrespro.ru>
 <20190910220358.6tj37k4v6vyaqac7@development>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1; format=flowed
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20190910220358.6tj37k4v6vyaqac7@development>
Precedence: bulk

On Wed, Sep 11, 2019 at 12:03:58AM +0200, Tomas Vondra wrote:
>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.
>

BTW, is there a place where we actually verify the signature of the new am
proc? Because I only see code like this:

+    case OPCLASS_OPTIONS_PROC:
+        ok = true;
+        break;

in all "validate" functions.


regards

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