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? If okay, I can work on these changes and attach it along with these error message changes. Thoughts? > > > > While this has no benefit at all, it saves some LOC and makes the code > > > > look better with lesser ereport(ERROR statements. PSA patch. > > > > > > > > Thoughts? > > > > > > I don't have a strong opinion on this, but the patch should add > > > __translator__ help comment for the error msg. > > > > Is the "/*- translator:" help comment something visible to the user or > > some other tool? > > > > We use similar comments at other places. So, it makes sense to retain > the comment as it might help translation tools. I will retail it. With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
