Horton

Thread

Commits

  1. Move pg_int64 back to postgres_ext.h

  2. pgbench: Make set_random_seed() 64-bit everywhere.

  3. Use PRI?64 instead of "ll?" in format strings (continued).

  4. Fix order of -I switches for building pg_regress.o.

  5. libpq: Deprecate pg_int64.

  6. Use PRI*64 instead of "ll*" in format strings (minimal trial)

  7. Fix header inclusion order in c.h.

  8. Use <stdint.h> and <inttypes.h> for c.h integers.

  9. Remove traces of BeOS.

  10. More correct way to check for existence of types, which allows to specify

  1. Cannot find a working 64-bit integer type on Illumos

    Japin Li <japinli@hotmail.com> — 2024-03-22T16:48:05Z

    Hi, hackers,

When I try to configure PostgreSQL 16.2 on Illumos using the following command,
it complains $subject.

    ./configure --enable-cassert --enable-debug --enable-nls --with-perl \
      --with-python --without-tcl --without-gssapi --with-openssl \
      --with-ldap --with-libxml --with-libxslt --without-systemd \
      --with-readline --enable-thread-safety --enable-dtrace \
      DTRACEFLAGS=-64 CFLAGS=-Werror

However, if I remove the `CFLAGS=-Werror`, it works fine.

I'm not sure what happened here.

$ uname -a
SunOS db_build 5.11 hunghu-20231216T132436Z i86pc i386 i86pc illumos
$ gcc --version
gcc (GCC) 10.4.0
Copyright (C) 2020 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

  2. Re: Cannot find a working 64-bit integer type on Illumos

    Andres Freund <andres@anarazel.de> — 2024-03-22T16:53:05Z

    Hi,

On 2024-03-23 00:48:05 +0800, Japin Li wrote:
> When I try to configure PostgreSQL 16.2 on Illumos using the following command,
> it complains $subject.
> 
>     ./configure --enable-cassert --enable-debug --enable-nls --with-perl \
>       --with-python --without-tcl --without-gssapi --with-openssl \
>       --with-ldap --with-libxml --with-libxslt --without-systemd \
>       --with-readline --enable-thread-safety --enable-dtrace \
>       DTRACEFLAGS=-64 CFLAGS=-Werror
> 
> However, if I remove the `CFLAGS=-Werror`, it works fine.

Likely there's an unrelated warning triggering the configure test to
fail. We'd need to see config.log to see what that is.

Greetings,

Andres Freund

  3. Re: Cannot find a working 64-bit integer type on Illumos

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-03-22T17:04:40Z

    Japin Li <japinli@hotmail.com> writes:
> When I try to configure PostgreSQL 16.2 on Illumos using the following command,
> it complains $subject.

>     ./configure --enable-cassert --enable-debug --enable-nls --with-perl \
>       --with-python --without-tcl --without-gssapi --with-openssl \
>       --with-ldap --with-libxml --with-libxslt --without-systemd \
>       --with-readline --enable-thread-safety --enable-dtrace \
>       DTRACEFLAGS=-64 CFLAGS=-Werror

> However, if I remove the `CFLAGS=-Werror`, it works fine.
> I'm not sure what happened here.

CFLAGS=-Werror breaks a whole lot of configure's tests, not only that
one.  (We even have this documented, see [1].)  So you can't inject
-Werror that way.  What I do on my buildfarm animals is the equivalent
of

	export COPT='-Werror'

after configure and before build.  I think configure pays no attention
to COPT, so it'd likely be safe to keep that set all the time, but in
the buildfarm client it's just as easy to be conservative.

			regards, tom lane

[1] https://www.postgresql.org/docs/devel/install-make.html#CONFIGURE-ENVVARS

  4. Re: Cannot find a working 64-bit integer type on Illumos

    Japin Li <japinli@hotmail.com> — 2024-03-22T17:11:31Z

    On Sat, 23 Mar 2024 at 00:53, Andres Freund <andres@anarazel.de> wrote:
> Hi,
>
> On 2024-03-23 00:48:05 +0800, Japin Li wrote:
>> When I try to configure PostgreSQL 16.2 on Illumos using the following command,
>> it complains $subject.
>>
>>     ./configure --enable-cassert --enable-debug --enable-nls --with-perl \
>>       --with-python --without-tcl --without-gssapi --with-openssl \
>>       --with-ldap --with-libxml --with-libxslt --without-systemd \
>>       --with-readline --enable-thread-safety --enable-dtrace \
>>       DTRACEFLAGS=-64 CFLAGS=-Werror
>>
>> However, if I remove the `CFLAGS=-Werror`, it works fine.
>
> Likely there's an unrelated warning triggering the configure test to
> fail. We'd need to see config.log to see what that is.
>

Thanks for your quick reply. Attach the config.log.

  5. Re: Cannot find a working 64-bit integer type on Illumos

    Japin Li <japinli@hotmail.com> — 2024-03-22T17:22:56Z

    On Sat, 23 Mar 2024 at 01:04, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Japin Li <japinli@hotmail.com> writes:
>> When I try to configure PostgreSQL 16.2 on Illumos using the following command,
>> it complains $subject.
>
>>     ./configure --enable-cassert --enable-debug --enable-nls --with-perl \
>>       --with-python --without-tcl --without-gssapi --with-openssl \
>>       --with-ldap --with-libxml --with-libxslt --without-systemd \
>>       --with-readline --enable-thread-safety --enable-dtrace \
>>       DTRACEFLAGS=-64 CFLAGS=-Werror
>
>> However, if I remove the `CFLAGS=-Werror`, it works fine.
>> I'm not sure what happened here.
>
> CFLAGS=-Werror breaks a whole lot of configure's tests, not only that
> one.  (We even have this documented, see [1].)  So you can't inject
> -Werror that way.  What I do on my buildfarm animals is the equivalent
> of
>
> 	export COPT='-Werror'
>
> after configure and before build.  I think configure pays no attention
> to COPT, so it'd likely be safe to keep that set all the time, but in
> the buildfarm client it's just as easy to be conservative.
>
> 			regards, tom lane
>
> [1] https://www.postgresql.org/docs/devel/install-make.html#CONFIGURE-ENVVARS

Thank you very much!  I didn't notice this part before.

  6. Re: Cannot find a working 64-bit integer type on Illumos

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-03-22T17:25:56Z

    Japin Li <japinli@hotmail.com> writes:
> On Sat, 23 Mar 2024 at 00:53, Andres Freund <andres@anarazel.de> wrote:
>> Likely there's an unrelated warning triggering the configure test to
>> fail. We'd need to see config.log to see what that is.

> Thanks for your quick reply. Attach the config.log.

Yup:

conftest.c:139:5: error: no previous prototype for 'does_int64_work' [-Werror=missing-prototypes]
  139 | int does_int64_work()
      |     ^~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
configure:17003: $? = 1
configure: program exited with status 1

This warning is harmless normally, but breaks the configure probe if
you enable -Werror.

No doubt we could improve that test snippet so that it does not
trigger that warning.  But trying to make configure safe for -Werror
seems like a fool's errand, for these reasons:

* Do you really want to try to make all of configure's probes proof
against every compiler warning everywhere?

* Many of the test snippets aren't readily under our control, as they
are supplied by Autoconf.

* In the majority of cases, any such failures would be silent, as
configure would just conclude that the feature it is probing for
isn't there.  So even finding there's a problem would be difficult.

The short answer is that Autoconf is not designed to support -Werror
and it's not worth it to try to make it do so.

			regards, tom lane

  7. Re: Cannot find a working 64-bit integer type on Illumos

    Andres Freund <andres@anarazel.de> — 2024-03-22T18:38:26Z

    Hi,

On 2024-03-22 13:25:56 -0400, Tom Lane wrote:
> The short answer is that Autoconf is not designed to support -Werror
> and it's not worth it to try to make it do so.

I wonder if we ought to make configure warn if it sees -Werror in CFLAGS -
this is far from the first time somebody stumbling with -Werror. Including a
few quite senior hackers, if I recall correctly.  We could also just filter it
temporarily and put it back at the end of configure.

I don't think there's great way of making the autoconf buildsystem use -Werror
continually, today. IIRC the best way is to use Makefile.custom.

Greetings,

Andres Freund

  8. Re: Cannot find a working 64-bit integer type on Illumos

    Robert Haas <robertmhaas@gmail.com> — 2024-03-22T19:02:45Z

    On Fri, Mar 22, 2024 at 2:38 PM Andres Freund <andres@anarazel.de> wrote:
> I wonder if we ought to make configure warn if it sees -Werror in CFLAGS -
> this is far from the first time somebody stumbling with -Werror. Including a
> few quite senior hackers, if I recall correctly.  We could also just filter it
> temporarily and put it back at the end of configure.

I think I made this mistake at some point, but I just looked at
config.log and corrected my mistake. I'm not strongly against having
an explicit check for -Werror, but I think the main problem here is
that the original poster didn't have a look at config.log to see what
the actual problem was, and at least IME that's necessary in pretty
much 100% of cases where configure fails for whatever reason. Perhaps
autotools could be better-designed in that regard, but we don't
necessarily want to work around every problem that can stem from that
design choice in our code, either.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  9. Re: Cannot find a working 64-bit integer type on Illumos

    Andres Freund <andres@anarazel.de> — 2024-03-22T19:31:22Z

    Hi,

On 2024-03-22 15:02:45 -0400, Robert Haas wrote:
> On Fri, Mar 22, 2024 at 2:38 PM Andres Freund <andres@anarazel.de> wrote:
> > I wonder if we ought to make configure warn if it sees -Werror in CFLAGS -
> > this is far from the first time somebody stumbling with -Werror. Including a
> > few quite senior hackers, if I recall correctly.  We could also just filter it
> > temporarily and put it back at the end of configure.
> 
> I think I made this mistake at some point, but I just looked at
> config.log and corrected my mistake.

IME the bigger issue is that sometimes it doesn't lead to outright failures,
just to lots of stuff not being detected as supported / present.

Greetings,

Andres Freund

  10. Re: Cannot find a working 64-bit integer type on Illumos

    Robert Haas <robertmhaas@gmail.com> — 2024-03-22T19:34:56Z

    On Fri, Mar 22, 2024 at 3:31 PM Andres Freund <andres@anarazel.de> wrote:
> IME the bigger issue is that sometimes it doesn't lead to outright failures,
> just to lots of stuff not being detected as supported / present.

Ugh. That does, indeed, sound not very nice.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  11. Re: Cannot find a working 64-bit integer type on Illumos

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-03-22T20:43:33Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Mar 22, 2024 at 3:31 PM Andres Freund <andres@anarazel.de> wrote:
>> IME the bigger issue is that sometimes it doesn't lead to outright failures,
>> just to lots of stuff not being detected as supported / present.

> Ugh. That does, indeed, sound not very nice.

I could get behind throwing an error if -Werror is spotted.  I think
trying to pull it out and put it back is too much work and a bit
too much magic.

			regards, tom lane

  12. Re: Cannot find a working 64-bit integer type on Illumos

    Thomas Munro <thomas.munro@gmail.com> — 2024-03-23T00:45:28Z

    On Sat, Mar 23, 2024 at 6:26 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> conftest.c:139:5: error: no previous prototype for 'does_int64_work' [-Werror=missing-prototypes]
>   139 | int does_int64_work()
>       |     ^~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors
> configure:17003: $? = 1
> configure: program exited with status 1

. o O ( int64_t, PRIdi64, etc were standardised a quarter of a century ago )

  13. Re: Cannot find a working 64-bit integer type on Illumos

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-03-23T02:23:13Z

    Thomas Munro <thomas.munro@gmail.com> writes:
> . o O ( int64_t, PRIdi64, etc were standardised a quarter of a century ago )

Yeah.  Now that we require C99 it's probably reasonable to assume
that those things exist.  I wouldn't be in favor of ripping out our
existing notations like UINT64CONST, because the code churn would be
substantial and the gain minimal.  But we could imagine reimplementing
that stuff atop <stdint.h> and then getting rid of the configure-time
probes.

			regards, tom lane

  14. Re: Cannot find a working 64-bit integer type on Illumos

    Japin Li <japinli@hotmail.com> — 2024-03-25T01:24:58Z

    On Sat, 23 Mar 2024 at 01:22, Japin Li <japinli@hotmail.com> wrote:
> On Sat, 23 Mar 2024 at 01:04, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Japin Li <japinli@hotmail.com> writes:
>>> When I try to configure PostgreSQL 16.2 on Illumos using the following command,
>>> it complains $subject.
>>
>>>     ./configure --enable-cassert --enable-debug --enable-nls --with-perl \
>>>       --with-python --without-tcl --without-gssapi --with-openssl \
>>>       --with-ldap --with-libxml --with-libxslt --without-systemd \
>>>       --with-readline --enable-thread-safety --enable-dtrace \
>>>       DTRACEFLAGS=-64 CFLAGS=-Werror
>>
>>> However, if I remove the `CFLAGS=-Werror`, it works fine.
>>> I'm not sure what happened here.
>>
>> CFLAGS=-Werror breaks a whole lot of configure's tests, not only that
>> one.  (We even have this documented, see [1].)  So you can't inject
>> -Werror that way.  What I do on my buildfarm animals is the equivalent
>> of
>>
>> 	export COPT='-Werror'
>>
>> after configure and before build.  I think configure pays no attention
>> to COPT, so it'd likely be safe to keep that set all the time, but in
>> the buildfarm client it's just as easy to be conservative.
>>
>> 			regards, tom lane
>>
>> [1] https://www.postgresql.org/docs/devel/install-make.html#CONFIGURE-ENVVARS
>
> Thank you very much!  I didn't notice this part before.

