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