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

On Wed, May 19, 2021 at 4:42 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Wed, May 19, 2021 at 4:10 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Wed, May 19, 2021 at 3:08 PM Bharath Rupireddy
> > <bharath.rupireddyforpostgres@gmail.com> wrote:
> > >
> > > On Wed, May 19, 2021 at 2:33 PM Amul Sul <sulamul@gmail.com> wrote:
> > > >
> > > > On Wed, May 19, 2021 at 2:09 PM Bharath Rupireddy
> > > > <bharath.rupireddyforpostgres@gmail.com> wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > parse_subscription_options function has some similar code when
> > > > > throwing errors [with the only difference in the option]. I feel we
> > > > > could just use a variable for the option and use it in the error.
> >
> > I am not sure how much it helps to just refactor this part of the code
> > alone unless we need to add/change it more. Having said that, this
> > function is being modified by one of the proposed patches for logical
> > decoding of 2PC and I noticed that the proposed patch is adding more
> > parameters to this function which already takes 14 input parameters,
> > so I suggested refactoring it. See comment 11 in email[1]. See, if
> > that makes sense to you then we can refactor this function such that
> > it can be enhanced easily by future patches.
>
> Thanks Amit for the comments. I agree to move the parse options to a
> new structure ParseSubOptions as suggested. Then the function can just
> be parse_subscription_options(ParseSubOptions opts); I wonder if we
> should also have a structure for parse_publication_options as we might
> add new options there in the future?
>

That function has just 5 parameters so not sure if that needs the same
treatment. Let's leave it for now.

-- 
With Regards,
Amit Kapila.