I try to use the following to compile it, however, it cannot compile it.

$ ../configure --enable-cassert --enable-debug --enable-nls --with-perl --with-python --without-tcl --without-gssapi --with-openssl --with-ldap --with-libxml --with-libxslt --without-systemd --with-readline --enable-thread-safety --enable-dtrace DTRACEFLAGS=-64
$ make COPT='-Werror' -s
/home/japin/postgres/debug/../src/bin/pg_dump/pg_dump_sort.c: In function 'repairDependencyLoop':
/home/japin/postgres/debug/../src/bin/pg_dump/pg_dump_sort.c:1276:3: error: format not a string literal and no format arguments [-Werror=format-security]
 1276 |   pg_log_warning(ngettext("there are circular foreign-key constraints on this table:",
      |   ^~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make[3]: *** [<builtin>: pg_dump_sort.o] Error 1
make[2]: *** [Makefile:43: all-pg_dump-recurse] Error 2
make[1]: *** [Makefile:42: all-bin-recurse] Error 2
make: *** [GNUmakefile:11: all-src-recurse] Error 2

  15. Re: Cannot find a working 64-bit integer type on Illumos

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-03-25T01:32:17Z

    Japin Li <japinli@hotmail.com> writes:
> /home/japin/postgres/debug/../src/bin/pg_dump/pg_dump_sort.c: In function 'repairDependencyLoop':
> /home/japin/postgres/debug/../src/bin/pg_dump/pg_dump_sort.c:1276:3: error: format not a string literal and no format arguments [-Werror=format-security]
>  1276 |   pg_log_warning(ngettext("there are circular foreign-key constraints on this table:",
>       |   ^~~~~~~~~~~~~~

Yeah, some of the older buildfarm animals issue that warning too.
AFAICS it's a bogus compiler heuristic: there is not anything
wrong with the code as given.

			regards, tom lane

  16. Re: Cannot find a working 64-bit integer type on Illumos

    Japin Li <japinli@hotmail.com> — 2024-03-25T01:38:41Z

    On Mon, 25 Mar 2024 at 09:32, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Japin Li <japinli@hotmail.com> writes:
>> /home/japin/postgres/debug/../src/bin/pg_dump/pg_dump_sort.c: In function 'repairDependencyLoop':
>> /home/japin/postgres/debug/../src/bin/pg_dump/pg_dump_sort.c:1276:3: error: format not a string literal and no format arguments [-Werror=format-security]
>>  1276 |   pg_log_warning(ngettext("there are circular foreign-key constraints on this table:",
>>       |   ^~~~~~~~~~~~~~
>
> Yeah, some of the older buildfarm animals issue that warning too.
> AFAICS it's a bogus compiler heuristic: there is not anything
> wrong with the code as given.
>

Thanks!  It seems I should remove -Werror option on Illumos.

  17. Re: Cannot find a working 64-bit integer type on Illumos

    Thomas Munro <thomas.munro@gmail.com> — 2024-04-18T00:31:05Z

    On Sat, Mar 23, 2024 at 3:23 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Thomas Munro <thomas.munro@gmail.com> writes:
> > . o O ( int64_t, PRIdi64, etc were standardised a quarter of a century ago )
>
> Yeah.  Now that we require C99 it's probably reasonable to assume
> that those things exist.  I wouldn't be in favor of ripping out our
> existing notations like UINT64CONST, because the code churn would be
> substantial and the gain minimal.  But we could imagine reimplementing
> that stuff atop <stdint.h> and then getting rid of the configure-time
> probes.

I played around with this a bit, but am not quite there yet.

printf() is a little tricky.  The standard wants us to use
<inttypes.h>'s PRId64 etc, but that might confuse our snprintf.c (in
theory, probably not in practice).  "ll" should have the right size on
all systems, but gets warnings from the printf format string checker
on systems where "l" is the right type.  So I think we still need to
probe for INT64_MODIFIER at configure-time.  Here's one way, but I can
see it's not working on Clang/Linux... perhaps instead of that dubious
incantation I should try compiling some actual printfs and check for
warnings/errors.

I think INT64CONST should just point to standard INT64_C().

For limits, why do we have this:

- * stdint.h limits aren't guaranteed to have compatible types with our fixed
- * width types. So just define our own.

?  I mean, how could they not have compatible types?

I noticed that configure.ac checks if int64 (no "_t") might be defined
already by system header pollution, but meson.build doesn't.  That's
an inconsistency that should be fixed, but which way?  Hmm, commit
15abc7788e6 said that was done for BeOS, which we de-supported.  So
maybe we should get rid of that?

  18. Re: Cannot find a working 64-bit integer type on Illumos

    Japin Li <japinli@hotmail.com> — 2024-04-18T06:07:22Z

    On Thu, 18 Apr 2024 at 08:31, Thomas Munro <thomas.munro@gmail.com> wrote:
> On Sat, Mar 23, 2024 at 3:23 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Thomas Munro <thomas.munro@gmail.com> writes:
>> > . o O ( int64_t, PRIdi64, etc were standardised a quarter of a century ago )
>>
>> Yeah.  Now that we require C99 it's probably reasonable to assume
>> that those things exist.  I wouldn't be in favor of ripping out our
>> existing notations like UINT64CONST, because the code churn would be
>> substantial and the gain minimal.  But we could imagine reimplementing
>> that stuff atop <stdint.h> and then getting rid of the configure-time
>> probes.
>
> I played around with this a bit, but am not quite there yet.
>
> printf() is a little tricky.  The standard wants us to use
> <inttypes.h>'s PRId64 etc, but that might confuse our snprintf.c (in
> theory, probably not in practice).  "ll" should have the right size on
> all systems, but gets warnings from the printf format string checker
> on systems where "l" is the right type.  So I think we still need to
> probe for INT64_MODIFIER at configure-time.  Here's one way, but I can
> see it's not working on Clang/Linux... perhaps instead of that dubious
> incantation I should try compiling some actual printfs and check for
> warnings/errors.
>
> I think INT64CONST should just point to standard INT64_C().
>
> For limits, why do we have this:
>
> - * stdint.h limits aren't guaranteed to have compatible types with our fixed
> - * width types. So just define our own.
>
> ?  I mean, how could they not have compatible types?
>
> I noticed that configure.ac checks if int64 (no "_t") might be defined
> already by system header pollution, but meson.build doesn't.  That's
> an inconsistency that should be fixed, but which way?  Hmm, commit
> 15abc7788e6 said that was done for BeOS, which we de-supported.  So
> maybe we should get rid of that?

Thanks for working on this!  I test the patch and it now works on Illumos when
configure with -Werror option.  However, there are some errors when compiling.

In file included from /home/japin/postgres/build/../src/include/c.h:834,
                 from /home/japin/postgres/build/../src/include/postgres_fe.h:25,
                 from /home/japin/postgres/build/../src/common/config_info.c:20:
/home/japin/postgres/build/../src/common/config_info.c: In function 'get_configdata':
/home/japin/postgres/build/../src/common/config_info.c:198:11: error: comparison of integer expressions of different signedness: 'int' and 'size_t' {aka 'long unsigned int'} [-Werror=sign-compare]
  198 |  Assert(i == *configdata_len);
      |           ^~
/home/japin/postgres/build/../src/common/config_info.c:198:2: note: in expansion of macro 'Assert'
  198 |  Assert(i == *configdata_len);
      |  ^~~~~~
In file included from /home/japin/postgres/build/../src/common/blkreftable.c:36:
/home/japin/postgres/build/../src/include/lib/simplehash.h: In function 'blockreftable_stat':
/home/japin/postgres/build/../src/include/lib/simplehash.h:1138:9: error: format '%llu' expects argument of type 'long long unsigned int', but argument 4 has type 'uint64' {aka 'long unsigned int'} [-Werror=format=]
 1138 |  sh_log("size: " UINT64_FORMAT ", members: %u, filled: %f, total chain: %u, max chain: %u, avg chain: %f, total_collisions: %u, max_collisions: %u, avg_collisions: %f",
      |         ^~~~~~~~
 1139 |      tb->size, tb->members, fillfactor, total_chain_length, max_chain_length, avg_chain_length,
      |      ~~~~~~~~
      |        |
      |        uint64 {aka long unsigned int}
/home/japin/postgres/build/../src/include/common/logging.h:125:46: note: in definition of macro 'pg_log_info'
  125 |  pg_log_generic(PG_LOG_INFO, PG_LOG_PRIMARY, __VA_ARGS__)
      |                                              ^~~~~~~~~~~
/home/japin/postgres/build/../src/include/lib/simplehash.h:1138:2: note: in expansion of macro 'sh_log'
 1138 |  sh_log("size: " UINT64_FORMAT ", members: %u, filled: %f, total chain: %u, max chain: %u, avg chain: %f, total_collisions: %u, max_collisions: %u, avg_collisions: %f",
      |  ^~~~~~
In file included from /home/japin/postgres/build/../src/include/access/xlogrecord.h:14,
                 from /home/japin/postgres/build/../src/include/access/xlogreader.h:41,
                 from /home/japin/postgres/build/../src/include/access/xlog_internal.h:23,
                 from /home/japin/postgres/build/../src/common/controldata_utils.c:28:
/home/japin/postgres/build/../src/include/access/rmgr.h: In function 'RmgrIdIsCustom':
/home/japin/postgres/build/../src/include/access/rmgr.h:50:42: error: comparison of integer expressions of different signedness: 'int' and 'unsigned int' [-Werror=sign-compare]
   50 |  return rmid >= RM_MIN_CUSTOM_ID && rmid <= RM_MAX_CUSTOM_ID;
      |                                          ^~
/home/japin/postgres/build/../src/common/blkreftable.c: In function 'BlockRefTableReaderGetBlocks':
/home/japin/postgres/build/../src/common/blkreftable.c:716:22: error: comparison of integer expressions of different signedness: 'unsigned int' and 'int' [-Werror=sign-compare]
  716 |         blocks_found < nblocks)
      |                      ^
/home/japin/postgres/build/../src/common/blkreftable.c:732:22: error: comparison of integer expressions of different signedness: 'unsigned int' and 'int' [-Werror=sign-compare]
  732 |         blocks_found < nblocks)
      |                      ^
/home/japin/postgres/build/../src/common/blkreftable.c:742:20: error: comparison of integer expressions of different signedness: 'unsigned int' and 'int' [-Werror=sign-compare]
  742 |   if (blocks_found >= nblocks)
      |                    ^~
cc1: all warnings being treated as errors
cc1: all warnings being treated as errors
make[2]: *** [../../src/Makefile.global:947: config_info.o] Error 1
make[2]: *** Waiting for unfinished jobs....
make[2]: *** [../../src/Makefile.global:947: controldata_utils.o] Error 1
In file included from /home/japin/postgres/build/../src/include/postgres_fe.h:25,
                 from /home/japin/postgres/build/../src/common/logging.c:15:
/home/japin/postgres/build/../src/common/logging.c: In function 'pg_log_generic_v':
/home/japin/postgres/build/../src/include/c.h:523:23: error: format '%llu' expects argument of type 'long long unsigned int', but argument 3 has type 'uint64' {aka 'long unsigned int'} [-Werror=format=]
  523 | #define UINT64_FORMAT "%" INT64_MODIFIER "u"
      |                       ^~~
/home/japin/postgres/build/../src/common/logging.c:259:21: note: in expansion of macro 'UINT64_FORMAT'
  259 |     fprintf(stderr, UINT64_FORMAT ":", lineno);
      |                     ^~~~~~~~~~~~~
/home/japin/postgres/build/../src/include/c.h:523:43: note: format string is defined here
  523 | #define UINT64_FORMAT "%" INT64_MODIFIER "u"
      |                        ~~~~~~~~~~~~~~~~~~~^
      |                                           |
      |                                           long long unsigned int
/home/japin/postgres/build/../src/common/unicode_norm.c: In function 'recompose_code':
/home/japin/postgres/build/../src/common/unicode_norm.c:290:17: error: comparison of integer expressions of different signedness: 'int' and 'long unsigned int' [-Werror=sign-compare]
  290 |   for (i = 0; i < lengthof(UnicodeDecompMain); i++)
      |                 ^
In file included from /home/japin/postgres/build/../src/include/c.h:834,
                 from /home/japin/postgres/build/../src/common/encnames.c:13:
/home/japin/postgres/build/../src/common/encnames.c: In function 'pg_encoding_to_char_private':
/home/japin/postgres/build/../src/common/encnames.c:593:19: error: comparison of integer expressions of different signedness: 'int' and 'pg_enc' [-Werror=sign-compare]
  593 |   Assert(encoding == p->encoding);
      |                   ^~
/home/japin/postgres/build/../src/common/encnames.c:593:3: note: in expansion of macro 'Assert'
  593 |   Assert(encoding == p->encoding);
      |   ^~~~~~
/home/japin/postgres/build/../src/common/jsonapi.c: In function 'pg_parse_json_incremental':
/home/japin/postgres/build/../src/common/jsonapi.c:693:11: error: comparison of integer expressions of different signedness: 'char' and 'JsonTokenType' [-Werror=sign-compare]
  693 |   if (top == tok)
      |           ^~
cc1: all warnings being treated as errors
cc1: all warnings being treated as errors
make[2]: *** [../../src/Makefile.global:947: logging.o] Error 1
make[2]: *** [../../src/Makefile.global:947: blkreftable.o] Error 1
/home/japin/postgres/build/../src/common/kwlookup.c: In function 'ScanKeywordLookup':
/home/japin/postgres/build/../src/common/kwlookup.c:50:10: error: comparison of integer expressions of different signedness: 'size_t' {aka 'long unsigned int'} and 'int' [-Werror=sign-compare]
   50 |  if (len > keywords->max_kw_len)
      |          ^
cc1: all warnings being treated as errors
make[2]: *** [../../src/Makefile.global:947: encnames.o] Error 1
cc1: all warnings being treated as errors
cc1: all warnings being treated as errors
make[2]: *** [../../src/Makefile.global:947: kwlookup.o] Error 1
In file included from /home/japin/postgres/build/../src/include/c.h:834,
                 from /home/japin/postgres/build/../src/include/postgres_fe.h:25,
                 from /home/japin/postgres/build/../src/common/file_utils.c:19:
/home/japin/postgres/build/../src/common/file_utils.c: In function 'pg_pwrite_zeros':
/home/japin/postgres/build/../src/common/file_utils.c:725:23: error: comparison of integer expressions of different signedness: 'ssize_t' {aka 'long int'} and 'size_t' {aka 'long unsigned int'} [-Werror=sign-compare]
  725 |  Assert(total_written == size);
      |                       ^~
/home/japin/postgres/build/../src/common/file_utils.c:725:2: note: in expansion of macro 'Assert'
  725 |  Assert(total_written == size);
      |  ^~~~~~
make[2]: *** [../../src/Makefile.global:947: unicode_norm.o] Error 1
cc1: all warnings being treated as errors
make[2]: *** [../../src/Makefile.global:947: file_utils.o] Error 1
/home/japin/postgres/build/../src/common/unicode_case.c: In function 'convert_case':
/home/japin/postgres/build/../src/common/unicode_case.c:155:31: error: comparison of integer expressions of different signedness: 'size_t' {aka 'long unsigned int'} and 'ssize_t' {aka 'long int'} [-Werror=sign-compare]
  155 |  while ((srclen < 0 || srcoff < srclen) && src[srcoff] != '\0')
      |                               ^
/home/japin/postgres/build/../src/common/wchar.c: In function 'pg_utf8_verifystr':
/home/japin/postgres/build/../src/common/wchar.c:1868:10: error: comparison of integer expressions of different signedness: 'int' and 'long unsigned int' [-Werror=sign-compare]
 1868 |  if (len >= STRIDE_LENGTH)
      |          ^~
/home/japin/postgres/build/../src/common/wchar.c:1870:14: error: comparison of integer expressions of different signedness: 'int' and 'long unsigned int' [-Werror=sign-compare]
 1870 |   while (len >= STRIDE_LENGTH)
      |              ^~
cc1: all warnings being treated as errors
make[2]: *** [../../src/Makefile.global:947: unicode_case.o] Error 1
cc1: all warnings being treated as errors
make[2]: *** [../../src/Makefile.global:947: jsonapi.o] Error 1
cc1: all warnings being treated as errors
make[2]: *** [../../src/Makefile.global:947: wchar.o] Error 1
make[1]: *** [Makefile:42: all-common-recurse] Error 2
make: *** [GNUmakefile:11: all-src-recurse] Error 2

For rmid >= RM_MIN_CUSTOM_ID && rmid <= RM_MAX_CUSTOM_ID comparison error, I
found that UINT8_MAX is defined as '255U' on Illumos, however, Linux glibc
uses '255' for UINT8_MAX, which is signed.

[1] https://github.com/illumos/illumos-gate/blob/master/usr/src/uts/common/sys/int_limits.h#L92
[2] https://sourceware.org/git/?p=glibc.git;a=blob;f=stdlib/stdint.h;h=bb3e8b5cc61fb3df8842225d2286de67e6f2ffe2;hb=refs/heads/master#l116


--
Regards,
Japin Li

  19. Re: Cannot find a working 64-bit integer type on Illumos

    Peter Eisentraut <peter@eisentraut.org> — 2024-04-18T08:46:57Z

    On 18.04.24 02:31, Thomas Munro wrote:
> On Sat, Mar 23, 2024 at 3:23 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Thomas Munro <thomas.munro@gmail.com> writes:
>>> . o O ( int64_t, PRIdi64, etc were standardised a quarter of a century ago )
>>
>> Yeah.  Now that we require C99 it's probably reasonable to assume
>> that those things exist.  I wouldn't be in favor of ripping out our
>> existing notations like UINT64CONST, because the code churn would be
>> substantial and the gain minimal.  But we could imagine reimplementing
>> that stuff atop <stdint.h> and then getting rid of the configure-time
>> probes.
> 
> I played around with this a bit, but am not quite there yet.

Looks promising.

> printf() is a little tricky.  The standard wants us to use
> <inttypes.h>'s PRId64 etc, but that might confuse our snprintf.c (in
> theory, probably not in practice).  "ll" should have the right size on
> all systems, but gets warnings from the printf format string checker
> on systems where "l" is the right type.

I'm not sure I understand the problem here.  Do you mean that in theory 
a platform's PRId64 could be something other than "l" or "ll"?

> For limits, why do we have this:
> 
> - * stdint.h limits aren't guaranteed to have compatible types with our fixed
> - * width types. So just define our own.
> 
> ?  I mean, how could they not have compatible types?

Maybe this means something like our int64 is long long int but the 
system's int64_t is long int underneath, but I don't see how that would 
matter for the limit macros.

> I noticed that configure.ac checks if int64 (no "_t") might be defined
> already by system header pollution, but meson.build doesn't.  That's
> an inconsistency that should be fixed, but which way?  Hmm, commit
> 15abc7788e6 said that was done for BeOS, which we de-supported.  So
> maybe we should get rid of that?

I had a vague recollection that it was for AIX, but the commit indeed 
mentions BeOS.  Could be removed in either case.

  20. Re: Cannot find a working 64-bit integer type on Illumos

    Thomas Munro <thomas.munro@gmail.com> — 2024-04-18T20:29:27Z

    On Thu, Apr 18, 2024 at 8:47 PM Peter Eisentraut <peter@eisentraut.org> wrote:
> I'm not sure I understand the problem here.  Do you mean that in theory
> a platform's PRId64 could be something other than "l" or "ll"?

Yes.  I don't know why anyone would do that, and the systems I checked
all have the obvious definitions, eg "ld", "lld" etc.  Perhaps it's an
acceptable risk?  It certainly gives us a tidier result.

For discussion, here is a variant that fully embraces <inttypes.h> and
the PRI*64 macros.

  21. Re: Cannot find a working 64-bit integer type on Illumos

    Thomas Munro <thomas.munro@gmail.com> — 2024-04-18T21:00:05Z

    On Thu, Apr 18, 2024 at 8:47 PM Peter Eisentraut <peter@eisentraut.org> wrote:
> Maybe this means something like our int64 is long long int but the
> system's int64_t is long int underneath, but I don't see how that would
> matter for the limit macros.

Agreed, so I don't think it's long vs long long (when they have the same width).

I wonder if this comment is a clue:

static char *
inet_net_ntop_ipv6(const u_char *src, int bits, char *dst, size_t size)
{
    /*
     * Note that int32_t and int16_t need only be "at least" large enough to
     * contain a value of the specified size.  On some systems, like Crays,
     * there is no such thing as an integer variable with 16 bits. Keep this
     * in mind if you think this function should have been coded to use
     * pointer overlays.  All the world's not a VAX.
     */

I'd seen that claim before somewhere else but I can't recall where.
So there were systems using those names in an ad hoc unspecified way
before C99 nailed this stuff down?  In modern C, int32_t is definitely
an exact width type (but there are other standardised variants like
int_fast32_t to allow for Cray-like systems that would prefer to use a
wider type, ie "at least", 32 bits wide, so I guess that's what
happened to that idea?).

Or perhaps it's referring to worries about the width of char, short,
int or the assumption of two's-complement.  I think if any of that
stuff weren't as assumed we'd have many problems in many places, so
I'm not seeing a problem.  (FTR C23 finally nailed down
two's-complement as a requirement, and although C might not say so,
POSIX says that char is a byte, and our assumption that int = int32_t
is pretty deeply baked into PostgreSQL, so it's almost impossible to
imagine that short has a size other than 16 bits; but these are all
assumptions made by the OLD coding, not by the patch I posted).  In
short, I guess that isn't what was meant.

  22. Re: Cannot find a working 64-bit integer type on Illumos

    Thomas Munro <thomas.munro@gmail.com> — 2024-04-18T21:22:10Z

    On Thu, Apr 18, 2024 at 6:09 PM Japin Li <japinli@hotmail.com> wrote:
> /home/japin/postgres/build/../src/common/config_info.c:198:11: error: comparison of integer expressions of different signedness: 'int' and 'size_t' {aka 'long unsigned int'} [-Werror=sign-compare]
>   198 |  Assert(i == *configdata_len);

Right, PostgreSQL doesn't compile cleanly with the "sign-compare"
warning.  There have been a few threads about that, and someone might
want to think harder about it, but it's a different topic unrelated to
<stdint.h>.

> /home/japin/postgres/build/../src/include/lib/simplehash.h:1138:9: error: format '%llu' expects argument of type 'long long unsigned int', but argument 4 has type 'uint64' {aka 'long unsigned int'} [-Werror=format=]

It seems my v1 patch's configure probe for INT64_FORMAT was broken.
In the v2 patch I tried not doing that probe at all, and instead
inviting <inttypes.h> into our world (that's the standardised way to
produce format strings, which has the slight complication that we are
intercepting printf calls...).  I suspect that'll work better for you.

  23. Re: Cannot find a working 64-bit integer type on Illumos

    Japin Li <japinli@hotmail.com> — 2024-04-19T05:25:05Z

    On Fri, 19 Apr 2024 at 05:22, Thomas Munro <thomas.munro@gmail.com> wrote:
> On Thu, Apr 18, 2024 at 6:09 PM Japin Li <japinli@hotmail.com> wrote:
>> /home/japin/postgres/build/../src/include/lib/simplehash.h:1138:9: error: format '%llu' expects argument of type 'long long unsigned int', but argument 4 has type 'uint64' {aka 'long unsigned int'} [-Werror=format=]
>
> It seems my v1 patch's configure probe for INT64_FORMAT was broken.
> In the v2 patch I tried not doing that probe at all, and instead
> inviting <inttypes.h> into our world (that's the standardised way to
> produce format strings, which has the slight complication that we are
> intercepting printf calls...).  I suspect that'll work better for you.

Yeah, the v2 patch fixed this problem.

--
Regards,
Japin Li

  24. Re: Cannot find a working 64-bit integer type on Illumos

    Peter Eisentraut <peter@eisentraut.org> — 2024-04-19T08:34:38Z

    On 18.04.24 02:31, Thomas Munro wrote:
> For limits, why do we have this:
> 
> - * stdint.h limits aren't guaranteed to have compatible types with our fixed
> - * width types. So just define our own.
> 
> ?  I mean, how could they not have compatible types?

The commit for this was 62e2a8dc2c7 and the thread was 
<https://www.postgresql.org/message-id/flat/E1YatAv-0007cu-KW%40gemulon.postgresql.org>. 
  The problem was that something like

     snprintf(bufm, sizeof(bufm), INT64_FORMAT, SEQ_MINVALUE);

could issue a warning if, say, INT64_FORMAT, which refers to our own 
int64, is based on long int, but SEQ_MINVALUE, which was then INT64_MIN, 
which refers to int64_t, which could be long long int.

So this is correct.  If we introduce the use of int64_t, then you need 
to be consistent still:

int64, PG_INT64_MIN, PG_INT64_MAX, INT64_FORMAT

int64_t, INT64_MIN, INT64_MAX, PRId64

  25. Re: Cannot find a working 64-bit integer type on Illumos

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-07-02T13:34:57Z

    On 18/04/2024 23:29, Thomas Munro wrote:
> On Thu, Apr 18, 2024 at 8:47 PM Peter Eisentraut <peter@eisentraut.org> wrote:
>> I'm not sure I understand the problem here.  Do you mean that in theory
>> a platform's PRId64 could be something other than "l" or "ll"?
> 
> Yes.  I don't know why anyone would do that, and the systems I checked
> all have the obvious definitions, eg "ld", "lld" etc.  Perhaps it's an
> acceptable risk?  It certainly gives us a tidier result.

Could we have a configure check or static assertion for that?

> For discussion, here is a variant that fully embraces <inttypes.h> and
> the PRI*64 macros.

Looks good to me.

Personally, I find "PRId64" pretty unreadable. "INT64_MODIFIER" wasn't 
nice either, though, and following standards is good, so I'm sure I'll 
get used to it.

They're both less readable than INT64_FORMAT and "%lld", which we use in 
most places, though. Perhaps "%lld" and casting the arguments to "long 
long" would be more readable in the places where this patch replaces 
INT64_MODIFIER with PRI*64, too.

-- 
Heikki Linnakangas
Neon (https://neon.tech)

  26. Re: Cannot find a working 64-bit integer type on Illumos

    Thomas Munro <thomas.munro@gmail.com> — 2024-07-04T02:55:59Z

    On Wed, Jul 3, 2024 at 1:34 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> On 18/04/2024 23:29, Thomas Munro wrote:
> > On Thu, Apr 18, 2024 at 8:47 PM Peter Eisentraut <peter@eisentraut.org> wrote:
> >> I'm not sure I understand the problem here.  Do you mean that in theory
> >> a platform's PRId64 could be something other than "l" or "ll"?
> >
> > Yes.  I don't know why anyone would do that, and the systems I checked
> > all have the obvious definitions, eg "ld", "lld" etc.  Perhaps it's an
> > acceptable risk?  It certainly gives us a tidier result.
>
> Could we have a configure check or static assertion for that?

Unfortunately, that theory turned out to be wrong.  The usual suspect,
Windows, uses something else: "I64" or something like that.  We could
teach our snprintf to grok that, but I don't like the idea anymore.
So let's go back to INT64_MODIFIER, with just a small amount of
configure time work to pick the right value.  I couldn't figure out
any header-only way to do that.

> Personally, I find "PRId64" pretty unreadable. "INT64_MODIFIER" wasn't
> nice either, though, and following standards is good, so I'm sure I'll
> get used to it.

Yeah, I like standards a lot but we've painted ourselves into a corner here...

New version attached.  This time I was brave enough to try to tackle
src/timezone too, which had comments planning to drop a lot of small
differences against the upstream tzcode once all supported branches
required C99.  I suppose that should make future maintenance easier,
and C89 disappeared from our window of interest with PostgreSQL 11.
It's a little hard to understand what changed, but to try to show it
better I diff'd master against upstream (after filtering through sed
and pgindent as recommended by README), and then diff'd patched
against upstream, and then ... ehm.. diff'd the two diffs, so that you
can see there are some hunks that go away.

IMHO it's a rather scary choice on tzcode's part to use int_fastN_t,
and hard for us to verify that it works correctly especially when
combined with our changes, but on the other hand I don't really expect
any system that PostgreSQL can run on to have "fast" types that really
differ from the "exact width" types.  My understanding is that this is
something of interest to historical supercomputers and
microcontrollers, but I can't find any evidence of general
purpose/commodity systems that we target using different sizes (anyone
know better?).

  27. Re: Cannot find a working 64-bit integer type on Illumos

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-07-04T03:10:34Z

    Thomas Munro <thomas.munro@gmail.com> writes:
> New version attached.  This time I was brave enough to try to tackle
> src/timezone too, which had comments planning to drop a lot of small
> differences against the upstream tzcode once all supported branches
> required C99.

Unless you've specifically checked that this reduces diffs against
upstream tzcode, I'd really prefer not to touch that code right now.
I know I'm overdue for a round of syncing src/timezone/ with upstream,
but I can't see how drive-by changes will make that easier.

> IMHO it's a rather scary choice on tzcode's part to use int_fastN_t,

Yeah, I was never pleased with that choice of theirs.  OTOH, I've
seen darn few portability complaints on their mailing list, so
it seems like they've got it right in isolation.  The problem
from our standpoint is that I don't think we want int_fastN_t
to leak into APIs visible to the rest of Postgres, because then
we risk issues related to their configuration methods being
totally unlike ours.

			regards, tom lane

  28. Re: Cannot find a working 64-bit integer type on Illumos

    Thomas Munro <thomas.munro@gmail.com> — 2024-07-04T03:44:06Z

    On Thu, Jul 4, 2024 at 3:10 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Unless you've specifically checked that this reduces diffs against
> upstream tzcode, I'd really prefer not to touch that code right now.
> I know I'm overdue for a round of syncing src/timezone/ with upstream,
> but I can't see how drive-by changes will make that easier.

Sure, I'll wait until you say it's a good time.  It does remove a
dozen or so hunks of difference, which should hopefully make that job
easier eventually but I don't want to get in your way.  I can see
there are a few more trivialities we could synchronise on, like const
keywords, to kill useless diffs (either dropping local improvements or
sending patches upstream).

> > IMHO it's a rather scary choice on tzcode's part to use int_fastN_t,
>
> Yeah, I was never pleased with that choice of theirs.  OTOH, I've
> seen darn few portability complaints on their mailing list, so
> it seems like they've got it right in isolation.  The problem
> from our standpoint is that I don't think we want int_fastN_t
> to leak into APIs visible to the rest of Postgres, because then
> we risk issues related to their configuration methods being
> totally unlike ours.

Yeah.  My first swing at this touched only .c files, no .h files, with
that in mind.

  29. Re: Cannot find a working 64-bit integer type on Illumos

    Peter Eisentraut <peter@eisentraut.org> — 2024-07-14T14:47:50Z

    On 04.07.24 03:55, Thomas Munro wrote:
> Unfortunately, that theory turned out to be wrong.  The usual suspect,
> Windows, uses something else: "I64" or something like that.  We could
> teach our snprintf to grok that, but I don't like the idea anymore.
> So let's go back to INT64_MODIFIER, with just a small amount of
> configure time work to pick the right value.  I couldn't figure out
> any header-only way to do that.

src/port/snprintf.c used to support I64 in the past, but it was later 
removed.  We could probably put it back.

>> Personally, I find "PRId64" pretty unreadable. "INT64_MODIFIER" wasn't
>> nice either, though, and following standards is good, so I'm sure I'll
>> get used to it.

Using PRId64 would be very beneficial because gettext understands it, 
and so we would no longer need the various workarounds for not putting 
INT64_FORMAT into the middle of a translated string.

But this could be a separate patch.  What you have works for now.

Here are some other comments on this patch set:

* v3-0001-Use-int64_t-support-from-stdint.h.patch

- src/include/c.h:

Maybe add a comment that above all the int8_t -> int8 etc. typedefs
that these are for backward compatibility or something like that.
Actually, just move the comment

+/* Historical names for limits in stdint.h. */

up a bit to it covers the types as well.

Also, these /* == 8 bits */ comments could be removed, I think.

- src/include/port/pg_bitutils.h:
- src/port/pg_bitutils.c:
- src/port/snprintf.c:

These changes look functionally correct, but I think I like the old
code layout better, like

#if (using long)
...
#elif (using long long)
...
#else
#error
#endif

rather than

#if (using long)
...
#else
static assertion
... // long long
#endif

which seems a bit more complicated.  I think you could leave the code
mostly alone and just change

defined(HAVE_LONG_INT_64) to SIZEOF_LONG == 8
defined(HAVE_LONG_LONG_INT_64) to SIZEOF_LONG_LONG == 8

in each case.

- src/include/postgres_ext.h:

-#define OID_MAX  UINT_MAX
-/* you will need to include <limits.h> to use the above #define */
+#define OID_MAX  UINT32_MAX

If the type Oid is unsigned int, then the OID_MAX should be UINT_MAX.
So this should not be changed.  Also, is the comment about <limits.h>
no longer applicable?


- src/interfaces/ecpg/ecpglib/typename.c:
- src/interfaces/ecpg/include/sqltypes.h:
- .../test/expected/compat_informix-sqlda.c:

-#ifdef HAVE_LONG_LONG_INT_64
+#if SIZEOF_LONG < 8

These changes alter the library behavior unnecessarily.  The old code
would always prefer to report back long long (ECPGt_long_long etc.),
but the new code will report back long (ECPGt_long etc.) if it is
64-bit.  I don't know the impact of these changes, but it seems
preferable to keep the existing behavior.

- src/interfaces/ecpg/include/ecpg_config.h.in:
- src/interfaces/ecpg/include/meson.build:

In the past, we have kept obsolete symbols as always defined in
ecpg_config.h.  We ought to do the same here.


* v3-0002-Remove-traces-of-BeOS.patch

This looks ok.  This could also be committed before 0001.


* v3-0003-Allow-tzcode-to-use-stdint.h-and-inttypes.h.patch

- src/timezone/localtime.c:

Addition of #include <stdint.h> is unnecessary, since it's already
included in c.h, and it's also not in the upstream code.

This looks like a typo:

-                                * UTC months are at least 28 days long 
(minus 1 second for a
+                                * UTC months are at least 2 days long 
(minus 1 second for a

-getsecs(const char *strp, int32 *const secsp)
+getsecs(const char *strp, int_fast32_t * const secsp)

Need to add int_fast32_t (and maybe the other types) to typedefs.list?

- src/timezone/zic.c:

+#include <inttypes.h>
+#include <stdint.h>

We don't need both of these.  Also, this is not in the upstream code
AFAICT.

  30. Re: Cannot find a working 64-bit integer type on Illumos

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-07-14T14:51:54Z

    Peter Eisentraut <peter@eisentraut.org> writes:
> On 04.07.24 03:55, Thomas Munro wrote:
>>> Personally, I find "PRId64" pretty unreadable. "INT64_MODIFIER" wasn't
>>> nice either, though, and following standards is good, so I'm sure I'll
>>> get used to it.

> Using PRId64 would be very beneficial because gettext understands it, 
> and so we would no longer need the various workarounds for not putting 
> INT64_FORMAT into the middle of a translated string.

Uh, really?  The translated strings live in /usr/share, which is
expected to be architecture-independent, so how would they make
that work?

			regards, tom lane

  31. Re: Cannot find a working 64-bit integer type on Illumos

    Peter Eisentraut <peter@eisentraut.org> — 2024-07-22T14:39:13Z

    On 14.07.24 16:51, Tom Lane wrote:
> Peter Eisentraut <peter@eisentraut.org> writes:
>> On 04.07.24 03:55, Thomas Munro wrote:
>>>> Personally, I find "PRId64" pretty unreadable. "INT64_MODIFIER" wasn't
>>>> nice either, though, and following standards is good, so I'm sure I'll
>>>> get used to it.
> 
>> Using PRId64 would be very beneficial because gettext understands it,
>> and so we would no longer need the various workarounds for not putting
>> INT64_FORMAT into the middle of a translated string.
> 
> Uh, really?  The translated strings live in /usr/share, which is
> expected to be architecture-independent, so how would they make
> that work?

Gettext has some special run-time support for this.  See here: 
<https://www.gnu.org/software/gettext/manual/html_node/Preparing-Strings.html#No-string-concatenation>

  32. Re: Cannot find a working 64-bit integer type on Illumos

    Peter Eisentraut <peter@eisentraut.org> — 2024-10-14T06:46:07Z

    To move this along a bit, I have committed the "Remove traces of BeOS." 
patch.

  33. Re: Cannot find a working 64-bit integer type on Illumos

    Thomas Munro <thomas.munro@gmail.com> — 2024-11-29T10:12:57Z

    On Tue, Jul 23, 2024 at 2:39 AM Peter Eisentraut <peter@eisentraut.org> wrote:
> On 14.07.24 16:51, Tom Lane wrote:
> > Peter Eisentraut <peter@eisentraut.org> writes:
> >> On 04.07.24 03:55, Thomas Munro wrote:
> >>>> Personally, I find "PRId64" pretty unreadable. "INT64_MODIFIER" wasn't
> >>>> nice either, though, and following standards is good, so I'm sure I'll
> >>>> get used to it.
> >
> >> Using PRId64 would be very beneficial because gettext understands it,
> >> and so we would no longer need the various workarounds for not putting
> >> INT64_FORMAT into the middle of a translated string.
> >
> > Uh, really?  The translated strings live in /usr/share, which is
> > expected to be architecture-independent, so how would they make
> > that work?
>
> Gettext has some special run-time support for this.  See here:
> <https://www.gnu.org/software/gettext/manual/html_node/Preparing-Strings.html#No-string-concatenation>

Nice.  I've never really looked into gettext() very seriously, so I
wondered about the portability of it all.  In our docs, a long time
ago, you wrote "... you need an implementation of the Gettext API.
Some operating systems have this built-in (e.g., Linux, NetBSD,
Solaris),".  It looks like we might still have those implementations
of libintl.so in our build farm today: mostly the GNU one, but also
NetBSD's, illumos's, and to the unknowable extent it is different
Solaris's.  They might all be using GNU msgfmt though.

I want to move this forward, but we have to decide which way.  I'm now
leaning back towards using <inttypes.h> macros, based on your
feedback.  So here's a rebase of v2, with some improvements, and some
responses to your earlier email:

On Mon, Jul 15, 2024 at 2:47 AM Peter Eisentraut <peter@eisentraut.org> wrote:
> On 04.07.24 03:55, Thomas Munro wrote:
> > Unfortunately, that theory turned out to be wrong.  The usual suspect,
> > Windows, uses something else: "I64" or something like that.  We could
> > teach our snprintf to grok that, but I don't like the idea anymore.
> > So let's go back to INT64_MODIFIER, with just a small amount of
> > configure time work to pick the right value.  I couldn't figure out
> > any header-only way to do that.
>
> src/port/snprintf.c used to support I64 in the past, but it was later
> removed.  We could probably put it back.

New idea: let's just redefine PRI...{32,64,PTR} on that platform,
instead of modifying snprintf.c.

> - src/include/c.h:
>
> Maybe add a comment that above all the int8_t -> int8 etc. typedefs
> that these are for backward compatibility or something like that.
> Actually, just move the comment
>
> +/* Historical names for limits in stdint.h. */
>
> up a bit to it covers the types as well.

Hmm I think it deserves a separate comment but yeah "EXACTLY N BITS"
and can be "used for numerical computations" can definitely go.  I
guess that's a memory of some pre-standard system that had similarly
named types with the "at least" meaning, but it's not helping anybody
today.

> Also, these /* == 8 bits */ comments could be removed, I think.

Yeah.

> - src/include/port/pg_bitutils.h:
> - src/port/pg_bitutils.c:
> - src/port/snprintf.c:
>
> These changes look functionally correct, but I think I like the old
> code layout better, like
>
> #if (using long)
> ...
> #elif (using long long)
> ...
> #else
> #error
> #endif
>
> rather than
>
> #if (using long)
> ...
> #else
> static assertion
> ... // long long
> #endif

Fair point, that was all a bit ugly.  Let's see...

> which seems a bit more complicated.  I think you could leave the code
> mostly alone and just change
>
> defined(HAVE_LONG_INT_64) to SIZEOF_LONG == 8
> defined(HAVE_LONG_LONG_INT_64) to SIZEOF_LONG_LONG == 8
>
> in each case.

What do you think about this approach?

            case 'z':
#if SIZEOF_SIZE_T == SIZEOF_LONG
                longflag = 1;
#elif SIZEOF_SIZE_T == SIZEOF_LONG_LONG
                longlongflag = 1;
#else
#error "cannot find integer type of the same size as size_t"
#endif

And then in the cases where the size is implied by the typename
(uint64_t -> 8) it's the same setup except with a number:

#if SIZEOF_LONG == 8
    return 63 - __builtin_clzl(word);
#elif SIZEOF_LONG_LONG == 8
    return 63 - __builtin_clzll(word);
#else
#error "cannot find integer type of the same size as uint64_t"
#endif

What I like about that is that it's very clear that the conditions
match the ...l() or ...ll() function.  It's obviously correct, and
would fail on a Deathstation 9000 that is trying to trick us.

> - src/include/postgres_ext.h:
>
> -#define OID_MAX  UINT_MAX
> -/* you will need to include <limits.h> to use the above #define */
> +#define OID_MAX  UINT32_MAX
>
> If the type Oid is unsigned int, then the OID_MAX should be UINT_MAX.
> So this should not be changed.  Also, is the comment about <limits.h>
> no longer applicable?

Right, yeah, hunk nuked.

> - src/interfaces/ecpg/ecpglib/typename.c:
> - src/interfaces/ecpg/include/sqltypes.h:
> - .../test/expected/compat_informix-sqlda.c:
>
> -#ifdef HAVE_LONG_LONG_INT_64
> +#if SIZEOF_LONG < 8
>
> These changes alter the library behavior unnecessarily.  The old code
> would always prefer to report back long long (ECPGt_long_long etc.),
> but the new code will report back long (ECPGt_long etc.) if it is
> 64-bit.  I don't know the impact of these changes, but it seems
> preferable to keep the existing behavior.

I agree that it should be better but I don't think that's right about
the preference.  The order of the tests here didn't matter, because
only HAVE_LONG_INT_64 would have been defined if sizeof(long) == 8,
not both, so it would still have preferred the first of { long, long
long } that had size 8 in that order.  But yeah that's confusing,
let's try changing it to the same form as the cases above (an
explicitly search for the integer type of the same size where the
conditions are obviously correct with a clear priority and no
default/fallback/assumption, just an error):

        case INT8OID:
#if SIZEOF_LONG == 8
            return ECPGt_long;
#elif SIZEOF_LONG_LONG == 8
            return ECPGt_long_long;
#else
#error "cannot find integer type of the same size as INT8OID"
#endif

> - src/interfaces/ecpg/include/ecpg_config.h.in:
> - src/interfaces/ecpg/include/meson.build:
>
> In the past, we have kept obsolete symbols as always defined in
> ecpg_config.h.  We ought to do the same here.

Normally we keep symbols that were once conditional and are now
constant, like ENABLE_THREAD_SAFETY.   Should that include
HAVE_LONG_INT_64 and HAVE_LONG_LONG_INT_64?  Only one would be
defined, but it might be that neither is true in the patch: it's a
typedef for int64_t, and the standard library might well have a
typedef to __internal_int64_type, which is neither long nor long long.
it seems like internal machinery for giving the client code an "int64"
type in the C89 days, and surely not part of our real API.

WIP patch attached.

(Let's leave that tzcode stuff far later.)

  34. Re: Cannot find a working 64-bit integer type on Illumos

    Thomas Munro <thomas.munro@gmail.com> — 2024-11-29T19:30:41Z

    On Fri, Nov 29, 2024 at 11:12 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> WIP patch attached.

Slightly better version.  I'd missed some configure stuff that could
be removed, and tidied up a few minor typos and mistakes.

I was thinking about that ECPG stuff: I bet real applications prefer
to use int64_t etc directly too instead of long, the worst type in C.
I wondered if the embedded SQL standard might know about that these
days (ECPGt_int64_t?), but I don't have the standard to hand.  DB2's
embedded SQL seems to have a type sqlint64, but I didn't look too
closely and of course even if we wanted to do something like that as
an optional API option, that'd be a later change.

BTW I forgot to mention earlier, I peeked at the source of gettext on
NetBSD and illumos, and both appear to handle those special
<inttypes.h> tokens when loading message catalogues.

  35. Re: Cannot find a working 64-bit integer type on Illumos

    Thomas Munro <thomas.munro@gmail.com> — 2024-11-29T23:42:53Z

    On Fri, Nov 29, 2024 at 11:12 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> New idea: let's just redefine PRI...{32,64,PTR} on that platform,
> instead of modifying snprintf.c.

D'oh, that's not going to fly.  gettext() would replace %<PRId64> with
the system's PRId64, so we can't avoid teaching our snprintf.c to
understand Windowsian format strings.  Here's a first attempt at that.
Tested a bit by removing the #ifdef WIN32 locally and playing around
with it.  CI passes on Windows, and I think that should be exercising
it via existing [U]INT64_FORMAT in various places that would break if
it didn't work.

It would be nice to see it working via gettext() too, but NLS seems to
be disabled on that OS in CI (and also the BF).  Huh, that seems
non-ideal.  Transcripts on pgsql-bugs show it working on that OS
though (with occasional encoding glitches, apparently), so I guess the
EDB build must have it enabled.

I tried out the gettext() support for this on Unix.  I didn't bother
with xgettext etc, I just manually added a message to fr.po to test
the runtime based on an example in another project, and it worked
unsurprisingly:

-                                errmsg("REJECT_LIMIT (%lld) must be
greater than zero",
-                                               (long long) reject_limit)));
+                                errmsg("REJECT_LIMIT (%" PRId64 ")
must be greater than zero",
+                                               reject_limit)));

+#, c-format
+msgid "REJECT_LIMIT (%<PRId64>) must be greater than zero"
+msgstr "REJECT_LIMIT (%<PRId64>) doit être supérieure à zéro"

postgres=# copy t from '/dev/null' with (reject_limit -1);
ERREUR:  REJECT_LIMIT (-1) doit être supérieure à zéro

  36. Re: Cannot find a working 64-bit integer type on Illumos

    Peter Eisentraut <peter@eisentraut.org> — 2024-12-03T13:22:56Z

    On 29.11.24 20:30, Thomas Munro wrote:
> On Fri, Nov 29, 2024 at 11:12 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> I was thinking about that ECPG stuff: I bet real applications prefer
> to use int64_t etc directly too instead of long, the worst type in C.
> I wondered if the embedded SQL standard might know about that these
> days (ECPGt_int64_t?), but I don't have the standard to hand.  DB2's
> embedded SQL seems to have a type sqlint64, but I didn't look too
> closely and of course even if we wanted to do something like that as
> an optional API option, that'd be a later change.

Interesting:

i) If “long long” is specified, then the <host parameter data type> of 
HV is BIGINT.
ii) If “long” is specified, then the <host parameter data type> of HV is 
INTEGER.
iii) If “short” is specified, then the <host parameter data type> of HV 
is SMALLINT.
[...]

