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

[Peter Eisentraut]

On 14.07.24 16:51, Tom Lane wrote:

Peter Eisentraut  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: 

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

[Tom Lane]

Peter Eisentraut  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

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

[Peter Eisentraut]

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  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 
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  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 
+#include 

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

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

[Thomas Munro]

On Thu, Jul 4, 2024 at 3:10 PM Tom Lane  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.

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

[Tom Lane]

Thomas Munro  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

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

[Heikki Linnakangas]

On 18/04/2024 23:29, Thomas Munro wrote:

On Thu, Apr 18, 2024 at 8:47 PM Peter Eisentraut  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  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)

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

[Peter Eisentraut]

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 
. 
 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

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

[Japin Li]

On Fri, 19 Apr 2024 at 05:22, Thomas Munro  wrote:
> On Thu, Apr 18, 2024 at 6:09 PM Japin Li  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  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

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

[Thomas Munro]

On Thu, Apr 18, 2024 at 6:09 PM Japin Li  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
.

> /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  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.

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

[Thomas Munro]

On Thu, Apr 18, 2024 at 8:47 PM Peter Eisentraut  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.

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

[Thomas Munro]

On Thu, Apr 18, 2024 at 8:47 PM Peter Eisentraut  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  and
the PRI*64 macros.


v2-0001-Use-int64_t-support-from-stdint.h-and-inttypes.h.patch
Description: Binary data


v2-0002-Remove-traces-of-BeOS.patch
Description: Binary data

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

[Peter Eisentraut]

On 18.04.24 02:31, Thomas Munro wrote:

On Sat, Mar 23, 2024 at 3:23 PM Tom Lane  wrote:

Thomas Munro  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  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
'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.

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

[Japin Li]

On Thu, 18 Apr 2024 at 08:31, Thomas Munro  wrote:
> On Sat, Mar 23, 2024 at 3:23 PM Tom Lane  wrote:
>> Thomas Munro  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  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
> '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 

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

[Thomas Munro]

On Sat, Mar 23, 2024 at 3:23 PM Tom Lane  wrote:
> Thomas Munro  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  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
'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?


0001-Use-int64_t-and-friends-from-stdint.h.patch
Description: Binary data


0002-Remove-traces-of-BeOS.patch
Description: Binary data

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

[Japin Li]

On Mon, 25 Mar 2024 at 09:32, Tom Lane  wrote:
> Japin Li  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.

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

[Tom Lane]

Japin Li  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

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

[Japin Li]

On Sat, 23 Mar 2024 at 01:22, Japin Li  wrote:
> On Sat, 23 Mar 2024 at 01:04, Tom Lane  wrote:
>> Japin Li  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]: *** [: 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

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

[Tom Lane]

Thomas Munro  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  and then getting rid of the configure-time
probes.

regards, tom lane

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

[Thomas Munro]

On Sat, Mar 23, 2024 at 6:26 AM Tom Lane  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 )

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

[Tom Lane]

Robert Haas  writes:
> On Fri, Mar 22, 2024 at 3:31 PM Andres Freund  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

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

[Robert Haas]

On Fri, Mar 22, 2024 at 3:31 PM Andres Freund  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

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

[Andres Freund]

Hi,

On 2024-03-22 15:02:45 -0400, Robert Haas wrote:
> On Fri, Mar 22, 2024 at 2:38 PM Andres Freund  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

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

[Robert Haas]

On Fri, Mar 22, 2024 at 2:38 PM Andres Freund  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

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

[Andres Freund]

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

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

[Tom Lane]

Japin Li  writes:
> On Sat, 23 Mar 2024 at 00:53, Andres Freund  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

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

[Japin Li]

On Sat, 23 Mar 2024 at 01:04, Tom Lane  wrote:
> Japin Li  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.

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

[Tom Lane]

Japin Li  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

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

[Andres Freund]

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