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
