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
