Re: parse_subscription_options - suggested improvements

On 8/8/21, 11:54 PM, "Peter Smith" <smithpb2250@gmail.com> wrote:
> v11 -> v12
>
> * A rebase was needed due to some recent pgindent changes on HEAD.

The patch looks correct to me.  I have a couple of small comments.

+    /* Start out with cleared opts. */
+    memset(opts, 0, sizeof(SubOpts));

Should we stop initializing opts in the callers?

-        if (opts->enabled &&
-            IsSet(supported_opts, SUBOPT_ENABLED) &&
-            !IsSet(opts->specified_opts, SUBOPT_ENABLED))
+        if (opts->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")));

IMO the way these 'if' statements are written isn't super readable.
Right now, it's written like this:

        if (opt && IsSet(someopt))
                ereport(ERROR, ...);

        if (otheropt && IsSet(someotheropt))
                ereport(ERROR, ...);

        if (opt)
                ereport(ERROR, ...);

        if (otheropt)
                ereport(ERROR, ...);

I think it would be easier to understand if it was written more like
this:

        if (opt)
        {
                if (IsSet(someopt))
                        ereport(ERROR, ...);
                else
                        ereport(ERROR, ...);
        }

        if (otheropt)
        {
                if (IsSet(someotheropt))
                        ereport(ERROR, ...);
                else
                        ereport(ERROR, ...);
        }

Of course, this does result in a small behavior change because the
order of the checks is different, but I'm not sure that's a big deal.
Ultimately, it would probably be nice to report all the errors instead
of just the first one that is hit, but again, I don't know if that's
worth the effort.

I attached a new version of the patch with my suggestions.  However, I
think v12 is committable.

Nathan