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

On Wed, Jun 30, 2021 at 11:11 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Jun 29, 2021 at 8:56 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > On Tue, Jun 29, 2021 at 4:37 PM Amit Kapila <amit.kapila16@gmail.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.

Added the assert back. PSA v9 patch set posted upthread.

With Regards,
Bharath Rupireddy.