On Fri, 2023-09-08 at 15:24 +0900, Michael Paquier wrote:
> Looking closer, there is much more inconsistency in this file
> depending on the routine called. How about something like the v2
> attached instead to provide more context in the error message about
> the function called? Let's say, when the provider is known, we could
> use:
> + elog(ERROR, "unsupported collprovider (%s): %c",
> + "pg_strncoll", locale->provider);
+1, thank you.
> And when the provider is not known, we could use:
> + elog(ERROR, "unsupported collprovider (%s)", "pg_myfunc");
It's not that the provider is "not known" -- if locale is NULL, then
the provider is known to be COLLPROVIDER_LIBC. So perhaps we can
instead do something like:
char provider = locale ? locale->provider : COLLPROVIDER_LIBC;
and then always follow the first error format?
[ Aside: it might be nice to refactor so that we used a pointer to a
special static struct rather than NULL, which would cut down on these
kinds of special cases. I had considered doing that before and didn't
get around to it. ]
> @Jeff (added now in CC), the refactoring done in d87d548c seems to be
> at the origin of this confusion, because, before this commit, we
> never
> generated this specific error for all these APIs where the locale is
> undefined. What is your take here?
Agreed.
Regards,
Jeff Davis