Feedback-ID: ie0a040ee:Fastmail
Message-ID: <b936d2fb-590d-49c3-a615-92c3a88c6c19@eisentraut.org>
Date: Sun, 14 Jul 2024 15:47:50 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Cannot find a working 64-bit integer type on Illumos
To: Thomas Munro <thomas.munro@gmail.com>,
 Heikki Linnakangas <hlinnaka@iki.fi>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Japin Li <japinli@hotmail.com>,
 Andres Freund <andres@anarazel.de>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
References: <ME3P282MB3166F9D1F71F787929C0C7E7B6312@ME3P282MB3166.AUSP282.PROD.OUTLOOK.COM>
 <20240322165305.6zrtxcmzdrywvmsu@awork3.anarazel.de>
 <ME3P282MB3166D876E9CB83009727A5ECB6312@ME3P282MB3166.AUSP282.PROD.OUTLOOK.COM>
 <944094.1711128356@sss.pgh.pa.us>
 <CA+hUKG+CR05ACFujUbeAd-yM+nNJBSpnyB=q3vbriCJuoSu9Ag@mail.gmail.com>
 <1229869.1711160593@sss.pgh.pa.us>
 <CA+hUKG+ZwX-reVMHak3yo-63rrh9AJwEVGW4rCbsoLeSKO54Pg@mail.gmail.com>
 <7cf1dcb7-306f-4418-bf25-bdc75130db10@eisentraut.org>
 <CA+hUKGKXEdVSfR6UHXfR3tSQ+TfBNk-ssYmO2F0wCdw7qcx86Q@mail.gmail.com>
 <d8d68452-7195-41cf-a31b-e660f5ac6e75@iki.fi>
 <CA+hUKG+rYp-=fTP5kjRmnfPq7YgK-e=J+gJxBvKarkPVzLwQmQ@mail.gmail.com>
From: Peter Eisentraut <peter@eisentraut.org>
Content-Language: en-US
In-Reply-To: <CA+hUKG+rYp-=fTP5kjRmnfPq7YgK-e=J+gJxBvKarkPVzLwQmQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/b936d2fb-590d-49c3-a615-92c3a88c6c19%40eisentraut.org>
Precedence: bulk

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.