Re: parse_subscription_options - suggested improvements

On Fri, Dec 3, 2021 at 11:02 AM Bossart, Nathan <bossartn@amazon.com> wrote:
>
> 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.

Thank you for taking an interest in my patch and moving it to a
"Ready" state in the CF.

>
> +       /* Start out with cleared opts. */
> +       memset(opts, 0, sizeof(SubOpts));
>
> Should we stop initializing opts in the callers?

For the initialization of opts I put memset within the function to
make it explicit that the bit-masks will work as intended without
having to look back at calling code for the initial values. In any
case, I think the caller declarations of SubOpts are trivial, (e.g.
SubOpts opts = {0};) so I felt caller initializations don't need to be
changed regardless of the memset.

>
> -               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 behaviour 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.
>

My patch was meant only to remove all the redundant conditions of the
HEAD code, so I did not rearrange any of the logic at all. Personally,
I also think your v13 is better and easier to read, but those subtle
behaviour differences were something I'd deliberately avoided in v12.
However, if the committer thinks it does not matter then your v13 is
fine by me.

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

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