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
0001-Introduce-opclass-parameters-20200318.patch.gz
Description: application/gzip
0002-Use-opclass-parameters-in-GiST-tsvector_ops-20200318.patch.gz
Description: application/gzip
0003-Use-opclass-parameters-in-contrib_intarray-20200318.patch.gz
Description: application/gzip
0004-Use-opclass-parameters-in-contrib_ltree-20200318.patch.gz
Description: application/gzip
0005-Use-opclass-parameters-in-contrib_pg_trgm-20200318.patch.gz
Description: application/gzip
0006-Use-opclass-parameters-in-contrib_hstore-20200318.patch.gz
Description: application/gzip
0007-Introduce-amattoptions-20200318.patch.gz
Description: application/gzip
0008-Use-amattoptions-in-contrib_bloom-20200318.patch.gz
Description: application/gzip
0009-Remove-pg_index.indoption-20200318.patch.gz
Description: application/gzip