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