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

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?

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