parse_subscription_options - suggested improvements

This is a re-posting of my original mail from here [1]
Created this new discussion thread for it as suggested here [2]

[1] https://www.postgresql.org/message-id/CAHut%2BPtT0--Tf%3DK_YOmoyB3RtakUOYKeCs76aaOqO2Y%2BJt38kQ%40mail.gmail.com
[2] https://www.postgresql.org/message-id/CAA4eK1L0GT5-RJrya4q9ve%3DVi8hQM_SeHhJekmWMnOqsCh3KbQ%40mail.gmail.com

===========================================

On Tue, Jul 6, 2021 at 6:21 PM Amit Kapila
<amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Fri, Jul 2, 2021 at 12:36 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Fri, Jul 2, 2021 at 8:35 AM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> > >
> > > > The latest patch sent by Bharath looks good to me. Would you like to
> > > > commit it or shall I take care of it?
> > >
> > > Please, go ahead.
> > >
> >
> > Okay, I'll push it early next week (by Tuesday) unless there are more
> > comments or suggestions. Thanks!
> >
>
> Pushed!

Yesterday, I needed to refactor a lot of code due to this push [1].

The refactoring exercise caused me to study these v11 changes much more deeply.

IMO there are a few improvements that should be made:

//////

1. Zap 'opts' up-front

+ *
+ * Caller is expected to have cleared 'opts'.

This comment is putting the onus on the caller to "do the right thing".

I think that hopeful expectations about input should be removed - the
function should just be responsible itself just to zap the SubOpts
up-front. It makes the code more readable, and it removes any
potential risk of garbage unintentionally passed in that struct.

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

Alternatively, at least there should be an assertion for some sanity check.

Assert(opt->specified_opts == 0);

----

2. Remove redundant conditions

/* Check for incompatible options from the user. */
- if (enabled && *enabled_given && *enabled)
+ if (opts->enabled &&
+ IsSet(supported_opts, SUBOPT_ENABLED) &&
+ IsSet(opts->specified_opts, SUBOPT_ENABLED))
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
/*- translator: both %s are strings of the form "option = value" */
errmsg("%s and %s are mutually exclusive options",
"connect = false", "enabled = true")));

- if (create_slot && create_slot_given && *create_slot)
+ if (opts->create_slot &&
+ IsSet(supported_opts, SUBOPT_CREATE_SLOT) &&
+ IsSet(opts->specified_opts, SUBOPT_CREATE_SLOT))
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("%s and %s are mutually exclusive options",
"connect = false", "create_slot = true")));

- if (copy_data && copy_data_given && *copy_data)
+ if (opts->copy_data &&
+ IsSet(supported_opts, SUBOPT_COPY_DATA) &&
+ IsSet(opts->specified_opts, SUBOPT_COPY_DATA))
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("%s and %s are mutually exclusive options",
"connect = false", "copy_data = true")));

By definition, this function only allows any option to be
"specified_opts" if that option is also "supported_opts". So, there is
really no need in the above code to re-check again that it is
supported.

It can be simplified like this:

/* Check for incompatible options from the user. */
- if (enabled && *enabled_given && *enabled)
+ if (opts->enabled &&
+ IsSet(opts->specified_opts, SUBOPT_ENABLED))
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
/*- translator: both %s are strings of the form "option = value" */
errmsg("%s and %s are mutually exclusive options",
"connect = false", "enabled = true")));

- if (create_slot && create_slot_given && *create_slot)
+ if (opts->create_slot &&
+ IsSet(opts->specified_opts, SUBOPT_CREATE_SLOT))
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("%s and %s are mutually exclusive options",
"connect = false", "create_slot = true")));

- if (copy_data && copy_data_given && *copy_data)
+ if (opts->copy_data &&
+ IsSet(opts->specified_opts, SUBOPT_COPY_DATA))
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("%s and %s are mutually exclusive options",
"connect = false", "copy_data = true")));

-----

3. Remove redundant conditions

Same as 2. Here are more examples of conditions where the redundant
checking of "supported_opts" can be removed.

/*
* Do additional checking for disallowed combination when slot_name = NONE
* was used.
*/
- if (slot_name && *slot_name_given && !*slot_name)
+ if (!opts->slot_name &&
+ IsSet(supported_opts, SUBOPT_SLOT_NAME) &&
+ IsSet(opts->specified_opts, SUBOPT_SLOT_NAME))
{
- if (enabled && *enabled_given && *enabled)
+ if (opts->enabled &&
+ IsSet(supported_opts, SUBOPT_ENABLED) &&
+ IsSet(opts->specified_opts, SUBOPT_ENABLED))
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
/*- translator: both %s are strings of the form "option = value" */
errmsg("%s and %s are mutually exclusive options",
"slot_name = NONE", "enabled = true")));

- if (create_slot && create_slot_given && *create_slot)
+ if (opts->create_slot &&
+ IsSet(supported_opts, SUBOPT_CREATE_SLOT) &&
+ IsSet(opts->specified_opts, SUBOPT_CREATE_SLOT))
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
+ /*- translator: both %s are strings of the form "option = value" */
errmsg("%s and %s are mutually exclusive options",
"slot_name = NONE", "create_slot = true")));

It can be simplified like this:

/*
* Do additional checking for disallowed combination when slot_name = NONE
* was used.
*/
- if (slot_name && *slot_name_given && !*slot_name)
+ if (!opts->slot_name &&
+ IsSet(opts->specified_opts, SUBOPT_SLOT_NAME))
{
- if (enabled && *enabled_given && *enabled)
+ if (opts->enabled &&
+ IsSet(opts->specified_opts, SUBOPT_ENABLED))
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
/*- translator: both %s are strings of the form "option = value" */
errmsg("%s and %s are mutually exclusive options",
"slot_name = NONE", "enabled = true")));

- if (create_slot && create_slot_given && *create_slot)
+ if (opts->create_slot &&
+ IsSet(opts->specified_opts, SUBOPT_CREATE_SLOT))
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
+ /*- translator: both %s are strings of the form "option = value" */
errmsg("%s and %s are mutually exclusive options",
"slot_name = NONE", "create_slot = true")));

------

4. Remove redundant conditions

- if (enabled && !*enabled_given && *enabled)
+ if (opts->enabled &&
+ IsSet(supported_opts, SUBOPT_ENABLED) &&
+ !IsSet(opts->specified_opts, SUBOPT_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 (create_slot && !create_slot_given && *create_slot)
+ if (opts->create_slot &&
+ IsSet(supported_opts, SUBOPT_CREATE_SLOT) &&
+ !IsSet(opts->specified_opts, SUBOPT_CREATE_SLOT))
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", "create_slot = false")));

This code can be simplified even more than the others mentioned,
because here the "specified_opts" checks were already done in the code
that precedes this.

It can be simplified like this:

- if (enabled && !*enabled_given && *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")));

- if (create_slot && !create_slot_given && *create_slot)
+ if (opts->create_slot)
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", "create_slot = false")));

//////

PSA my patch which includes all the fixes mentioned above.

(Make check, and TAP subscription tests are tested and pass OK)

------
[1] https://github.com/postgres/postgres/commit/8aafb02616753f5c6c90bbc567636b73c0cbb9d4

Kind Regards,
Peter Smith.
Fujitsu Australia