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

On Tue, Jun 29, 2021 at 8:56 PM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> On Tue, Jun 29, 2021 at 4:37 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > Few comments:
> > ===============
>
> > 2.
> > +parse_subscription_options(List *stmt_options, SubOpts *opts)
> > {
> > ListCell *lc;
> > - bool connect_given = false;
> > - bool create_slot_given = false;
> > - bool copy_data_given = false;
> > - bool refresh_given = false;
> > + bits32 supported_opts;
> > + bits32 specified_opts;
> >
> > - /* If connect is specified, the others also need to be. */
> > - Assert(!connect || (enabled && create_slot && copy_data));
> >
> > I am not sure whether removing this assertion will bring back the
> > coverity error for which it was added but I see that the reason for
> > which it was added still holds true. The same is explained by Michael
> > as well in his email [1]. I think it is better to keep an equivalent
> > assert.
>
> The coverity was complaining FORWARD_NULL which, I think, can occur
> with the pointers. In the patch, we don't deal with the pointers for
> the options but with the bitmaps. So, I don't think we need that
> assertion. However, we can look for the coverity warnings in the
> buildfarm after this patch gets in and fix if found any warnings.
>

I think irrespective of whether coverity reports or not, the assert
appears useful to me because we are still doing the check for the
other three options only if connect is supported.

--
With Regards,
Amit Kapila.