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

On Thu, Jun 10, 2021 at 1:28 AM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> On Wed, Jun 9, 2021 at 10:37 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> >
[...]

I checked the v4* patches.
Everything applies and builds and tests OK for me.

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

This misses the point of my question. And deducing the meaning of the
"the others" from the code is completely backwards! The comment
describes the code. The code doesn't describe the comment.

Again, I was asking why “the others” are only these 3 options?. What
about binary? What about streaming? What about refresh?
In other words - what was the *intent* of that comment, and does the
new code still meet the requirements of that intent? I think it does
not.

If you see github [1] when that code was first implemented you can
see that “the others” referred to every option other than the
“connect”. At that time, the only others were those 3 - enabled,
create_slot, copy_data. But now there are lots more options so
something definitely needs to change.
E.g.
- Maybe the Assert now needs to include all the new options as well?
- Maybe the entire reason for the Assert has become redundant now due
to the introduction of SubOpts. It looks like it was not functional
code - just something to quieten a static analysis tool.
- Certainly “the others” is too vague and no longer has the original
meaning anymore

I don't know the answer; my guess is that all this has become obsolete
and the whole Assert and the dodgy comment can just be deleted.

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

Thanks for splitting them. My votes are +1 for patch 0001 and -1 for
patch 0002. As you say, someone else can decide.

------
[1] https://github.com/postgres/postgres/commit/b1ff33fd9bb82937f4719f264972e6a3c83cdb89#

Kind Regards,
Peter Smith
Fujitsu Australia.