From: Tom Lane <tgl@sss.pgh.pa.us>
To: Michael Paquier <michael@paquier.xyz>
cc: Corey Huinker <corey.huinker@gmail.com>,
        jian he <jian.universality@gmail.com>, Tomas Vondra <tomas@vondra.me>,
        pgsql-hackers@lists.postgresql.org
Subject: Re: Extended Statistics set/restore/clear functions.
In-reply-to: <aSZgkssl7NT1jI2h@paquier.xyz>
References: <aRbLV7Tru3ulO5ru@paquier.xyz> <aRrHBj7I4CeV21dj@paquier.xyz>
 <CACJufxGGzkb58BU+YyTa9cBAawhybwk2cPFZ1XupS-8xuAzN9A@mail.gmail.com>
 <aRwGeN7o22HJclg5@paquier.xyz>
 <CADkLM=czQ0UUyBn3AiHZfJJWRwXH5e2xn056yG0mpuXpW09RSw@mail.gmail.com>
 <CACJufxHVKiVEvCsc11oxOM=0GgjqChFwi-ii4RJuw95ZFB_fHQ@mail.gmail.com>
 <CADkLM=eRop0t==hg0TU3KuDH6DgnUxQ6vZxV9th=6vW1JG36MQ@mail.gmail.com>
 <CACJufxGgEnOzFq0HjmkKBTnEUgToGFC0g0A4wPWoJMzOuoWUhw@mail.gmail.com>
 <CADkLM=crdyK3w7-EDEdDfXHGidxm2ms+bQYNFOZUD4eOLuLXEQ@mail.gmail.com>
 <aSVas5CIr1mvR2VK@paquier.xyz> <aSZgkssl7NT1jI2h@paquier.xyz>
Comments: In-reply-to Michael Paquier <michael@paquier.xyz>
	message dated "Wed, 26 Nov 2025 11:06:10 +0900"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1341063.1764895052.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Thu, 04 Dec 2025 19:37:32 -0500
Message-ID: <1341064.1764895052@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/1341064.1764895052%40sss.pgh.pa.us>
Precedence: bulk

Michael Paquier <michael@paquier.xyz> writes:
> Okay.  I have gone other these two and after an extra round of
> polishing, mainly around comments (some suggested by Chao, actually),
> style, a fix for the test with valid UTF8 sequences failing in
> non-UTF8 databases, plus a few more tests that were missing, I have
> applied the two patches for the input functions.

I notice that since e1405aa5e went in, a number of older buildfarm
animals are issuing warnings about it:

In file included from pg_dependencies.c:21:0:
pg_dependencies.c: In function "dependencies_scalar":
../../../../src/include/nodes/miscnodes.h:54:15: warning: the comparison w=
ill always evaluate as "true" for the address of "escontext" will never be=
 NULL [-Waddress]
  ((escontext) !=3D NULL && IsA(escontext, ErrorSaveContext) && \\
               ^
pg_dependencies.c:497:8: note: in expansion of macro "SOFT_ERROR_OCCURRED"
    if (SOFT_ERROR_OCCURRED(&escontext))
        ^
../../../../src/include/nodes/miscnodes.h:54:15: warning: the comparison w=
ill always evaluate as "true" for the address of "escontext" will never be=
 NULL [-Waddress]
  ((escontext) !=3D NULL && IsA(escontext, ErrorSaveContext) && \\
               ^
pg_dependencies.c:542:8: note: in expansion of macro "SOFT_ERROR_OCCURRED"
    if (SOFT_ERROR_OCCURRED(&escontext))
        ^
../../../../src/include/nodes/miscnodes.h:54:15: warning: the comparison w=
ill always evaluate as "true" for the address of "escontext" will never be=
 NULL [-Waddress]
  ((escontext) !=3D NULL && IsA(escontext, ErrorSaveContext) && \\
               ^
pg_dependencies.c:572:8: note: in expansion of macro "SOFT_ERROR_OCCURRED"
    if (SOFT_ERROR_OCCURRED(&escontext))
        ^
In file included from pg_ndistinct.c:21:0:
pg_ndistinct.c: In function "ndistinct_scalar":
../../../../src/include/nodes/miscnodes.h:54:15: warning: the comparison w=
ill always evaluate as "true" for the address of "escontext" will never be=
 NULL [-Waddress]
  ((escontext) !=3D NULL && IsA(escontext, ErrorSaveContext) && \\
               ^
pg_ndistinct.c:438:8: note: in expansion of macro "SOFT_ERROR_OCCURRED"
    if (SOFT_ERROR_OCCURRED(&escontext))
        ^
../../../../src/include/nodes/miscnodes.h:54:15: warning: the comparison w=
ill always evaluate as "true" for the address of "escontext" will never be=
 NULL [-Waddress]
  ((escontext) !=3D NULL && IsA(escontext, ErrorSaveContext) && \\
               ^
pg_ndistinct.c:488:9: note: in expansion of macro "SOFT_ERROR_OCCURRED"
    if (!SOFT_ERROR_OCCURRED(&escontext))
         ^

The above is from rhinoceros, but about half a dozen other animals
are reporting the same.  They are all running very old gcc versions,
mostly from RHEL7 or CentOS 7.

This of course arises because the source code is passing the address
of a local ErrorSaveContext variable to that macro.  We don't have
any other instances of that coding pattern AFAICS, so I wonder if
that was really the most adapted way to do it.  Looking at the
code, it seems to be turning around and stuffing a different error
message into a passed-in ErrorSaveContext, which feels a little
weird.

It may be that there's not a better way, in which case I'll probably
just teach my buildfarm-warning-scraper script to ignore these
warnings, as it's already doing for some other warnings from these
same animals (-Wmissing-braces mostly).  But I thought I'd raise a
question about it.

Also, I don't think these errdetail messages meet our style
guidelines:

                        errdetail("Invalid \"%s\" value.", PG_DEPENDENCIES=
_KEY_ATTRIBUTES));

They're supposed to be complete sentences.

			regards, tom lane