public inbox for [email protected]  

help / color / mirror / Atom feed

From: Jeff Davis <[email protected]>
To: Michael Paquier <[email protected]>
To: Ranier Vilela <[email protected]>
Cc: Robert Haas <[email protected]>
Cc: Pg Hackers <[email protected]>
Subject: Re: Avoid a possible null pointer (src/backend/utils/adt/pg_locale.c)
Date: Fri, 08 Sep 2023 14:24:00 -0700
Message-ID: <[email protected]> (raw)
In-Reply-To: <ZPq+HKWh8fQ05/[email protected]>
References: <CAEudQApjNNJz-uSTa1WYueWntSLD7sA=_0BXgSt6acFTFwudoA@mail.gmail.com>
	<CA+TgmoZiu5HUd1GmM3qtrxVNA2mb-9ZbVmoRxS=usRe+gFmOow@mail.gmail.com>
	<CAEudQAr6uCWQq2F-3pPJ+RO7btn4Mrr8opno-Cs-56wby0CQkQ@mail.gmail.com>
	<[email protected]>
	<CAEudQAptJj0xZRaSjxhGvOGAJ9yk24rQJ3NoEu=Q-3jbcwtprw@mail.gmail.com>
	<CAEudQArof7O73ntXrQMnuRcwMhpN5POOAMz4-aHdT+Di3oqMVw@mail.gmail.com>
	<ZPq+HKWh8fQ05/[email protected]>

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

view thread (2+ messages)

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Avoid a possible null pointer (src/backend/utils/adt/pg_locale.c)
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox