Horton

Thread

Commits

  1. Refactor error messages for unsupported providers in pg_locale.c

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

    Ranier Vilela <ranier.vf@gmail.com> — 2023-09-01T14:38:12Z

    Hi,

If a null locale is reached in these paths.
elog will dereference a null pointer.

best regards,

Ranier Vilela

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

    Robert Haas <robertmhaas@gmail.com> — 2023-09-01T20:16:49Z

    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

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

    Ranier Vilela <ranier.vf@gmail.com> — 2023-09-02T12:13:11Z

    Em sex., 1 de set. de 2023 às 17:17, Robert Haas <robertmhaas@gmail.com>
escreveu:

> 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.
>
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.

best regards,
Ranier Vilela

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

    Michael Paquier <michael@paquier.xyz> — 2023-09-04T01:01:43Z

    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

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

    Ranier Vilela <ranier.vf@gmail.com> — 2023-09-04T14:27:24Z

    Em dom., 3 de set. de 2023 às 22:01, Michael Paquier <michael@paquier.xyz>
escreveu:

> 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.
>
Sure.
I have no objection to the wording of the message.
If there is consensus, I can send another patch.

best regards,
Ranier Vilela

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

    Ranier Vilela <ranier.vf@gmail.com> — 2023-09-06T10:57:03Z

    Em seg., 4 de set. de 2023 às 11:27, Ranier Vilela <ranier.vf@gmail.com>
escreveu:

> Em dom., 3 de set. de 2023 às 22:01, Michael Paquier <michael@paquier.xyz>
> escreveu:
>
>> 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.
>>
> Sure.
> I have no objection to the wording of the message.
> If there is consensus, I can send another patch.
>
I think no one objected.

v1 attached.

best regards,
Ranier Vilela

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

    Michael Paquier <michael@paquier.xyz> — 2023-09-08T06:24:28Z

    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

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

    Jeff Davis <pgsql@j-davis.com> — 2023-09-08T21:24:00Z

    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

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

    Ranier Vilela <ranier.vf@gmail.com> — 2023-09-10T21:28:08Z

    Em sex., 8 de set. de 2023 às 03:24, Michael Paquier <michael@paquier.xyz>
escreveu:

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

+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?

best regards,
Ranier Vilela

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

    Michael Paquier <michael@paquier.xyz> — 2023-09-10T22:24:27Z

    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

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

    Jeff Davis <pgsql@j-davis.com> — 2023-09-11T19:15:49Z

    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

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

    Michael Paquier <michael@paquier.xyz> — 2023-09-12T00:03:27Z

    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

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

    Ranier Vilela <ranier.vf@gmail.com> — 2023-09-12T00:06:53Z

    Em seg., 11 de set. de 2023 às 21:03, Michael Paquier <michael@paquier.xyz>
escreveu:

> 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.
>
LGTM.

best regards,
Ranier Vilela

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

    Jeff Davis <pgsql@j-davis.com> — 2023-09-12T20:51:25Z

    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

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

    Ranier Vilela <ranier.vf@gmail.com> — 2023-09-13T00:40:04Z

    Em ter., 12 de set. de 2023 às 17:51, Jeff Davis <pgsql@j-davis.com>
escreveu:

> 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__?
>
According to the msvc documentation, __func__ requires C++11.
https://learn.microsoft.com/en-us/cpp/cpp/func?view=msvc-170

I think that is not a good idea.

best regards,
Ranier Vilela

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

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2023-09-13T00:59:22Z

    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

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

    Michael Paquier <michael@paquier.xyz> — 2023-09-13T01:16:45Z

    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

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

    Michael Paquier <michael@paquier.xyz> — 2023-09-13T02:48:59Z

    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

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

    Jeff Davis <pgsql@j-davis.com> — 2023-09-13T15:14:11Z

    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

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

    Michael Paquier <michael@paquier.xyz> — 2023-09-14T01:32:02Z

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

Applied, then.  Thanks.
--
Michael

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

    Ranier Vilela <ranier.vf@gmail.com> — 2023-09-14T11:08:42Z

    Em qua., 13 de set. de 2023 às 22:32, Michael Paquier <michael@paquier.xyz>
escreveu:

> On Wed, Sep 13, 2023 at 08:14:11AM -0700, Jeff Davis wrote:
> > Looks good to me, thank you.
>
> Applied, then.  Thanks.
>
Thank you Michael, for the commit.

best regards,
Ranier Vilela