I suppose that makes sense.

> BTW I forgot to mention earlier, I peeked at the source of gettext on
> NetBSD and illumos, and both appear to handle those special
> <inttypes.h> tokens when loading message catalogues.

Ah great, thanks for checking that.

  37. Re: Cannot find a working 64-bit integer type on Illumos

    Peter Eisentraut <peter@eisentraut.org> — 2024-12-03T13:24:47Z

    On 30.11.24 00:42, Thomas Munro wrote:
> On Fri, Nov 29, 2024 at 11:12 PM Thomas Munro <thomas.munro@gmail.com> wrote:
>> New idea: let's just redefine PRI...{32,64,PTR} on that platform,
>> instead of modifying snprintf.c.
> 
> D'oh, that's not going to fly.  gettext() would replace %<PRId64> with
> the system's PRId64, so we can't avoid teaching our snprintf.c to
> understand Windowsian format strings.  Here's a first attempt at that.
> Tested a bit by removing the #ifdef WIN32 locally and playing around
> with it.  CI passes on Windows, and I think that should be exercising
> it via existing [U]INT64_FORMAT in various places that would break if
> it didn't work.

This patch looks good to me.

In meson.build, this comment seems to be misplaced by accident:

+# Check if __int128 is a working 128 bit integer type, and if so
+# define PG_INT128_TYPE to that typename.
  cdata.set('SIZEOF_VOID_P', cc.sizeof('void *', args: test_c_args))
  cdata.set('SIZEOF_SIZE_T', cc.sizeof('size_t', args: test_c_args))

