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