On Wed, May 19, 2021 at 5:56 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> 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.
Thanks. I will work on the new structure ParseSubOption only for
subscription options.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
