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

On Wed, Jun 9, 2021 at 10:37 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Wed, Jun 2, 2021 at 10:41 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > On Wed, Jun 2, 2021 at 11:43 AM Peter Smith <smithpb2250@gmail.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

Thanks.

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

Removed.

> 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?

It is an existing Assert and comment for ensuring somebody doesn't
call parse_subscription_options with SUBOPT_CONNECT, without
SUBOPT_ENABLED, SUBOPT_CREATE_SLOT and SUBOPT_COPY_DATA. In other
words, when SUBOPT_CONNECT is passed in, the other three options
should also be passed. " the others" there in the comment makes sense
just by looking at the Assert statement.

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

Divided the patch into two.

> 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?

I personally don't like the repeated code when there's a chance of
doing it better. It might not reduce the loc, but it removes the many
similar ereport(ERROR calls. PSA v4-0002 patch. I think the committer
can take a call on it.

> 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)

Done.

> 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:

Done.

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

Changed.

PSA v4 patch set.

With Regards,
Bharath Rupireddy.