Re: [PATCH] Do not use StdRdOptions in Access Methods

Hi Michael,

Thanks for taking a look at this.

On Wed, Oct 23, 2019 at 12:51 PM Michael Paquier <michael@paquier.xyz> wrote:
> On Wed, Oct 23, 2019 at 11:16:25AM +0900, Amit Langote wrote:
> > IMO, parts of the patch that only refactors the existing code should
> > be first in the list as it is easier to review, especially if it adds
> > no new concepts.  In this case, your patch to break StdRdOptions into
> > more manageable chunks would be easier to understand if it built upon
> > a simplified framework of parsing reloptions text arrays.
>
> Thanks for doing a split.  This helps in proving the point that this
> portion has independent value.
>
> s/BuildRelOptions/buildRelOptions/ for consistency with the other
> routines (see first character's case-ing)?

Hmm, if we're inventing a new API to replace the old one, why not use
that opportunity to be consistent with our general style, which
predominantly seems to be either words_separated_by_underscore() or
UpperCamelCase().  Thoughts?

> +/*
> + * Using parseRelOptions(), allocateReloptStruct(), and fillRelOptions()
> + * directly is Deprecated; use BuildRelOptions() instead.
> + */
>  extern relopt_value *parseRelOptions(Datum options, bool validate,
> Compatibility is surely a concern for existing extensions, but that's
> not the first interface related to reloptions that we'd break in this
> release (/me whistles).  So my take would be to move all the past
> routines to be static and only within reloptions.c, and just publish
> the new one.  That's by far not the most popular API we provide.

OK, done.

> +   /*
> +    * Allocate and fill the struct.  Caller-specified struct size and the
> +    * relopt_parse_elt table (relopt_elems + num_relopt_elems) must match.
> +    */
> The comment should be about a multiplication, no?

I didn't really mean to specify any mathematical operation by the "+"
in that comment, but I can see how it's confusing. :)

>  It seems to me that
> an assertion would be appropriate here then to insist on the
> relationship between all that, and also it would be nice to document
> better what's expected from the caller of the new routine regarding
> all the arguments needed.  In short, what's expected of
> relopt_struct_size, relopt_elems and num_relopt_elems?

You might know already, but in short, the values in the passed-in
relopt_parse_elts array (relopt_elems) must fit within
relopt_struct_size.  Writing an Assert turned out to be tricky given
that alignment must be considered, but I have tried to add one.  Pleas
check, it very well might be wrong. ;)

Attached updated patch.  It would be nice to hear whether this patch
is really what Nikolay intended to eventually do with this code.

Thanks,
Amit