Re: [PATCH] Opclass parameters

From: Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru>
To: Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Nikolay Shaplov <dhyan(at)nataraj(dot)su>, Oleg Bartunov <obartunov(at)gmail(dot)com>
Subject: Re: [PATCH] Opclass parameters
Date: 2019-09-10 01:30:41
Message-ID: dcb740af-2a93-8785-eb26-a4273029f2fb@postgrespro.ru
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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).


- 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.


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).

3. Use amattoptions() in contrib/bloom.

This is an attempt to replace "col1"-"col32" AM reloptions with
per-column option "bits" using new amattoptions API:

-CREATE INDEX bloomidx ON tst USING bloom (i, t) WITH (col1 = 3);
+CREATE INDEX bloomidx ON tst USING bloom (i DEFAULT (bits = 3), t);

4. An example of using new API in GiST tsvector_ops.

Opclass options definition looks much simpler now:

typedef struct GistTsVectorOptions
{
/* GistAttOptions am_att_options; (future) */
int siglen;
} GistTsVectorOptions;

Datum
gtsvector_options(PG_FUNCTION_ARGS)
{
local_relopts *relopts = (local_relopts *) PG_GETARG_POINTER(0);
GistTsVectorOptions *options = NULL;

extend_local_reloptions(relopts, options, sizeof(*options));
add_local_int_reloption(relopts, "siglen", "signature length",
SIGLEN_DEFAULT, 1, SIGLEN_MAX,
&options->siglen);

PG_RETURN_VOID();
}

5. Remove pg_index.indoption column (experimental).

pg_index.indoption (array of per-column ASC/DESC, NULLS FIRST/LAST
flags) can be removed now, and these flags can be stored as
special per-column AM-specific options "desc" and "nulls_first".

--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Attachment Content-Type Size
0001-Introduce-opclass-parameters-20190910.patch text/x-patch 83.2 KB
0002-Introduce-amattoptions-20190910.patch text/x-patch 7.1 KB
0003-Use-amattoptions-in-contrib-bloom-20190910.patch text/x-patch 8.3 KB
0004-Use-opclass-parameters-in-GiST-tsvector_ops-20190910.patch text/x-patch 29.1 KB
0005-Remove-pg_index.indoption-20190910.patch text/x-patch 39.9 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Amit Kapila 2019-09-10 02:21:08 Re: [HACKERS] [PATCH] pageinspect function to decode infomasks
Previous Message Tom Lane 2019-09-10 00:13:25 Re: Should we add xid_current() or a int8->xid cast?