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

On Wed, Jun 2, 2021 at 9:07 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> On Wed, Jun 2, 2021 at 12:55 AM Bharath Rupireddy
> <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
> >
> > On Mon, May 24, 2021 at 7:04 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> > > Please see a3dc926 and the surrounding discussion for reasons why we've
> > > been using bitmaps for option parsing lately.
> >
> > Thanks for the suggestion. Here's a WIP patch implementing the
> > subscription command options as bitmaps similar to what commit a3dc926
> > did. Thoughts?
>
> I took a look at this latest WIP patch.

Thanks.

> The patch applied cleanly.
> The code builds OK.
> The make check result is OK.
> The TAP subscription make check result is OK.

Thanks for testing.

> Below are some minor review comments:
>
> ------
>
> +typedef struct SubOptVals
> +{
> + bool connect;
> + bool enabled;
> + bool create_slot;
> + char *slot_name;
> + bool copy_data;
> + char *synchronous_commit;
> + bool refresh;
> + bool binary;
> + bool streaming;
> +} SubOptVals;
> +
> +/* options for CREATE/ALTER SUBSCRIPTION */
> +typedef struct SubOpts
> +{
> + bits32 supported_opts; /* bitmask of supported SUBOPT_* */
> + bits32 specified_opts; /* bitmask of user specified SUBOPT_* */
> + SubOptVals vals;
> +} SubOpts;
> +
>
> 1. These seem only used by the subscriptioncmds.c file, so should they
> be declared in there also instead of in the .h?

Agreed.

> 2. I don't see what was gained by having the SubOptVals as a separate
> struct; OTOH the code accessing the vals is more verbose because of
> it. Maybe consider combining everything into SubOpts and then can just
> access "opts.copy_data" (etc) instead of "opts.vals.copy_data";

Agreed.

> + /* 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. 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?

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?

With Regards,
Bharath Rupireddy.