Hi,
while rebasing the patch series [1] adding bloom/multi-minmax BRIN
opclasses, I've decided to also rebase it on top of this patch, because it
needs the opclass parameters. So I had to rebase this too - it went mostly
fine, with reasonably limited bitrot. The rebased patch series is attached.
Using this patch series in [1] was mostly smooth, I only have two minor
comments at this point:
1) We need a better infrastructure to parse opclass parameters. For
example the gtsvector_options does this:
Datum
gtsvector_options(PG_FUNCTION_ARGS)
{
Datum raw_options = PG_GETARG_DATUM(0);
bool validate = PG_GETARG_BOOL(1);
relopt_int siglen =
{ {"siglen", "signature length", 0, 0, 6, RELOPT_TYPE_INT },
SIGLEN_DEFAULT, 1, SIGLEN_MAX };
relopt_gen *optgen[] = { &siglen.gen };
int offsets[] = { offsetof(GistTsVectorOptions, siglen) };
GistTsVectorOptions *options =
parseAndFillLocalRelOptions(raw_options, optgen, offsets, 1,
sizeof(GistTsVectorOptions), validate);
PG_RETURN_POINTER(options);
}
So in other words, it builds all the various pieces (relopts, optgen,
offsets, lengths etc.) manually, which is really error-prone and difficult
to maintain. We need to make it simpler - ideally as simple as defining a
custom GUC, or just an array of relopt_* structs.
2) The 0001 part does this in index_opclass_options_generic:
get_opclass_name(opclass, InvalidOid, &str);
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("operator class \"%s\" has no options",
opclassname.data)));
But that's a bit broken, because get_opclass_name() appends the opclass
name to 'str', but with a space at the beginning. So this produces
messages like
ERROR: operator class " int4_bloom_ops" has no options
which is not right. I haven't checked if a better function already exists,
or whether we need to implement it.
regards
https://www.postgresql.org/message-id/flat/c1138ead-7668-f0e1-0638-c3be3237e812%402ndquadrant.com
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
