Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options - Mailing list pgsql-hackers

On Thu, Jun 10, 2021 at 8:55 AM Peter Smith <smithpb2250@gmail.com> wrote:
> > > 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.

Hm. I get it. Unfortunately the commit b1ff33f is missing information
on what the coverity tool was complaining of and it has no related
discussion at all.

I agree to remove that assertion entirely. I will post a new patch set soon.

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

Let's see how it goes further.

With Regards,
Bharath Rupireddy.