In c.h, you include <inttypes.h> instead of <stdint.h>.  Is there a 
reason for that?

  38. Re: Cannot find a working 64-bit integer type on Illumos

    Thomas Munro <thomas.munro@gmail.com> — 2024-12-04T03:04:24Z

    On Wed, Dec 4, 2024 at 2:24 AM Peter Eisentraut <peter@eisentraut.org> wrote:
> This patch looks good to me.

Thanks!  Pushed.  Let's see what the build farm says.

> In meson.build, this comment seems to be misplaced by accident:

Oops, fixed.

> In c.h, you include <inttypes.h> instead of <stdint.h>.  Is there a
> reason for that?

<stdint.h> was already there for uintptr_t at least.  <inttypes.h> is
needed to be able to define:

#define INT64_FORMAT "%" PRId64

  39. Re: Cannot find a working 64-bit integer type on Illumos

    Thomas Munro <thomas.munro@gmail.com> — 2024-12-04T05:58:42Z

    On Wed, Dec 4, 2024 at 4:04 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> Thanks!  Pushed.  Let's see what the build farm says.

A couple of armv7 animals seemed to die in the Perl tests.  Huh.  Well
I know that Perl was sensitive to this stuff but it passed on 32 bit
CI (x86).  I will try to reproduce that on local ARM hardware...

  40. Re: Cannot find a working 64-bit integer type on Illumos

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-12-04T06:19:32Z

    Thomas Munro <thomas.munro@gmail.com> writes:
