On Wed, May 19, 2021 at 4:42 PM Bharath Rupireddy <bharath.rupireddyforpostg...@gmail.com> wrote: > > On Wed, May 19, 2021 at 4:10 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > On Wed, May 19, 2021 at 3:08 PM Bharath Rupireddy > > <bharath.rupireddyforpostg...@gmail.com> wrote: > > > > > > On Wed, May 19, 2021 at 2:33 PM Amul Sul <sula...@gmail.com> wrote: > > > > > > > > On Wed, May 19, 2021 at 2:09 PM Bharath Rupireddy > > > > <bharath.rupireddyforpostg...@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.