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?
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")));
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.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com