> A couple of armv7 animals seemed to die in the Perl tests.  Huh.  Well
> I know that Perl was sensitive to this stuff but it passed on 32 bit
> CI (x86).  I will try to reproduce that on local ARM hardware...

turaco seems unhappy because of

2024-12-04 05:37:14.831 GMT [9584:18] pg_regress/plperl_setup LOG:  statement: CREATE EXTENSION plperl;
Util.c: loadable library and perl binaries are mismatched (got first handshake key 0xa300080, needed 0xa400080)

Not clear to me what these changes would have done to trigger that.

			regards, tom lane

  41. Re: Cannot find a working 64-bit integer type on Illumos

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-12-04T19:41:19Z

    Also ... grison and turaco are emitting warnings that were
not there a couple of days ago:

 grison        | 2024-12-04 17:10:09 | reconstruct.c:701:33: warning: passing argument 2 of 'copy_file_range' from incompatible pointer type [-Wincompatible-pointer-types]
 turaco        | 2024-12-04 16:15:11 | reconstruct.c:701:61: warning: passing argument 2 of 'copy_file_range' from incompatible pointer type [-Wincompatible-pointer-types]

The code they are complaining about is from ac8110155 ("Allow using
copy_file_range in write_reconstructed_file") back in April:

            off_t        off = offsetmap[i];
            ...
                wb = copy_file_range(s->fd, &off, wfd, NULL, BLCKSZ - nwritten, 0);

Now, on my Linux box "man copy_file_range" saith

       ssize_t copy_file_range(int fd_in, loff_t *off_in,
                               int fd_out, loff_t *off_out,
                               size_t len, unsigned int flags);

So apparently, "off_t" was the same as "loff_t" before 962da900a,
but it no longer is the same on 32-bit machines.  (In any case,
if all machines that have copy_file_range define it like this,
perhaps we ought to be declaring this variable as loff_t not off_t?)

Digging a bit deeper, the full warning report is

reconstruct.c: In function "write_reconstructed_file":
reconstruct.c:701:33: warning: passing argument 2 of "copy_file_range" from incompatible pointer type [-Wincompatible-pointer-types]
     wb = copy_file_range(s->fd, &off, wfd, NULL, BLCKSZ - nwritten, 0);
                                 ^~~~
In file included from reconstruct.c:15:
/usr/include/unistd.h:1107:49: note: expected "__off64_t *" {aka "long long int *"} but argument is of type "off_t *" {aka "long int *"}
 ssize_t copy_file_range (int __infd, __off64_t *__pinoff,
                                      ~~~~~~~~~~~^~~~~~~~

Since these are 32-bit machines, "long int" is 32 bits (confirmed from
their configure results), which means "off_t" is only 32 bits, which
really sounds quite broken.  I thought it was 64 bits pretty much
everywhere nowadays.  Did 962da900a cause that?  Maybe that explains
the Perl library compatibility problems these machines are reporting?

			regards, tom lane

  42. Re: Cannot find a working 64-bit integer type on Illumos

    Thomas Munro <thomas.munro@gmail.com> — 2024-12-04T21:03:02Z

    On Thu, Dec 5, 2024 at 8:41 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> So apparently, "off_t" was the same as "loff_t" before 962da900a,
> but it no longer is the same on 32-bit machines.

Right, that's really weird.

> (In any case,
> if all machines that have copy_file_range define it like this,
> perhaps we ought to be declaring this variable as loff_t not off_t?)

Hmm.  FreeBSD just has off_t in its declaration, but off_t is always
64 bit, and I don't think it knows about loff_t.

I guess copy_file_range() didn't follow the usual plan of respecting
_FILE_OFFSET_BITS, so there's no 32 bit off_t support for it in libc,
hence they needed to invent (or leak from the kernel?) loff_t.  I
don't think we should change that, because we use AC_SYS_LARGEFILE to
find that we need -D_FILE_OFFSET_BITS=64 on 32 bit systems, so off_t
should be the same as loff_t.  If something weren't broken
somewhere...  Somehow it's getting lost?

> Since these are 32-bit machines, "long int" is 32 bits (confirmed from
> their configure results), which means "off_t" is only 32 bits, which
> really sounds quite broken.  I thought it was 64 bits pretty much
> everywhere nowadays.  Did 962da900a cause that?  Maybe that explains
> the Perl library compatibility problems these machines are reporting?

Right, that definitely should upset Perl, as it changes the size of
struct stat and all kinds of stuff, probably.

The affected systems are mostly running ancient OSes, and lapwing is
32 bit x86, so it's not only armv7.  Current 32 bit x86 systems are
working fine.  Maybe I should boot up a Debian 7 x86 build (my
attempts to run ancient Debian/armv7 on qemu last night failed, x86
sounds easier...)  The armv7 systems except turaco are running the
same era of Debian as lapwing, but I'm not sure what turaco is.

  43. Re: Cannot find a working 64-bit integer type on Illumos

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-12-04T21:20:37Z

    I wrote:
> So apparently, "off_t" was the same as "loff_t" before 962da900a,
> but it no longer is the same on 32-bit machines.

OK, I see what is happening.  On platforms that need it, we define
_FILE_OFFSET_BITS as 64 in pg_config.h to ensure that off_t is
big enough.  However, 962da900a did this:

--- a/src/include/postgres_ext.h
+++ b/src/include/postgres_ext.h
@@ -23,7 +23,7 @@
 #ifndef POSTGRES_EXT_H
 #define POSTGRES_EXT_H
 
-#include "pg_config_ext.h"
+#include <stdint.h>

Since c.h reads postgres_ext.h first, <stdint.h> is now pulled in
before pg_config.h, and that in turn pulls in <features.h>, which
locks down the decision that off_t will be 32 bits, as well as some
other decisions we don't want.  We can NOT include any system headers
before pg_config.h; I'm surprised we're not seeing related failures on
the Solaris-en, where _LARGEFILE_SOURCE is similarly critical.

Another rather serious problem here is that we no longer provide
macro PG_INT64_TYPE, which seems rather likely to break applications
that were relying on it.  That is part of our external API, we
can't just remove it on a whim.

I think the least painful solution would be to revert the parts
of 962da900a that got rid of pg_config_ext.h and PG_INT64_TYPE.
Since PG_INT64_TYPE is a macro not a typedef, it might be okay
to #define it as int64_t even before we've read that header,
so as not to give up the principle of relying on stdint.h for the
underlying definition.

Now that I see this, I'm fairly astonished that there aren't
more problems than we've noticed.  I wonder whether it'd be
a good idea to put in a static assert somewhere about the
width of off_t, so that the next screwup of this sort will
be easier to diagnose.

			regards, tom lane

  44. Re: Cannot find a working 64-bit integer type on Illumos

    Thomas Munro <thomas.munro@gmail.com> — 2024-12-04T21:44:06Z

    On Thu, Dec 5, 2024 at 10:20 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wrote:
> > So apparently, "off_t" was the same as "loff_t" before 962da900a,
> > but it no longer is the same on 32-bit machines.
>
> OK, I see what is happening.  On platforms that need it, we define
> _FILE_OFFSET_BITS as 64 in pg_config.h to ensure that off_t is
> big enough.  However, 962da900a did this:
>
> --- a/src/include/postgres_ext.h
> +++ b/src/include/postgres_ext.h
> @@ -23,7 +23,7 @@
>  #ifndef POSTGRES_EXT_H
>  #define POSTGRES_EXT_H
>
> -#include "pg_config_ext.h"
> +#include <stdint.h>
>
> Since c.h reads postgres_ext.h first, <stdint.h> is now pulled in
> before pg_config.h, and that in turn pulls in <features.h>, which
> locks down the decision that off_t will be 32 bits, as well as some
> other decisions we don't want.  We can NOT include any system headers
> before pg_config.h; I'm surprised we're not seeing related failures on
> the Solaris-en, where _LARGEFILE_SOURCE is similarly critical.

Ah, right.   Oops.

> Another rather serious problem here is that we no longer provide
> macro PG_INT64_TYPE, which seems rather likely to break applications
> that were relying on it.  That is part of our external API, we
> can't just remove it on a whim.

I had concluded that PG_INT64_TYPE wasn't part of our external API but
pg_int64 was, based on the comment:

 /* Define a signed 64-bit integer type for use in client API declarations. */
-typedef PG_INT64_TYPE pg_int64;
+typedef int64_t pg_int64;

But yeah, I obviously hadn't considered that other interaction, and ...

> I think the least painful solution would be to revert the parts
> of 962da900a that got rid of pg_config_ext.h and PG_INT64_TYPE.
> Since PG_INT64_TYPE is a macro not a typedef, it might be okay
> to #define it as int64_t even before we've read that header,
> so as not to give up the principle of relying on stdint.h for the
> underlying definition.

... yeah that sounds like a plan.  Looking into it.

> Now that I see this, I'm fairly astonished that there aren't
> more problems than we've noticed.  I wonder whether it'd be
> a good idea to put in a static assert somewhere about the
> width of off_t, so that the next screwup of this sort will
> be easier to diagnose.

Right, we could assert that it hasn't changed from what configure detected.

  45. Re: Cannot find a working 64-bit integer type on Illumos

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-12-04T21:50:05Z

    Thomas Munro <thomas.munro@gmail.com> writes:
> On Thu, Dec 5, 2024 at 10:20 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Another rather serious problem here is that we no longer provide
>> macro PG_INT64_TYPE, which seems rather likely to break applications
>> that were relying on it.  That is part of our external API, we
>> can't just remove it on a whim.

> I had concluded that PG_INT64_TYPE wasn't part of our external API but
> pg_int64 was, based on the comment:

>  /* Define a signed 64-bit integer type for use in client API declarations. */
> -typedef PG_INT64_TYPE pg_int64;
> +typedef int64_t pg_int64;

Oh, hmm, maybe so.  OTOH, that typedef breaks the idea of #define'ing
PG_INT64_TYPE as int64_t.  We need this header to be readable without
any prior system headers, so I'm afraid we're all the way back to
making configure derive the name of a 64-bit type.

			regards, tom lane

  46. Re: Cannot find a working 64-bit integer type on Illumos

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-12-04T21:55:46Z

    I wrote:
> ... We need this header to be readable without
> any prior system headers, so I'm afraid we're all the way back to
> making configure derive the name of a 64-bit type.

Could another way be to read pg_config.h before postgres_ext.h?
I think the current order was intentional, but maybe the
disadvantages outweigh the advantages now.

			regards, tom lane

  47. Re: Cannot find a working 64-bit integer type on Illumos

    Thomas Munro <thomas.munro@gmail.com> — 2024-12-04T21:58:19Z

    On Thu, Dec 5, 2024 at 10:55 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wrote:
> > ... We need this header to be readable without
> > any prior system headers, so I'm afraid we're all the way back to
> > making configure derive the name of a 64-bit type.
>
> Could another way be to read pg_config.h before postgres_ext.h?
> I think the current order was intentional, but maybe the
> disadvantages outweigh the advantages now.

Yeah I was just testing that idea :-)  I can't see why it needs to be
first, but was looking for what the original reason was...

  48. Re: Cannot find a working 64-bit integer type on Illumos

    Thomas Munro <thomas.munro@gmail.com> — 2024-12-04T22:45:50Z

    On Thu, Dec 5, 2024 at 10:58 AM Thomas Munro <thomas.munro@gmail.com> wrote:
> On Thu, Dec 5, 2024 at 10:55 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Could another way be to read pg_config.h before postgres_ext.h?
> > I think the current order was intentional, but maybe the
> > disadvantages outweigh the advantages now.
>
> Yeah I was just testing that idea :-)  I can't see why it needs to be
> first, but was looking for what the original reason was...

