Re: parse_subscription_options - suggested improvements

On 8/8/21, 11:54 PM, "Peter Smith" <smithpb2250(at)gmail(dot)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