Horton

Thread

Commits

  1. Update ICU C++ API symbols

  2. Hide ICU C++ APIs from pg_locale.h

  3. Remove implicit cast from 'void *'

  4. Inline CRC computation for small fixed-length input on x86

  1. implicit casts from void*

    John Naylor <johncnaylorls@gmail.com> — 2025-07-01T03:20:07Z

    I received on off-list report that commit e2809e3a101 causes an error
when building an extension written in C++, since $subject is in a
header file. The fix is simply to add an explicit cast, so I plan to
push the attached soon.

Bikeshedding: We could additionally change the pg_crc*.c files to make
them consistent, but I have not done that yet. It seems we prefer
explicit casts anyway but don't enforce that.

-- 
John Naylor
Amazon Web Services

  2. Re: implicit casts from void*

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-07-01T03:36:10Z

    John Naylor <johncnaylorls@gmail.com> writes:
> I received on off-list report that commit e2809e3a101 causes an error
> when building an extension written in C++, since $subject is in a
> header file. The fix is simply to add an explicit cast, so I plan to
> push the attached soon.

Hmpfh.  No objection to your patch, but I wonder why
"headerscheck --cplusplus" didn't find this?  Can we get it
to do so?

> Bikeshedding: We could additionally change the pg_crc*.c files to make
> them consistent, but I have not done that yet. It seems we prefer
> explicit casts anyway but don't enforce that.

Meh.  There are an awful lot of places where we assume such casts
are okay.  I'm willing to adopt a stricter definition in header
files, but it feels like requiring it in .c files is useless
make-work.  As a perhaps not quite exact parallel, we mostly
don't object to writing

	if (ptr)

as a shortcut for

	if (ptr != NULL)

though it's hard to see the former as anything but an implicit
cast to boolean.

			regards, tom lane

  3. Re: implicit casts from void*

    John Naylor <johncnaylorls@gmail.com> — 2025-07-01T05:56:57Z

    On Tue, Jul 1, 2025 at 10:36 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> John Naylor <johncnaylorls@gmail.com> writes:
> > I received on off-list report that commit e2809e3a101 causes an error
> > when building an extension written in C++, since $subject is in a
> > header file. The fix is simply to add an explicit cast, so I plan to
> > push the attached soon.
>
> Hmpfh.  No objection to your patch, but I wonder why
> "headerscheck --cplusplus" didn't find this?  Can we get it
> to do so?

Good question, and it turns out it catches it just fine, but you have
to configure with CPPFLAGS="-msse4.2" (or run the script on a Red Hat
9-ish system).

--
John Naylor
Amazon Web Services

  4. Re: implicit casts from void*

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-07-01T14:24:07Z

    John Naylor <johncnaylorls@gmail.com> writes:
> On Tue, Jul 1, 2025 at 10:36 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Hmpfh.  No objection to your patch, but I wonder why
>> "headerscheck --cplusplus" didn't find this?  Can we get it
>> to do so?

> Good question, and it turns out it catches it just fine, but you have
> to configure with CPPFLAGS="-msse4.2" (or run the script on a Red Hat
> 9-ish system).

Ha, indeed you are right.  On my RHEL9 box, it's kinda drowned out
by complaints about

/usr/include/c++/11/bits/range_access.h:109:3: error: template with C linkage
  109 |   template<typename _Tp> _Tp* end(valarray<_Tp>&) noexcept;
      |   ^~~~~~~~