Seems good to me.  Also there were another couple of contortions due
to the older ordering, which we could improve I think?

  49. Re: Cannot find a working 64-bit integer type on Illumos

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-12-04T23:16:31Z

    Thomas Munro <thomas.munro@gmail.com> writes:
> On Thu, Dec 5, 2024 at 10:58 AM Thomas Munro <thomas.munro@gmail.com> wrote:
>> On Thu, Dec 5, 2024 at 10:55 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Could another way be to read pg_config.h before postgres_ext.h?
>>> I think the current order was intentional, but maybe the
>>> disadvantages outweigh the advantages now.

> Seems good to me.  Also there were another couple of contortions due
> to the older ordering, which we could improve I think?

In c.h, I'd put in a very explicit comment in front of pg_config.h.
Also, I don't like making it look like postgres_ext.h is of the
same ilk as the config headers, since it isn't.  So maybe like

/*
 * These headers must be included before any system headers, because
 * on some platforms they affect the behavior of the system headers
 * (for example, by defining _FILE_OFFSET_BITS).
 */
#include "pg_config.h"
#include "pg_config_manual.h"   /* must be after pg_config.h */
#include "pg_config_os.h"       /* must be before any system header files */

/* Pull in fundamental symbols that we also expose to applications */
#include "postgres_ext.h"

/* System header files that should be available everywhere in Postgres */
#include <inttypes.h>
...

The comment for pg_config_os.h is redundant this way, maybe we could
rewrite it as something more useful?

Also, there's probably no reason anymore that postgres_ext.h couldn't
be placed after those fundamental system headers, and that might be
clearer.  (I think perhaps the main reason for the existing ordering
was to demonstrate that postgres_ext.h could be included before any
system headers, and that's no longer a consideration.)


I don't especially care for your proposed changes to postgres_ext.h.
That substantially expands the footprint of what gets defined by
pulling that in, and some users might not care for that.  (For
example, because they have ordering assumptions similar to what we're
dealing with here.)  Now you already snuck the camel's nose under the
tent by including stdint.h there, and maybe these additional headers
wouldn't do any further damage.  But I don't see a strong argument to
change long-standing external APIs any more than we absolutely must.

			regards, tom lane

  50. Re: Cannot find a working 64-bit integer type on Illumos

    Thomas Munro <thomas.munro@gmail.com> — 2024-12-05T00:21:46Z

    On Thu, Dec 5, 2024 at 12:16 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> In c.h, I'd put in a very explicit comment in front of pg_config.h.
> Also, I don't like making it look like postgres_ext.h is of the
> same ilk as the config headers, since it isn't.  So maybe like
>
> /*
>  * These headers must be included before any system headers, because
>  * on some platforms they affect the behavior of the system headers
>  * (for example, by defining _FILE_OFFSET_BITS).
>  */
> #include "pg_config.h"
> #include "pg_config_manual.h"   /* must be after pg_config.h */
> #include "pg_config_os.h"       /* must be before any system header files */
>
> /* Pull in fundamental symbols that we also expose to applications */
> #include "postgres_ext.h"
>
> /* System header files that should be available everywhere in Postgres */
> #include <inttypes.h>
> ...
>
> The comment for pg_config_os.h is redundant this way, maybe we could
> rewrite it as something more useful?

OK, how about:

+#include "pg_config_os.h"              /* config from
include/port/PORTNAME.h */

That demystifies where it's really coming from ("portname" is used in
the meson script, and seems more informative than "template").

> Also, there's probably no reason anymore that postgres_ext.h couldn't
> be placed after those fundamental system headers, and that might be
> clearer.  (I think perhaps the main reason for the existing ordering
> was to demonstrate that postgres_ext.h could be included before any
> system headers, and that's no longer a consideration.)

Yeah, that makes sense.

> I don't especially care for your proposed changes to postgres_ext.h.
> That substantially expands the footprint of what gets defined by
> pulling that in, and some users might not care for that.  (For
> example, because they have ordering assumptions similar to what we're
> dealing with here.)  Now you already snuck the camel's nose under the
> tent by including stdint.h there, and maybe these additional headers
> wouldn't do any further damage.  But I don't see a strong argument to
> change long-standing external APIs any more than we absolutely must.

If someone wants to define things like that before potentially
including system headers, they're not going about it the right way if
they're including our headers first (or anything at all not under
their direct control).  But OK, I can work with the
not-broken-so-don't-fix-it and
pulling-in-more-stuff-that-maybe-they-don't-want arguments. :-)

  51. Re: Cannot find a working 64-bit integer type on Illumos

    Thomas Munro <thomas.munro@gmail.com> — 2024-12-05T00:59:32Z

    On Thu, Dec 5, 2024 at 10:03 AM Thomas Munro <thomas.munro@gmail.com> wrote:
> The affected systems are mostly running ancient OSes, and lapwing is
> 32 bit x86, so it's not only armv7.  Current 32 bit x86 systems are
> working fine.

I was trying to figure out how I missed this, and I think it might be
that the meson build scripts didn't port AC_SYS_LARGEFILES.  So if you
build on a 32 bit Linux system with meson (like one of CI's tasks, and
also build farm animal adder) then I think you finish up with 32 bit
off_t and no SIZEOF_OFF_T, because we don't do AC_SYS_LARGEFILES'
dance to figure out if this system needs -D_FILE_OFFSET_BITS=64 (or
other similar macros for AIX, Solaris etc).  I will look into that.

  52. Re: Cannot find a working 64-bit integer type on Illumos

    Thomas Munro <thomas.munro@gmail.com> — 2024-12-05T01:06:09Z

    On Thu, Dec 5, 2024 at 1:59 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> I was trying to figure out how I missed this, and I think it might be
