Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options - Mailing list pgsql-hackers

On Fri, May 21, 2021 at 9:21 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Thu, May 20, 2021 at 2:11 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > On Wed, May 19, 2021 at 6:13 PM Bharath Rupireddy
> > <bharath.rupireddyforpostgres@gmail.com> wrote:
> > > Thanks. I will work on the new structure ParseSubOption only for
> > > subscription options.
> >
> > PSA v2 patch that has changes for 1) new ParseSubOption structure 2)
> > the error reporting code refactoring.
> >
>
> I have applied the v2 patch and done some review of the code.
>
> - The patch applies OK.
>
> - The code builds OK.
>
> - The make check and TAP subscription tests are OK
>
>
> I am not really a big fan of this patch - it claims to make things
> easier for future options, but IMO the changes sometimes seem at the
> expense of readability of the *current* code. The following comments
> are only posted here, not as endorsement, but because I already
> reviewed the code so they may be of some use in case the patch goes
> ahead...
>
> COMMENTS
> ==========
>
> parse_subscription_options:
>
> 1.
> I felt the function implementation is less readable now than
> previously due to the plethora of "opts->" introduced everywhere.
> Maybe it would be worthwhile to assign all those opts back to local
> vars (of the same name as the original previous 14 args), just for the
> sake of getting rid of all those "opts->"?
>
> ----------
>
> 2.
> (not the fault of this patch) Inside the parse_subscription_options
> function, there seem many unstated assertions that if a particular
> option member opts->XXX is passed, then the opts->XXX_given is also
> present (although that is never checked). Perhaps the code should
> explicitly Assert those XXX_given vars?
>
> ----------
>
> 3.
> @@ -225,65 +238,63 @@ parse_subscription_options(List *options,
>   * We've been explicitly asked to not connect, that requires some
>   * additional processing.
>   */
> - if (connect && !*connect)
> + if (opts->connect && !*opts->connect)
>   {
> + char *option = NULL;
>
> "option" seems too generic. Maybe "incompatible_option" would be a
> better name for that variable?
>
> ----------
>
> 4.
> - errmsg("%s and %s are mutually exclusive options",
> - "slot_name = NONE", "create_slot = true")));
> + option = NULL;
>
> - if (enabled && !*enabled_given && *enabled)
> - ereport(ERROR,
> - (errcode(ERRCODE_SYNTAX_ERROR),
> - /*- translator: both %s are strings of the form "option = value" */
> - errmsg("subscription with %s must also set %s",
> - "slot_name = NONE", "enabled = false")));
> + if (opts->enabled && !*opts->enabled_given && *opts->enabled)
> + option = "enabled = false";
> + else if (opts->create_slot && !create_slot_given && *opts->create_slot)
> + option = "create_slot = false";
>
>
> In the above code you don't need to set option = NULL, because it must
> already be NULL. But for this 2nd chunk of code I think it would be
> better to introduce another variable called something like
> "required_option".
>
> ==========
>
> Create Subscription:
>
> 5.
> @@ -346,22 +357,32 @@ CreateSubscription(CreateSubscriptionStmt *stmt,
> bool isTopLevel)
>   char originname[NAMEDATALEN];
>   bool create_slot;
>   List    *publications;
> + ParseSubOptions *opts;
> +
> + opts = (ParseSubOptions *) palloc0(sizeof(ParseSubOptions));
> +
> + /* Fill only the options that are of interest here. */
> + opts->stmt_options = stmt->options;
> + opts->connect = &connect;
>
>
> I feel that this code ought to be using a stack variable instead of
> allocating on the heap because - less code, easier to read, no free
> required. etc.
>
> Just memset it to fill all 0s before assigning the values.
>
> ----------
>
> 6.
> + /* Fill only the options that are of interest here. */
>
> The comment is kind of redundant, and what you are setting are not
> really all options either.
>
> Maybe better like this? Or maybe don't have the comment at all?
>
> /* Assign only members of interest. */
> MemSet(&opts, 0, sizeof(opts));
> opts.stmt_options = stmt->options;
> opts.connect = &connect;
> opts.enabled_given = &enabled_given;
> opts.enabled = &enabled;
> opts.create_slot = &create_slot;
> ...
>
> ==========
>
> AlterSubscription
>
> 7.
> Same review comment as for CreateSubscription.
> - Use a stack variable and memset.
> - Change or remove the comment.
>
> ----------
>
> 8.
> For AlterSubscriotion you could also declare the "opts" just time only
> and memset it at top of the function, instead of the current code
> which repeats 5X the same thing.
>
> ----------
>
> 9.
> + /* For DROP PUBLICATION, copy_data option is not supported. */
> + opts->copy_data = isadd ? ©_data : NULL;
>
> The opts struct is already zapped 0/NULL so this code maybe should be:
>
> if (isadd)
> opts.copy_data = ©_data;
>
> ==========
>
> 10.
> Since the new typedef ParseSubOptions was added by this patch
> shouldn't the src/tools/pgindent/typedefs.list file be updated also?
>

Thinking about this some more, a few other things occurred to me which
might help simplify the code.

==========

11.

+/*
+ * Structure to hold subscription options for parsing
+ */
+typedef struct ParseSubOptions
+{
+ List *stmt_options;
+ bool *connect;
+ bool *enabled_given;
+ bool *enabled;
+ bool *create_slot;
+ bool *slot_name_given;
+ char **slot_name;
+ bool *copy_data;
+ char **synchronous_commit;
+ bool *refresh;
+ bool *binary_given;
+ bool *binary;
+ bool *streaming_given;
+ bool *streaming;
+} ParseSubOptions;

Maybe I am mistaken, but I am wondering why all this indirection is
even necessary anymore?

IIUC previously args were declared like bool * so the information
could be returned to the caller. But now you have ParseSubOption * to
do that, so can't you simply declare all those "given" members as bool
instead of bool *; And when you do that you can remove all the
unnecessary storage vars in the calling code as well.

--------

12.
The original code seems to have a way to "register interest in the
option" by passing the storage var in which to return the result. (eg.
pass "enabled" not NULL).

IIUC the purpose of this is what it says in the function comment
function comment:

 * Since not all options can be specified in both commands, this function
 * will report an error on options if the target output pointer is NULL to
 * accommodate that.

But now that you have your new struct, I wonder if there is another
easy way to achieve the same. e.g. you now could add some more members
instead of the non-NULL pointer to register interest in a particular
option. Then those other members (like "enabled") also only need to be
bool instead of bool *;

Something like this?

BEFORE:
else if (strcmp(defel->defname, "enabled") == 0 && opts->enabled)
{
if (*opts->enabled_given)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("conflicting or redundant options")));

*opts->enabled_given = true;
*opts->enabled = defGetBoolean(defel);
}
AFTER:
if (opts->enabled_is_allowed && strcmp(defel->defname, "enabled") == 0)
{
if (opts->enabled_given)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("conflicting or redundant options")));

opts->enabled_given = true;
opts->enabled = defGetBoolean(defel);
}

I am unsure if this will lead to better code or not; Anyway, it is
something to consider - maybe you can experiment with it to see.

----------

13.
Regardless of review comment #12, I think all those strcmp conditions
ought to be reversed for better efficiency.

e.g.
BEFORE:
else if (strcmp(defel->defname, "binary") == 0 && opts->binary)
AFTER:
else if (opts->binary && strcmp(defel->defname, "binary") == 0)

----------
Kind Regards,
Peter Smith.
Fujitsu Australia.