Re: Avoid a possible null pointer (src/backend/utils/adt/pg_locale.c)

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