Re: [PATCH] Opclass parameters

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