Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options

On Wed, Jun 2, 2021 at 3:33 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> > + /* If connect option is supported, the others also need to be. */
> > + Assert((supported_opts & SUBOPT_CONNECT) == 0 ||
> > +    ((supported_opts & SUBOPT_ENABLED) != 0 &&
> > + (supported_opts & SUBOPT_CREATE_SLOT) != 0 &&
> > + (supported_opts & SUBOPT_COPY_DATA) != 0));
> > +
> > + /* Set default values for the supported options. */
> > + if ((supported_opts & SUBOPT_CONNECT) != 0)
> > + vals->connect = true;
> > +
> > + if ((supported_opts & SUBOPT_ENABLED) != 0)
> > + vals->enabled = true;
> > +
> > + if ((supported_opts & SUBOPT_CREATE_SLOT) != 0)
> > + vals->create_slot = true;
> > +
> > + if ((supported_opts & SUBOPT_SLOT_NAME) != 0)
> > + vals->slot_name = NULL;
> > +
> > + if ((supported_opts & SUBOPT_COPY_DATA) != 0)
> > + vals->copy_data = true;
> >
> > 3. Are all those "!= 0" really necessary when checking the
> > supported_opts against the bit masks? Maybe it is just a style thing,
> > but since there are so many of them I felt it contributed to clutter
> > and made the code less readable. This pattern was in many places, not
> > just the example above.
>
> Yeah these are necessary to know whether a particular option's bit is
> set in the bitmask.

Hmmm. Maybe I did not ask the question properly. See below.

> How about having a macro like below:
> #define IsSet(val, option)  ((val & option) != 0)
> The if statements can become like below:
> if (IsSet(supported_opts, SUBOPT_CONNECT))
> if (IsSet(supported_opts, SUBOPT_ENABLED))
> if (IsSet(supported_opts, SUBOPT_SLOT_NAME))
> if (IsSet(supported_opts, SUBOPT_COPY_DATA))
>
> The above looks better to me. Thoughts?

Yes, it looks better, but (since the masks are all 1 bit) I was only
asking why not do like:

if (supported_opts & SUBOPT_CONNECT)
if (supported_opts & SUBOPT_ENABLED)
if (supported_opts & SUBOPT_SLOT_NAME)
if (supported_opts & SUBOPT_COPY_DATA)

>
> Can we implement a similar idea for the parse_publication_options
> although there are only two options right now. Option parsing code
> will be consistent for logical replication DDLs and is extensible.
> Thoughts?

I have no strong opinion about it. It seems a trade off between having
a goal of "code consistency", versus "if it aint broke don't fix it".

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