> that the meson build scripts didn't port AC_SYS_LARGEFILES.  So if you
> build on a 32 bit Linux system with meson (like one of CI's tasks, and
> also build farm animal adder) then I think you finish up with 32 bit
> off_t and no SIZEOF_OFF_T, because we don't do AC_SYS_LARGEFILES'
> dance to figure out if this system needs -D_FILE_OFFSET_BITS=64 (or
> other similar macros for AIX, Solaris etc).  I will look into that.

Ahh, correction, it does define it (or else perl would have
complained), but it seems that meson magically puts it into the
compiler command line without being asked.  So it is defined without
pg_config.h being involved, and thus earlier.  Huh.

  53. Re: Cannot find a working 64-bit integer type on Illumos

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-12-05T01:25:14Z

    Thomas Munro <thomas.munro@gmail.com> writes:
> If someone wants to define things like that before potentially
> including system headers, they're not going about it the right way if
> they're including our headers first (or anything at all not under
> their direct control).  But OK, I can work with the
> not-broken-so-don't-fix-it and
> pulling-in-more-stuff-that-maybe-they-don't-want arguments. :-)

v2 LGTM.

			regards, tom lane

  54. Re: Cannot find a working 64-bit integer type on Illumos

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-12-05T03:29:38Z

    Thomas Munro <thomas.munro@gmail.com> writes:
> On Thu, Dec 5, 2024 at 1:59 PM Thomas Munro <thomas.munro@gmail.com> wrote:
>> I was trying to figure out how I missed this, and I think it might be
>> that the meson build scripts didn't port AC_SYS_LARGEFILES.  So if you
>> build on a 32 bit Linux system with meson (like one of CI's tasks, and
>> also build farm animal adder) then I think you finish up with 32 bit
>> off_t and no SIZEOF_OFF_T, because we don't do AC_SYS_LARGEFILES'
>> dance to figure out if this system needs -D_FILE_OFFSET_BITS=64 (or
>> other similar macros for AIX, Solaris etc).  I will look into that.

> Ahh, correction, it does define it (or else perl would have
> complained), but it seems that meson magically puts it into the
> compiler command line without being asked.  So it is defined without
> pg_config.h being involved, and thus earlier.  Huh.

That does not seem great.  Compile an extension without the same
CPPFLAGS, you silently get an ABI-incompatible module.  We really
ought to be putting these ABI-critical flags into pg_config.h.
It's especially bad that this works differently between autoconf
and meson builds.

			regards, tom lane

  55. Re: Cannot find a working 64-bit integer type on Illumos

    Thomas Munro <thomas.munro@gmail.com> — 2024-12-05T04:26:53Z

    On Thu, Dec 5, 2024 at 4:29 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Thomas Munro <thomas.munro@gmail.com> writes:
> > Ahh, correction, it does define it (or else perl would have
> > complained), but it seems that meson magically puts it into the
> > compiler command line without being asked.  So it is defined without
> > pg_config.h being involved, and thus earlier.  Huh.
>
> That does not seem great.  Compile an extension without the same
> CPPFLAGS, you silently get an ABI-incompatible module.  We really
> ought to be putting these ABI-critical flags into pg_config.h.
> It's especially bad that this works differently between autoconf
> and meson builds.

It makes the whole MinGW circus worse that I'd realised:

https://www.postgresql.org/message-id/flat/CA%2BhUKG%2B6ZPD_X5ADMwX2uUtXqe_wv8%2BKQ5xFeAR2zbcodjNZvw%40mail.gmail.com#ddad894fcfa9733d30a579f7eb52ebf6

  56. Re: Cannot find a working 64-bit integer type on Illumos

    Thomas Munro <thomas.munro@gmail.com> — 2024-12-10T02:02:07Z

    On Thu, Dec 5, 2024 at 12:16 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Now you already snuck the camel's nose under the
> tent by including stdint.h there, and maybe these additional headers
> wouldn't do any further damage.