/tmp/headerscheck.u5CrRM/test.cpp:1:1: note: ‘extern "C"’ linkage started here
    1 | extern "C" {
      | ^~~~~~~~~~

but looking closer, I do see some

./src/include/port/pg_crc32c.h: In function ‘pg_crc32c pg_comp_crc32c_dispatch(pg_crc32c, const void*, size_t)’:
./src/include/port/pg_crc32c.h:75:42: error: invalid conversion from ‘const void*’ to ‘const unsigned char*’ [-fpermissive]
   75 |                 const unsigned char *p = data;
      |                                          ^~~~
      |                                          |
      |                                          const void*


			regards, tom lane

  5. cpluspluscheck vs ICU again

    John Naylor <johncnaylorls@gmail.com> — 2025-07-02T07:01:13Z

    On Tue, Jul 1, 2025 at 9:24 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Ha, indeed you are right.  On my RHEL9 box, it's kinda drowned out
> by complaints about
>
> /usr/include/c++/11/bits/range_access.h:109:3: error: template with C linkage
>   109 |   template<typename _Tp> _Tp* end(valarray<_Tp>&) noexcept;
>       |   ^~~~~~~~
> /tmp/headerscheck.u5CrRM/test.cpp:1:1: note: ‘extern "C"’ linkage started here
>     1 | extern "C" {
>       | ^~~~~~~~~~

After pushing my fix, I looked into this, and CI works around this by
disabling ICU. A proper fix was discussed here, but it trailed off:

https://www.postgresql.org/message-id/flat/20230311033727.koa4saxy5wyquu6s%40awork3.anarazel.de#03346c63050bbc69dfca8981a5698e4a

I came up with the attached -- Andres, Peter, does this match your recollection?

--
John Naylor
Amazon Web Services

  6. Re: cpluspluscheck vs ICU again

    Peter Eisentraut <peter@eisentraut.org> — 2025-07-02T07:19:39Z

    On 02.07.25 09:01, John Naylor wrote:
> On Tue, Jul 1, 2025 at 9:24 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Ha, indeed you are right.  On my RHEL9 box, it's kinda drowned out
>> by complaints about
>>
>> /usr/include/c++/11/bits/range_access.h:109:3: error: template with C linkage
>>    109 |   template<typename _Tp> _Tp* end(valarray<_Tp>&) noexcept;
>>        |   ^~~~~~~~
>> /tmp/headerscheck.u5CrRM/test.cpp:1:1: note: ‘extern "C"’ linkage started here
>>      1 | extern "C" {
>>        | ^~~~~~~~~~
> 
> After pushing my fix, I looked into this, and CI works around this by
> disabling ICU. A proper fix was discussed here, but it trailed off:
> 
> https://www.postgresql.org/message-id/flat/20230311033727.koa4saxy5wyquu6s%40awork3.anarazel.de#03346c63050bbc69dfca8981a5698e4a
> 
> I came up with the attached -- Andres, Peter, does this match your recollection?

This looks sensible to me.  Assuming that it works for this purpose, it 
seems otherwise harmless.

  7. Re: cpluspluscheck vs ICU again

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-07-02T14:44:50Z

    John Naylor <johncnaylorls@gmail.com> writes:
> After pushing my fix, I looked into this, and CI works around this by
> disabling ICU. A proper fix was discussed here, but it trailed off:
> https://www.postgresql.org/message-id/flat/20230311033727.koa4saxy5wyquu6s%40awork3.anarazel.de#03346c63050bbc69dfca8981a5698e4a
> I came up with the attached -- Andres, Peter, does this match your recollection?

I tested this on my RHEL9 box, and confirm that I get a clean build
and headerscheck is silent.

			regards, tom lane

  8. Re: cpluspluscheck vs ICU again

    Andres Freund <andres@anarazel.de> — 2025-07-04T12:39:45Z

    Hi,

On 2025-07-02 14:01:13 +0700, John Naylor wrote:
> On Tue, Jul 1, 2025 at 9:24 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Ha, indeed you are right.  On my RHEL9 box, it's kinda drowned out
> > by complaints about
> >
> > /usr/include/c++/11/bits/range_access.h:109:3: error: template with C linkage
> >   109 |   template<typename _Tp> _Tp* end(valarray<_Tp>&) noexcept;
> >       |   ^~~~~~~~
> > /tmp/headerscheck.u5CrRM/test.cpp:1:1: note: ‘extern "C"’ linkage started here
> >     1 | extern "C" {
> >       | ^~~~~~~~~~
> 
> After pushing my fix, I looked into this, and CI works around this by
> disabling ICU. A proper fix was discussed here, but it trailed off:
> 
> https://www.postgresql.org/message-id/flat/20230311033727.koa4saxy5wyquu6s%40awork3.anarazel.de#03346c63050bbc69dfca8981a5698e4a
> 
> I came up with the attached -- Andres, Peter, does this match your recollection?

I think the proper fix here would be to not expose ucol.h to the world,
i.e. not include it from something like pg_locale.h.


> diff --git a/src/include/utils/pg_locale.h b/src/include/utils/pg_locale.h
> index 44ff60a25b4..300c78ba93c 100644
> --- a/src/include/utils/pg_locale.h
> +++ b/src/include/utils/pg_locale.h
> @@ -15,7 +15,12 @@
>  #include "mb/pg_wchar.h"
>  
>  #ifdef USE_ICU
> +/* only include the C APIs, to avoid errors in cpluspluscheck */
> +#undef U_SHOW_CPLUSPLUS_API
> +#define U_SHOW_CPLUSPLUS_API 0
>  #include <unicode/ucol.h>
> +/* restore so that extensions can include the C++ APIs */
> +#undef U_SHOW_CPLUSPLUS_API
>  #endif

Does the #undef U_SHOW_CPLUSPLUS_API thing actually work, given that ucol.h
presumably won't be included again due to #ifdef protection?

Greetings,

Andres Freund

  9. Re: cpluspluscheck vs ICU again

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-07-04T15:11:49Z

    Andres Freund <andres@anarazel.de> writes:
> On 2025-07-02 14:01:13 +0700, John Naylor wrote:
>> I came up with the attached -- Andres, Peter, does this match your recollection?

> I think the proper fix here would be to not expose ucol.h to the world,
> i.e. not include it from something like pg_locale.h.

The stumbling block to that is that pg_locale_struct has a field of
type UCollator.  Of course there are workarounds, but I think all of
them are strictly worse than including <ucol.h> here.

>> +/* restore so that extensions can include the C++ APIs */
>> +#undef U_SHOW_CPLUSPLUS_API

> Does the #undef U_SHOW_CPLUSPLUS_API thing actually work, given that ucol.h
> presumably won't be included again due to #ifdef protection?

Good point.  If a .cpp file wants access to the C++ APIs, it'd have
to include <unicode/ucol.h> before not after including pg_locale.h.
That should work AFAICS, but this #undef doesn't help.

			regards, tom lane

  10. Re: cpluspluscheck vs ICU again

    John Naylor <johncnaylorls@gmail.com> — 2025-07-07T05:52:58Z

    On Fri, Jul 4, 2025 at 10:11 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Andres Freund <andres@anarazel.de> writes:

> >> +/* restore so that extensions can include the C++ APIs */
> >> +#undef U_SHOW_CPLUSPLUS_API
>
> > Does the #undef U_SHOW_CPLUSPLUS_API thing actually work, given that ucol.h
> > presumably won't be included again due to #ifdef protection?
>
> Good point.  If a .cpp file wants access to the C++ APIs, it'd have
> to include <unicode/ucol.h> before not after including pg_locale.h.
> That should work AFAICS, but this #undef doesn't help.

I see that now. If extensions follow the practice of including system
headers before Postgres headers, it should be fine. I've attached v2
which removes the useless #undef and drafts an explanatory commit
message.

--
John Naylor
Amazon Web Services

  11. Re: cpluspluscheck vs ICU again

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-07-07T16:06:10Z

    John Naylor <johncnaylorls@gmail.com> writes:
> I see that now. If extensions follow the practice of including system
> headers before Postgres headers, it should be fine. I've attached v2
> which removes the useless #undef and drafts an explanatory commit
> message.

Works for me.

			regards, tom lane

  12. Re: cpluspluscheck vs ICU again

    John Naylor <johncnaylorls@gmail.com> — 2025-07-09T07:40:40Z

    On Mon, Jul 7, 2025 at 11:06 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> John Naylor <johncnaylorls@gmail.com> writes:
> > I see that now. If extensions follow the practice of including system
> > headers before Postgres headers, it should be fine. I've attached v2
> > which removes the useless #undef and drafts an explanatory commit
> > message.
>
> Works for me.

Pushed.

-- 
John Naylor
Amazon Web Services

  13. Re: cpluspluscheck vs ICU again

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-08-05T17:26:22Z

    John Naylor <johncnaylorls@gmail.com> writes:
> On Mon, Jul 7, 2025 at 11:06 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> John Naylor <johncnaylorls@gmail.com> writes:
>>> I see that now. If extensions follow the practice of including system
>>> headers before Postgres headers, it should be fine. I've attached v2
>>> which removes the useless #undef and drafts an explanatory commit
>>> message.

>> Works for me.

> Pushed.

Just when you thought it was safe to go back in the water ...
I tried cpluspluscheck with late-model libicu (76.1 on Fedora 42)
and darned if it didn't blow up in exactly the same way.
Investigation reveals that they've split the "U_SHOW_CPLUSPLUS_API"
symbol into two, and now if you really really don't want any C++
stuff you need to also set "U_SHOW_CPLUSPLUS_HEADER_API" to zero.

I've confirmed that this re-silences the failures:

diff --git a/src/include/utils/pg_locale.h b/src/include/utils/pg_locale.h
index 931f5b3b880..2b072cafb4d 100644
--- a/src/include/utils/pg_locale.h
+++ b/src/include/utils/pg_locale.h
@@ -18,6 +18,8 @@
 /* only include the C APIs, to avoid errors in cpluspluscheck */
 #undef U_SHOW_CPLUSPLUS_API
 #define U_SHOW_CPLUSPLUS_API 0
+#undef U_SHOW_CPLUSPLUS_HEADER_API
+#define U_SHOW_CPLUSPLUS_HEADER_API 0
 #include <unicode/ucol.h>
 #endif

This shouldn't complicate extensions' lives any further than
before; the rule still is "include ICU headers first
if you want their C++ symbols".

BTW, I see that you applied ed26c4e25 only to master, but don't
we want to back-patch?  cpluspluscheck is not just an exercise in a
vacuum, it's to ensure that C++-coded extensions don't have trouble
with our headers.

			regards, tom lane

  14. Re: cpluspluscheck vs ICU again

    John Naylor <johncnaylorls@gmail.com> — 2025-08-06T12:16:00Z

    On Wed, Aug 6, 2025 at 12:26 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> diff --git a/src/include/utils/pg_locale.h b/src/include/utils/pg_locale.h
> index 931f5b3b880..2b072cafb4d 100644
> --- a/src/include/utils/pg_locale.h
> +++ b/src/include/utils/pg_locale.h
> @@ -18,6 +18,8 @@
>  /* only include the C APIs, to avoid errors in cpluspluscheck */
>  #undef U_SHOW_CPLUSPLUS_API
>  #define U_SHOW_CPLUSPLUS_API 0
> +#undef U_SHOW_CPLUSPLUS_HEADER_API
> +#define U_SHOW_CPLUSPLUS_HEADER_API 0
>  #include <unicode/ucol.h>
>  #endif
>
> This shouldn't complicate extensions' lives any further than
> before; the rule still is "include ICU headers first
> if you want their C++ symbols".
>
> BTW, I see that you applied ed26c4e25 only to master, but don't
> we want to back-patch?  cpluspluscheck is not just an exercise in a
> vacuum, it's to ensure that C++-coded extensions don't have trouble
> with our headers.

I was thinking that it was run only when developing new features, not
for backpatch-able bug fixes, but that's a flawed assumption. I'll
remedy that soon along with the new symbols above, unless you beat me
to it.

--
John Naylor
Amazon Web Services

  15. Re: cpluspluscheck vs ICU again

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-08-06T14:01:03Z

    John Naylor <johncnaylorls@gmail.com> writes:
> On Wed, Aug 6, 2025 at 12:26 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> BTW, I see that you applied ed26c4e25 only to master, but don't
>> we want to back-patch?  cpluspluscheck is not just an exercise in a
>> vacuum, it's to ensure that C++-coded extensions don't have trouble
>> with our headers.

> I was thinking that it was run only when developing new features, not
> for backpatch-able bug fixes, but that's a flawed assumption. I'll
> remedy that soon along with the new symbols above, unless you beat me
> to it.

Sounds good, thanks for dealing with it.

			regards, tom lane

  16. Re: cpluspluscheck vs ICU again

    John Naylor <johncnaylorls@gmail.com> — 2025-08-07T10:30:26Z

    On Wed, Aug 6, 2025 at 7:16 PM John Naylor <johncnaylorls@gmail.com> wrote:
> > BTW, I see that you applied ed26c4e25 only to master, but don't
> > we want to back-patch?  cpluspluscheck is not just an exercise in a
> > vacuum, it's to ensure that C++-coded extensions don't have trouble
> > with our headers.
>
> I was thinking that it was run only when developing new features, not
> for backpatch-able bug fixes, but that's a flawed assumption. I'll
> remedy that soon along with the new symbols above, unless you beat me
> to it.

This is done.

-- 
John Naylor
Amazon Web Services