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