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)?
+/*
+ * 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.
+ /*
+ * 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? 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?
--
Michael