Hi Michael, Thanks for taking a look at this.
On Wed, Oct 23, 2019 at 12:51 PM Michael Paquier <mich...@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
BuildRelOptions-v2.patch
Description: Binary data