Even though we fixed the immediate issue (thanks), this comment stayed
with me.  I did that because I didn't want to change any interfaces at
the same time as the <stdint.h> retrofit, but I agree that it feels a
bit odd hidden in there, and doesn't appear to conform to
postgres_ext.h's self-description.  Stepping back, and I realise it's
difficult to answer with certainty, I wonder why anyone would ever
want to use postgres_ext.h directly for the definition of pg_int64
*without* being a user of libpq-fe.h.  I can't find any references to
pg_int64 (excluding forks of our code) on github; there are a few
things like proxies and suchlike that include postgres_ext.h for other
things, mostly bogusly (they also include libpq-fe.h, or they say they
want NAMEDATALEN, which isn't there anymore).

We have just three lo_*64() functions using that type and then
pg_usec_time_t.  Seems like a very narrow usage that hasn't spread,
likely only used to receive arguments, and really quite specific to
libpq-fe.h and not one of the "fundamental Postgres declarations".
Maybe we should consider moving #include <stdint.h> into libpq-fe.h?

And if we included <stdint.h> overtly, rather than covertly in
postgres_ext.h, why would we still want a third name for int64_t?  We
could change the three lo_*64() declarations to use the standard type
directly, but keep the historical typedef marked deprecated.

> But I don't see a strong argument to
> change long-standing external APIs any more than we absolutely must.

So perhaps you'll hate that idea then.  I wonder if you'll hate it
more than keeping the #include in postgres_ext.h, hence putting the
idea forward!

  57. Re: Cannot find a working 64-bit integer type on Illumos

    Thomas Munro <thomas.munro@gmail.com> — 2025-03-02T22:35:31Z

    On Tue, Dec 10, 2024 at 3:02 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> On Thu, Dec 5, 2024 at 12:16 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Now you already snuck the camel's nose under the
> > tent by including stdint.h there, and maybe these additional headers
> > wouldn't do any further damage.
>
> Even though we fixed the immediate issue (thanks), this comment stayed
> with me.  I did that because I didn't want to change any interfaces at
> the same time as the <stdint.h> retrofit, but I agree that it feels a
> bit odd hidden in there, and doesn't appear to conform to
> postgres_ext.h's self-description.  Stepping back, and I realise it's
> difficult to answer with certainty, I wonder why anyone would ever
> want to use postgres_ext.h directly for the definition of pg_int64
> *without* being a user of libpq-fe.h.  I can't find any references to
> pg_int64 (excluding forks of our code) on github; there are a few
> things like proxies and suchlike that include postgres_ext.h for other
> things, mostly bogusly (they also include libpq-fe.h, or they say they
> want NAMEDATALEN, which isn't there anymore).
>
> We have just three lo_*64() functions using that type and then
> pg_usec_time_t.  Seems like a very narrow usage that hasn't spread,
> likely only used to receive arguments, and really quite specific to
> libpq-fe.h and not one of the "fundamental Postgres declarations".
> Maybe we should consider moving #include <stdint.h> into libpq-fe.h?
>
> And if we included <stdint.h> overtly, rather than covertly in
> postgres_ext.h, why would we still want a third name for int64_t?  We
> could change the three lo_*64() declarations to use the standard type
> directly, but keep the historical typedef marked deprecated.
>
> > But I don't see a strong argument to
> > change long-standing external APIs any more than we absolutely must.
>
> So perhaps you'll hate that idea then.  I wonder if you'll hate it
> more than keeping the #include in postgres_ext.h, hence putting the
> idea forward!

Does anyone else have thoughts about this arguable leftover quirk from
the <stdint.h> refactoring work?  In brief, shouldn't libpq-fe.h
include <stdint.h> directly, and use int64_t explicitly, instead of
doing it "secretly" in another header that came about because of
historical namespace pollution concerns, now gone?  We require you to
have a 64 bit integer type, we require C99, C99 requires int64_t to be
defined if you have a 64 bit type, and there doesn't seem to be any
reason to want pg_int64 other than to use these large object functions
in libpq-fe.h.  The current arrangement feels a bit obfuscated,
leading to the patch in the previous email.  Adding Ishii-san who
introduced the three uses of pg_int64 to libpq-fe.h in 461ef73f.

  58. Re: Cannot find a working 64-bit integer type on Illumos

    Peter Eisentraut <peter@eisentraut.org> — 2025-03-04T17:00:12Z

    On 10.12.24 03:02, Thomas Munro wrote:
> On Thu, Dec 5, 2024 at 12:16 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Now you already snuck the camel's nose under the
>> tent by including stdint.h there, and maybe these additional headers
>> wouldn't do any further damage.
> 
> Even though we fixed the immediate issue (thanks), this comment stayed
> with me.  I did that because I didn't want to change any interfaces at
> the same time as the <stdint.h> retrofit, but I agree that it feels a
> bit odd hidden in there, and doesn't appear to conform to
> postgres_ext.h's self-description.  Stepping back, and I realise it's
> difficult to answer with certainty, I wonder why anyone would ever
> want to use postgres_ext.h directly for the definition of pg_int64
> *without* being a user of libpq-fe.h.  I can't find any references to
> pg_int64 (excluding forks of our code) on github; there are a few
> things like proxies and suchlike that include postgres_ext.h for other
> things, mostly bogusly (they also include libpq-fe.h, or they say they
> want NAMEDATALEN, which isn't there anymore).
> 
> We have just three lo_*64() functions using that type and then
> pg_usec_time_t.  Seems like a very narrow usage that hasn't spread,
> likely only used to receive arguments, and really quite specific to
> libpq-fe.h and not one of the "fundamental Postgres declarations".
> Maybe we should consider moving #include <stdint.h> into libpq-fe.h?
> 
> And if we included <stdint.h> overtly, rather than covertly in
> postgres_ext.h, why would we still want a third name for int64_t?  We
> could change the three lo_*64() declarations to use the standard type
> directly, but keep the historical typedef marked deprecated.

I agree with your patch 0001-Deprecate-pg_int64.patch.  I don't see a 
reason to keep the current arrangement around pg_int64.

  59. Re: Cannot find a working 64-bit integer type on Illumos

    Thomas Munro <thomas.munro@gmail.com> — 2025-03-25T08:41:21Z

    On Wed, Mar 5, 2025 at 6:00 AM Peter Eisentraut <peter@eisentraut.org> wrote:
> I agree with your patch 0001-Deprecate-pg_int64.patch.  I don't see a
> reason to keep the current arrangement around pg_int64.

Thanks for looking!  Pushed.

  60. Re: Cannot find a working 64-bit integer type on Illumos

    Nathan Bossart <nathandbossart@gmail.com> — 2025-03-25T21:33:59Z

    On Tue, Mar 25, 2025 at 09:41:21PM +1300, Thomas Munro wrote:
> On Wed, Mar 5, 2025 at 6:00 AM Peter Eisentraut <peter@eisentraut.org> wrote:
>> I agree with your patch 0001-Deprecate-pg_int64.patch.  I don't see a
>> reason to keep the current arrangement around pg_int64.
> 
> Thanks for looking!  Pushed.

anaconda seems to be upset about this one [0].  I've spent all of 30
seconds looking at it so far, but it appears to be using an old version of
the header file.

In file included from pg_regress.c:34:
/usr/local/include/libpq-fe.h:623:8: error: unknown type name 'pg_int64'
  623 | extern pg_int64 lo_lseek64(PGconn *conn, int fd, pg_int64 offset, int whence);
      |        ^
/usr/local/include/libpq-fe.h:623:50: error: unknown type name 'pg_int64'
  623 | extern pg_int64 lo_lseek64(PGconn *conn, int fd, pg_int64 offset, int whence);
      |                                                  ^
/usr/local/include/libpq-fe.h:627:8: error: unknown type name 'pg_int64'
  627 | extern pg_int64 lo_tell64(PGconn *conn, int fd);
      |        ^
/usr/local/include/libpq-fe.h:629:48: error: unknown type name 'pg_int64'
  629 | extern int      lo_truncate64(PGconn *conn, int fd, pg_int64 len);
      |                                                     ^
4 errors generated.

[0] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=anaconda&dt=2025-03-25%2021%3A22%3A33

-- 
nathan

  61. Re: Cannot find a working 64-bit integer type on Illumos

    Thomas Munro <thomas.munro@gmail.com> — 2025-03-25T21:39:44Z

    On Wed, Mar 26, 2025 at 10:34 AM Nathan Bossart
<nathandbossart@gmail.com> wrote:
> anaconda seems to be upset about this one [0].  I've spent all of 30
> seconds looking at it so far, but it appears to be using an old version of
> the header file.
>
> In file included from pg_regress.c:34:
> /usr/local/include/libpq-fe.h:623:8: error: unknown type name 'pg_int64'

Looks like it's mixing up /usr/local/include and our source tree...

  62. Re: Cannot find a working 64-bit integer type on Illumos

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-03-25T23:01:37Z

    Thomas Munro <thomas.munro@gmail.com> writes:
> On Wed, Mar 26, 2025 at 10:34 AM Nathan Bossart
> <nathandbossart@gmail.com> wrote:
>> In file included from pg_regress.c:34:
>> /usr/local/include/libpq-fe.h:623:8: error: unknown type name 'pg_int64'

> Looks like it's mixing up /usr/local/include and our source tree...

Yeah.  That's because the compile command for pg_regress.c has
-I../../../src/include/libpq too late, after -I switches added
for other things:

ccache cc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Werror=unguarded-availability-new -Wendif-labels -Wmissing-format-attribute -Wcast-function-type -Wformat-security -Wmissing-variable-declarations -fno-strict-aliasing -fwrapv -fexcess-precision=standard -Wno-unused-command-line-argument -Wno-compound-token-split-by-macro -Wno-format-truncation -Wno-cast-function-type-strict -g -O2 -fPIC -DPIC -fvisibility=hidden -I../../../src/include -I/usr/local/include -I/usr/local/include/libxml2  -I/usr/local/include -I../../../src/port -I../../../src/interfaces/libpq '-DHOST_TUPLE="x86_64-unknown-freebsd15.0"' '-DSHELLPROG="/bin/sh"'  -c -o pg_regress.o pg_regress.c

How did that work before?  Perhaps somebody just now added a libpq
dependency to pg_regress.c?

			regards, tom lane

  63. Re: Cannot find a working 64-bit integer type on Illumos

    Daniel Gustafsson <daniel@yesql.se> — 2025-03-25T23:06:30Z

    > On 26 Mar 2025, at 00:01, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> How did that work before?  Perhaps somebody just now added a libpq
> dependency to pg_regress.c?

I believe the libpq dependency came in 66d6086cbcbfc8 which wasn't all that
recent.

--
Daniel Gustafsson

  64. Re: Cannot find a working 64-bit integer type on Illumos

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-03-25T23:36:34Z

    Daniel Gustafsson <daniel@yesql.se> writes:
>> On 26 Mar 2025, at 00:01, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> How did that work before?  Perhaps somebody just now added a libpq
>> dependency to pg_regress.c?

> I believe the libpq dependency came in 66d6086cbcbfc8 which wasn't all that
> recent.

It looks like this has been broken for a very long time, but it must
never have mattered before because libpq-fe.h is so stable, and
pg_regress doesn't use any new-ish APIs from it.  So pulling in
whatever version the platform had still worked.

I think this should work to fix it:

-pg_regress.o: override CPPFLAGS += -I$(top_builddir)/src/port -I$(libpq_srcdir) $(EXTRADEFS)
+pg_regress.o: override CPPFLAGS := -I$(top_builddir)/src/port -I$(libpq_srcdir) $(EXTRADEFS) $(CPPFLAGS)

but I haven't tested yet.

			regards, tom lane

  65. Re: Cannot find a working 64-bit integer type on Illumos

    Thomas Munro <thomas.munro@gmail.com> — 2025-04-10T07:20:17Z

    On Wed, Mar 26, 2025 at 12:36 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Daniel Gustafsson <daniel@yesql.se> writes:
> >> On 26 Mar 2025, at 00:01, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> How did that work before?  Perhaps somebody just now added a libpq
> >> dependency to pg_regress.c?
>
> > I believe the libpq dependency came in 66d6086cbcbfc8 which wasn't all that
> > recent.
>
> It looks like this has been broken for a very long time, but it must
> never have mattered before because libpq-fe.h is so stable, and
> pg_regress doesn't use any new-ish APIs from it.  So pulling in
> whatever version the platform had still worked.
>
> I think this should work to fix it:
>
> -pg_regress.o: override CPPFLAGS += -I$(top_builddir)/src/port -I$(libpq_srcdir) $(EXTRADEFS)
> +pg_regress.o: override CPPFLAGS := -I$(top_builddir)/src/port -I$(libpq_srcdir) $(EXTRADEFS) $(CPPFLAGS)
>
> but I haven't tested yet.

Our meson scripts also have this problem, which I couldn't figure out
how to fix completely in my first attempt:

https://www.postgresql.org/message-id/flat/CA%2BhUKGKispvxLyrBn3%3D3mp0BB1N%2BRBYR5eE2guCOksnwEoOcPQ%40mail.gmail.com

  66. Re: Cannot find a working 64-bit integer type on Illumos

    Peter Eisentraut <peter@eisentraut.org> — 2025-09-03T15:04:03Z

    On 10.12.24 03:02, Thomas Munro wrote:
> And if we included <stdint.h> overtly, rather than covertly in
> postgres_ext.h, why would we still want a third name for int64_t?  We
> could change the three lo_*64() declarations to use the standard type
> directly, but keep the historical typedef marked deprecated.
> 
>> But I don't see a strong argument to
>> change long-standing external APIs any more than we absolutely must.
> 
> So perhaps you'll hate that idea then.  I wonder if you'll hate it
> more than keeping the #include in postgres_ext.h, hence putting the
> idea forward!

This change in commit 3c86223c998 is problematic.

commit 3c86223c998
Author: Thomas Munro <tmunro@postgresql.org>
Date:   Tue Mar 25 08:17:53 2025

     libpq: Deprecate pg_int64.

     ...

     Keep a typedef marked deprecated for backward compatibility, but 
move it
     into libpq-fe.h where it was used.


Consider a third-party extension that does something like dblink or 
postgres_fdw.  It will compile against a server and also a libpq.  The 
server and the libpq might not be of the same major version.  (On 
Debian, only the latest libpq will be available.)  If you have for 
example server version 17 and libpq version 18, then you will get the 
pg_int64 typedef both from postgres_ext.h (from the PG17 server 
includes) and from libpq-fe.h (from PG18 libpq).  That is not allowed in 
C99, and even if it were, the underlying types of PG_INT64_TYPE (in 
PG17) and int64_t (in PG18) might be different (long int vs. long long 
int) and this would fail.

I think this could be fixed by moving the definition of pg_int64 back to 
postgres_ext.h.  Then extension builds would only get one definition, 
because of the header guards.  Depending on include order, they could 
get a different underlying type, but that's a smaller problem, since the 
type is supposed to be deprecated anyway.

  67. Re: Cannot find a working 64-bit integer type on Illumos

    Thomas Munro <thomas.munro@gmail.com> — 2025-09-04T00:39:43Z

    On Thu, Sep 4, 2025 at 3:04 AM Peter Eisentraut <peter@eisentraut.org> wrote:
> This change in commit 3c86223c998 is problematic.
>
> commit 3c86223c998
> Author: Thomas Munro <tmunro@postgresql.org>
> Date:   Tue Mar 25 08:17:53 2025
>
>      libpq: Deprecate pg_int64.
>
>      ...
>
>      Keep a typedef marked deprecated for backward compatibility, but
> move it
>      into libpq-fe.h where it was used.
>
>
> Consider a third-party extension that does something like dblink or
> postgres_fdw.  It will compile against a server and also a libpq.  The
> server and the libpq might not be of the same major version.  (On
> Debian, only the latest libpq will be available.)  If you have for
> example server version 17 and libpq version 18, then you will get the
> pg_int64 typedef both from postgres_ext.h (from the PG17 server
> includes) and from libpq-fe.h (from PG18 libpq).  That is not allowed in
> C99, and even if it were, the underlying types of PG_INT64_TYPE (in
> PG17) and int64_t (in PG18) might be different (long int vs. long long
> int) and this would fail.
>
> postgres_ext.h.  Then extension builds would only get one definition,
> because of the header guards.  Depending on include order, they could
> get a different underlying type, but that's a smaller problem, since the
> type is supposed to be deprecated anyway.

Hmm.  So we're talking about a Debian system compiling an extension
that has these headers in its search path:

/usr/include/postgresql/postgres_ext.h <-- v18[1]
/usr/include/postgresql/17/server/postgres_ext.h <-- v17[2] (or other
older release)

I guess it's only a problem if the v17 header is found first, but such
an extension must build OK if the libpq headers are found first,
right?  I'm not sure what to think about that yet, ie how to decide
which orders should work.

Is there an argument for recommending/requiring the newer one be used?
 The counter argument must be that we could eventually remove macros
for stuff we remove in future that are still useful in the older
server that your extension is building against.  (Here we removed
something, but it's not something that matches the file's own mission
statement, it's just ancient C evolution stuff and I still think that
commit was a reasonable choice, though of course this wrinkle needs a
solution.)

Other ideas to make cross-version postgres_ext.h work, at least as far
as this issue goes:

1.  Wrap the new typedef in #ifndef PG_INT64_TYPE, with a comment to
explain that it's for older releases of postgres_ext.h that supplied
the typedef.
2.  Just delete the new typedef completely, and if someone wants it
they can define it themselves.  IIRC I couldn't actually find a single
example of another project using the typename in github or the Debian
code search.  In other words, if people are using those 3 functions
they're using some other typename, that's just how we spelled it, so
technically it may be a (noop) cast when you call them.

[1] https://packages.debian.org/trixie/amd64/libpq-dev/filelist
[2] https://packages.debian.org/trixie/amd64/postgresql-server-dev-17/filelist

  68. Re: Cannot find a working 64-bit integer type on Illumos

    Peter Eisentraut <peter@eisentraut.org> — 2025-09-04T09:01:37Z

    On 03.09.25 17:04, Peter Eisentraut wrote:
> Consider a third-party extension that does something like dblink or 
> postgres_fdw.  It will compile against a server and also a libpq.  The 
> server and the libpq might not be of the same major version.  (On 
> Debian, only the latest libpq will be available.)  If you have for 
> example server version 17 and libpq version 18, then you will get the 
> pg_int64 typedef both from postgres_ext.h (from the PG17 server 
> includes) and from libpq-fe.h (from PG18 libpq).  That is not allowed in 
> C99, and even if it were, the underlying types of PG_INT64_TYPE (in 
> PG17) and int64_t (in PG18) might be different (long int vs. long long 
> int) and this would fail.
> 
> I think this could be fixed by moving the definition of pg_int64 back to 
> postgres_ext.h.  Then extension builds would only get one definition, 
> because of the header guards.  Depending on include order, they could 
> get a different underlying type, but that's a smaller problem, since the 
> type is supposed to be deprecated anyway.

Here is a patch that has been reported to fix the problem.

  69. Re: Cannot find a working 64-bit integer type on Illumos

    Peter Eisentraut <peter@eisentraut.org> — 2025-09-04T09:05:03Z

    On 04.09.25 02:39, Thomas Munro wrote:
> Hmm.  So we're talking about a Debian system compiling an extension
> that has these headers in its search path:
> 
> /usr/include/postgresql/postgres_ext.h <-- v18[1]
> /usr/include/postgresql/17/server/postgres_ext.h <-- v17[2] (or other
> older release)
> 
> I guess it's only a problem if the v17 header is found first, but such
> an extension must build OK if the libpq headers are found first,
> right?  I'm not sure what to think about that yet, ie how to decide
> which orders should work.

The logs I have seen have the libpq include path first.

I don't think the order matters.  The problem is that you have two 
conflicting typedefs of the same name in two separate header files.  So 
if you include both of them, you'll get an error.

  70. Re: Cannot find a working 64-bit integer type on Illumos

    Peter Eisentraut <peter@eisentraut.org> — 2025-09-12T13:51:43Z

    On 04.09.25 11:01, Peter Eisentraut wrote:
> On 03.09.25 17:04, Peter Eisentraut wrote:
>> Consider a third-party extension that does something like dblink or 
>> postgres_fdw.  It will compile against a server and also a libpq.  The 
>> server and the libpq might not be of the same major version.  (On 
>> Debian, only the latest libpq will be available.)  If you have for 
>> example server version 17 and libpq version 18, then you will get the 
>> pg_int64 typedef both from postgres_ext.h (from the PG17 server 
>> includes) and from libpq-fe.h (from PG18 libpq).  That is not allowed 
>> in C99, and even if it were, the underlying types of PG_INT64_TYPE (in 
>> PG17) and int64_t (in PG18) might be different (long int vs. long long 
>> int) and this would fail.
>>
>> I think this could be fixed by moving the definition of pg_int64 back 
>> to postgres_ext.h.  Then extension builds would only get one 
>> definition, because of the header guards.  Depending on include order, 
>> they could get a different underlying type, but that's a smaller 
>> problem, since the type is supposed to be deprecated anyway.
> 
> Here is a patch that has been reported to fix the problem.

I propose to go ahead with this patch in a few days if there are no 
other solutions coming.

  71. Re: Cannot find a working 64-bit integer type on Illumos

    Peter Eisentraut <peter@eisentraut.org> — 2025-09-16T09:03:23Z

    On 12.09.25 15:51, Peter Eisentraut wrote:
> On 04.09.25 11:01, Peter Eisentraut wrote:
>> On 03.09.25 17:04, Peter Eisentraut wrote:
>>> Consider a third-party extension that does something like dblink or 
>>> postgres_fdw.  It will compile against a server and also a libpq.  
>>> The server and the libpq might not be of the same major version.  (On 
>>> Debian, only the latest libpq will be available.)  If you have for 
>>> example server version 17 and libpq version 18, then you will get the 
>>> pg_int64 typedef both from postgres_ext.h (from the PG17 server 
>>> includes) and from libpq-fe.h (from PG18 libpq).  That is not allowed 
>>> in C99, and even if it were, the underlying types of PG_INT64_TYPE 
>>> (in PG17) and int64_t (in PG18) might be different (long int vs. long 
>>> long int) and this would fail.
>>>
>>> I think this could be fixed by moving the definition of pg_int64 back 
>>> to postgres_ext.h.  Then extension builds would only get one 
>>> definition, because of the header guards.  Depending on include 
>>> order, they could get a different underlying type, but that's a 
>>> smaller problem, since the type is supposed to be deprecated anyway.
>>
>> Here is a patch that has been reported to fix the problem.
> 
> I propose to go ahead with this patch in a few days if there are no 
> other solutions coming.

done