Обсуждение: Avoid a possible null pointer (src/backend/utils/adt/pg_locale.c)

On Fri, Sep 1, 2023 at 11:47 AM Ranier Vilela <ranier.vf@gmail.com> wrote:
> If a null locale is reached in these paths.
> elog will dereference a null pointer.

True. That's sloppy coding.

I don't know enough about this code to be sure whether the error
messages that you propose are for the best.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Sat, Sep 02, 2023 at 09:13:11AM -0300, Ranier Vilela wrote:
> I tried to keep the same behavior as before.
> Note that if the locale equals COLLPROVIDER_LIBC,
> the message to the user will be the same.

-    /* shouldn't happen */
-    elog(ERROR, "unsupported collprovider: %c", locale->provider);
+    elog(ERROR, "collprovider '%c' does not support pg_strnxfrm_prefix()",
+             locale->provider);

Based on what's written here, these messages could be better because
full sentences are not encouraged in error messages, even for
non-translated elogs:
https://www.postgresql.org/docs/current/error-style-guide.html

Perhaps something like "could not use collprovider %c: no support for
%s", where the function name is used, would be more consistent.
--
Michael

On Wed, Sep 06, 2023 at 07:57:03AM -0300, Ranier Vilela wrote:
> I think no one objected.

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);

And when the provider is not known, we could use:
+       elog(ERROR, "unsupported collprovider (%s)", "pg_myfunc");

@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?
--
Michael

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

On Sun, Sep 10, 2023 at 06:28:08PM -0300, Ranier Vilela wrote:
> +1
> But as Jeff mentioned, when the locale is NULL,
> the provider is known to be COLLPROVIDER_LIBC.
>
> I think we can use this to provide an error message,
> when the locale is NULL.
>
> What do you think about v3 attached?

This does not apply for me on HEAD, and it seems to me that the patch
has some parts that apply on top of v2 (or v1?) while others would
apply to HEAD.

Anyway, what you are suggesting to change compared to v2 is that:

+    /*
+     * if locale is NULL, then
+     * the provider is known to be COLLPROVIDER_LIBC
+     */
     if (!locale)
-        elog(ERROR, "unsupported collprovider");
+        elog(ERROR, "collprovider '%c' does not support (%s)",
+            COLLPROVIDER_LIBC, "pg_strxfrm_prefix");

I'm OK with enforcing COLLPROVIDER_LIBC in this path, but I also value
consistency across all the error messages of this file.  After
sleeping on it, and as that's a set of elogs, "unsupported
collprovider" is fine for me across the board as these should not be
user-visible.

This should be made consistent down to 16, I guess, but only after
16.0 is tagged as we are in release week.
--
Michael

On Mon, 2023-09-11 at 07:24 +0900, Michael Paquier wrote:
> I'm OK with enforcing COLLPROVIDER_LIBC in this path, but I also
> value
> consistency across all the error messages of this file.  After
> sleeping on it, and as that's a set of elogs, "unsupported
> collprovider" is fine for me across the board as these should not be
> user-visible.

That's fine with me.

Regards,
    Jeff Davis

On Mon, Sep 11, 2023 at 12:15:49PM -0700, Jeff Davis wrote:
> That's fine with me.

Okay.  Then, please find attached a v4 for HEAD and REL_16_STABLE.
--
Michael

On Tue, 2023-09-12 at 09:03 +0900, Michael Paquier wrote:
> On Mon, Sep 11, 2023 at 12:15:49PM -0700, Jeff Davis wrote:
> > That's fine with me.
>
> Okay.  Then, please find attached a v4 for HEAD and REL_16_STABLE.

One question: would it make sense to use __func__?

Other than that, looks good. Thank you.

Regards,
    Jeff Davis

At Tue, 12 Sep 2023 09:03:27 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> On Mon, Sep 11, 2023 at 12:15:49PM -0700, Jeff Davis wrote:
> > That's fine with me.
> 
> Okay.  Then, please find attached a v4 for HEAD and REL_16_STABLE.

For example, they result in the following message:

ERROR:  unsupported collprovider (pg_strcoll): i

Even if it is an elog message, I believe we can make it clearer. The
pg_strcoll seems like a collation privier at first glance. Not sure
about others, though, I would spell it as the follows instead:

ERROR:  unsupported collprovider in pg_strcoll: i
ERROR:  unsupported collprovider in pg_strcoll(): i

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Tue, Sep 12, 2023 at 09:40:04PM -0300, Ranier Vilela wrote:
> I think that is not a good idea.

Hm?  We already use __func__ across the tree even on Windows and
nobody has complained about that.  Using a macro for the elog()
generated would be slightly more elegant, actually.
--
Michael

On Wed, Sep 13, 2023 at 09:59:22AM +0900, Kyotaro Horiguchi wrote:
> At Tue, 12 Sep 2023 09:03:27 +0900, Michael Paquier <michael@paquier.xyz> wrote in
> > On Mon, Sep 11, 2023 at 12:15:49PM -0700, Jeff Davis wrote:
> > > That's fine with me.
> >
> > Okay.  Then, please find attached a v4 for HEAD and REL_16_STABLE.
>
> For example, they result in the following message:
>
> ERROR:  unsupported collprovider (pg_strcoll): i
>
> Even if it is an elog message, I believe we can make it clearer. The
> pg_strcoll seems like a collation privier at first glance. Not sure
> about others, though, I would spell it as the follows instead:
>
> ERROR:  unsupported collprovider in pg_strcoll: i
> ERROR:  unsupported collprovider in pg_strcoll(): i

Hmm.  I see your point, one could be confused that the function name
is the provider with this wording.  How about that instead:
 ERROR:  unsupported collprovider for %s: %c

I've hidden that in a macro that uses __func__ as Jeff has suggested.

What do you think?
--
Michael

On Wed, 2023-09-13 at 11:48 +0900, Michael Paquier wrote:
> Hmm.  I see your point, one could be confused that the function name
> is the provider with this wording.  How about that instead:
>  ERROR:  unsupported collprovider for %s: %c
>
> I've hidden that in a macro that uses __func__ as Jeff has suggested.
>
> What do you think?

Looks good to me, thank you.

Regards,
    Jeff Davis

On Wed, Sep 13, 2023 at 08:14:11AM -0700, Jeff Davis wrote:
> Looks good to me, thank you.

Applied, then.  Thanks.
--
Michael