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

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.

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

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?

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";

------

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

------

Kind Regards,
Peter Smith.
Fujitsu Australia