Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options - Mailing list pgsql-hackers

On Tue, Jul 6, 2021 at 6:21 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Jul 2, 2021 at 12:36 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Fri, Jul 2, 2021 at 8:35 AM Alvaro Herrera <alvherre@alvh.no-ip.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