Message-ID: <7073610042fcf97e1bea2ce08b7e0214b5e11094.camel@j-davis.com>
Subject: Re: Avoid a possible null pointer
 (src/backend/utils/adt/pg_locale.c)
From: Jeff Davis <pgsql@j-davis.com>
To: Michael Paquier <michael@paquier.xyz>, Ranier Vilela
 <ranier.vf@gmail.com>
Cc: Robert Haas <robertmhaas@gmail.com>, Pg Hackers
	 <pgsql-hackers@postgresql.org>
Date: Fri, 08 Sep 2023 14:24:00 -0700
In-Reply-To: <ZPq+HKWh8fQ05/5U@paquier.xyz>
References: 
	<CAEudQApjNNJz-uSTa1WYueWntSLD7sA=_0BXgSt6acFTFwudoA@mail.gmail.com>
	 <CA+TgmoZiu5HUd1GmM3qtrxVNA2mb-9ZbVmoRxS=usRe+gFmOow@mail.gmail.com>
	 <CAEudQAr6uCWQq2F-3pPJ+RO7btn4Mrr8opno-Cs-56wby0CQkQ@mail.gmail.com>
	 <ZPUsd2nHkS+UXhDc@paquier.xyz>
	 <CAEudQAptJj0xZRaSjxhGvOGAJ9yk24rQJ3NoEu=Q-3jbcwtprw@mail.gmail.com>
	 <CAEudQArof7O73ntXrQMnuRcwMhpN5POOAMz4-aHdT+Di3oqMVw@mail.gmail.com>
	 <ZPq+HKWh8fQ05/5U@paquier.xyz>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4-0ubuntu2 
MIME-Version: 1.0
Archived-At: <https://www.postgresql.org/message-id/7073610042fcf97e1bea2ce08b7e0214b5e11094.camel%40j-davis.com>
Precedence: bulk

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.=C2=A0 How about something like the v2
> attached instead to provide more context in the error message about
> the function called?=C2=A0 Let's say, when the provider is known, we coul=
d
> use:
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 elog(ERROR, "unsupported collprovid=
er (%s): %c",
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 "pg_s=
trncoll", locale->provider);

+1, thank you.

> And when the provider is not known, we could use:
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 elog(ERROR, "unsupported collprovid=
er (%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 =3D 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.=C2=A0 What is your take here?

Agreed.

Regards,
	Jeff Davis