Re: [PATCH] Opclass parameters

Attached new version of reordered patches.

Questionable patches for AM-specific per-attribute options were moved to
the end, so they can be skipped now.

On 16.03.2020 18:22, Alexander Korotkov wrote:

> 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()?

Yes, 'base' field was used to calculate offsets. Now I started to pass offsets
instead of pointers to the fields of template structure (that gave us
additional type checking). Some comments were added.

> * 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?

There are no examples for AM and opclass options interaction now. But AM and
opclass can register custom callbacks that will be called after parsing in
their registration order. In these callbacks it is possible to post-process
option values, check presence or absence of some options.

The main benefit of putting both option into single bytea is that it does not
require major modifications of reloption processing code. And it also does
not require to split reloption list obtained from SQL into two separate lists
for AM and opclass options.

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

Yes, the code will be broken after introduction of GiST per-attribute options.
GistHstoreOptions should include GistAttOptions which simply did not exist in
the previous version of the patches. I added empty XxxAttOptions for all AMs
in patch #7, and GistAttOptions and GinAttOptions now are included into
corresponding structures for opclass options.

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