Re: [PATCH] Opclass parameters

From: Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru>
To: Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru>
Cc: Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, 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: 2020-03-16 15:22:50
Message-ID: CAPpHfduqgQCnFEqP=Ef2p8c3ZcNNrc5gZ+p8iVmO3HSmzj4jkw@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi!

I took a look on this patchset. There is a first set of questions.

* Patchset badly needs comments. I've to literally reverse engineer
to get what's going on. But I still don't understand many things.

* I'm curious about what local_relopts.base field means.

void
extend_local_reloptions(local_relopts *opts, void *base, Size base_size)
{
Assert(opts->base_size < base_size);
opts->base = base;
opts->base_size = base_size;
}

/*
* add_local_reloption
* Add an already-created custom reloption to the local list.
*/
static void
add_local_reloption(local_relopts *relopts, relopt_gen *newoption, void *pval)
{
local_relopt *opt = palloc(sizeof(*opt));

opt->option = newoption;
opt->offset = (char *) pval - (char *) relopts->base;

relopts->options = lappend(relopts->options, opt);
}

Datum
ghstore_options(PG_FUNCTION_ARGS)
{
local_relopts *relopts = (local_relopts *) PG_GETARG_POINTER(0);
GistHstoreOptions *options = NULL;

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

PG_RETURN_VOID();
}

It's not commented, but I guess it's used to calculate offsets from
pointers passed to add_local_*_reloption(). Is it better to just pass
offsets to add_local_*_reloption()?

* It's generally unclear how does amattoptions and opclass options
interact. As I get we now don't have an example where both
amattoptions and opclass options involved. What is general benefit
from putting both two kind of options into single bytea? Can opclass
options method do something useful with amattoptions? For instance,
some amattoptions can be calculated from opclass options? That would
be some point for putting these options together, but it doesn't look
like opclass options method can do this?

* It current opclass code safe for introduction new atattoptions. For
instace, would ghstore_*() work the same way expecting
GistHstoreOptions struct to be passed as opclass options if gist would
introduce own attoptions? I guess not. If I'm wrong, please clarify
this. And patchset needs comment one could get this without guessing.

------
Alexander Korotkov

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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Justin Pryzby 2020-03-16 15:41:36 Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
Previous Message Fabien COELHO 2020-03-16 15:20:21 Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)