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

On Wed, Jun 2, 2021 at 10:41 PM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> On Wed, Jun 2, 2021 at 11:43 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> > 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)
>
> Please review the attached v3 patch further.

OK. I have applied the v3 patch and reviewed it again:

- It applies OK.
- The code builds OK.
- The make check and TAP subscription tests are OK

========

1.
+/*
+ * Structure to hold the bitmaps and values of all the options for
+ * CREATE/ALTER SUBSCRIPTION commands.
+ */

There seems to be an extra space before "commands."

------

2.
+ /* If connect option is supported, the others also need to be. */
+ Assert(!IsSet(supported_opts, SUBOPT_CONNECT) ||
+ (IsSet(supported_opts, SUBOPT_ENABLED) &&
+ IsSet(supported_opts, SUBOPT_CREATE_SLOT) &&
+ IsSet(supported_opts, SUBOPT_COPY_DATA)));

This comment about "the others" doesn’t make sense to me.

e.g. Why only these 3 options? What about all those other SUBOPT_* options?

------

3.
I feel that this patch should be split into 2 parts
a) the SubOpts changes, and
b) the mutually exclusive options change.

I agree that the new SubOpts struct etc. is an improvement over existing code.

But, for the mutually exclusive options part I don't see what is
gained by the new patch code. I preferred the old code with its
multiple ereports. Although it was a bit repetitive IMO it was easier
to read that way, and length-wise there is almost no difference. So if
it is less readable and not a lot shorter then what is the benefit of
the change?

------

4.
- char *slotname;
- bool slotname_given;
- char *synchronous_commit;
- bool binary_given;
- bool binary;
- bool streaming_given;
- bool streaming;
-
- parse_subscription_options(stmt->options,
- NULL, /* no "connect" */
- NULL, NULL, /* no "enabled" */
- NULL, /* no "create_slot" */
- &slotname_given, &slotname,
- NULL, /* no "copy_data" */
- &synchronous_commit,
- NULL, /* no "refresh" */
- &binary_given, &binary,
- &streaming_given, &streaming);
-
- if (slotname_given)
+ SubOpts opts = {0};

I feel it would be simpler to declare/init this "opts" variable just 1
time at top of the function AlterSubscription, instead of the 6
separate declarations in this v3 patch. Doing that can allow other
code simplifications too. (see #5)

------

5.
case ALTER_SUBSCRIPTION_DROP_PUBLICATION:
{
bool isadd = stmt->kind == ALTER_SUBSCRIPTION_ADD_PUBLICATION;
- bool copy_data;
- bool refresh;
List *publist;
+ SubOpts opts = {0};
+
+ opts.supported_opts |= SUBOPT_REFRESH;
+
+ if (isadd)
+ opts.supported_opts |= SUBOPT_COPY_DATA;

I think having a separate "isadd" variable is made moot now since
adding the SubOpts struct.

Instead you can do this:
+ if (stmt->kind == ALTER_SUBSCRIPTION_ADD_PUBLICATION)
+ opts.supported_opts |= SUBOPT_COPY_DATA;

OR (after #4) you could do this:

case ALTER_SUBSCRIPTION_ADD_PUBLICATION:
opts.supported_opts |= SUBOPT_COPY_DATA;
/* fall thru. */
case ALTER_SUBSCRIPTION_DROP_PUBLICATION:

------

6.
+
+#define IsSet(val, option) ((val & option) != 0)
+

Your IsSet macro might be better if changed to test *multiple* bits are all set.

Like this:
#define IsSet(val, bits) ((val & (bits)) == (bits))

~

Most of the code remains the same, but some can be simplified.
e.g.
+ /* If connect option is supported, the others also need to be. */
+ Assert(!IsSet(supported_opts, SUBOPT_CONNECT) ||
+ (IsSet(supported_opts, SUBOPT_ENABLED) &&
+ IsSet(supported_opts, SUBOPT_CREATE_SLOT) &&
+ IsSet(supported_opts, SUBOPT_COPY_DATA)));

Becomes:
Assert(!IsSet(supported_opts, SUBOPT_CONNECT) ||
IsSet(supported_opts, SUBOPT_ENABLED|SUBOPT_CREATE_SLOT|SUBOPT_COPY_DATA));

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