On Mon, Apr 26, 2021 at 5:49 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Mon, Apr 26, 2021 at 5:29 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > Hi,
> >
> > While reviewing one of the logical replication patches, I found that
> > we do not include hint messages to display the actual option which has
> > been specified more than once in case of redundant option error. I
> > felt including this will help in easily identifying the error, users
> > will not have to search through the statement to identify where the
> > actual error is present. Attached a patch for the same.
> > Thoughts?
>
> +1. A more detailed error will be useful in a rare scenario like users
> have specified duplicate options along with a lot of other options,
> they will know for which option the error is thrown by the server.
>
> Instead of adding errhint or errdetail, how about just changing the
> error message itself to something like "option \"%s\" specified more
> than once" or "parameter \"%s\" specified more than once" like we have
> in other places in the code?
>
Both seemed fine but I preferred using errdetail as I felt it is
slightly better for the details to appear in a new line.
> Comments on the patch:
>
> 1) generally errhint and errdetail messages should end with a period
> ".", please see their usage in other places.
> + errhint("Option \"streaming\" specified more
> than once")));
>
Modified it.
> 2) I think it should be errdetail instead of errhint, because you are
> giving more information about the error, but not hinting user how to
> overcome it. If you had to say something like "Remove duplicate
> \"password\" option." or "The \"password\" option is specified more
> than once. Remove all the duplicates.", then it makes sense to use
> errhint.
Modified it.
Attached patch for the same.
Regards,
Vignesh