Re: 64 bit numbers vs format strings - Mailing list pgsql-hackers

On 02.03.25 22:08, Thomas Munro wrote:
> On Mon, Mar 3, 2025 at 6:21 AM Peter Eisentraut <peter@eisentraut.org> wrote:
>> On 05.12.24 23:18, Thomas Munro wrote:
>>>       Old: errmsg("hello %llu", (unsigned long long) x)
>>>       New: errmsg("hello %" PRIu64, x)
>>
>> I have committed the subset of this patch for pg_checksums.c so that the
>> translators and whoever else might be affected can try this out at small
>> scale.  (I don't expect any particular problems.)  Then we can move on
>> to the rest in a few weeks, I think.
> 
> Good plan, thanks.  Here's a rebase.

I think this went ok, and we can proceed here.

I looked through the v2-0001 patch in detail.  Most of it is mechanical, 
so no problems.  I couple of issues you already mentioned:

- correct placeholder for Datum (uintptr_t)

- i64abs() definition needs return cast

- I don't think it's proper to assume that pgoff_t or off_t matches int64_t.

A few additional comments:

- In src/backend/access/transam/xlogreader.c, you change a cast that is 
part of an arithmetic expression:

-                                     ((long long) total_len) - gotlen,
+                                     total_len - gotlen,

Is this no longer required to keep the math correct?  Both total_len and 
gotlen are uint32.  Maybe this was meant to convert to signed arithmetic?

- In src/backend/backup/basebackup.c, you change

-static long long int total_checksum_failures;
+static int64 total_checksum_failures;

I don't think it is required, and I don't think it should be encouraged, 
to expunge all uses of long long int, or something like that.  I think 
you should use long long int for "I need a big counter" and int64 when 
you want to control the storage layout.  Similar to how you might choose 
int vs. int32.  So I would leave this file alone.

- In src/bin/pg_verifybackup/astreamer_verify.c, you change the 
signedness of some arguments, e.g., in member_verify_header():

         report_backup_error(mystreamer->context,
-                           "\"%s\" has size %llu in \"%s\" but size 
%llu in the manifest",
+                           "\"%s\" has size %" PRId64 " in \"%s\" but 
size %" PRId64 " in the manifest",

The first signedness change is correct (member->size is pgoff_t), but 
the second is not (m->size is uint64).

I think it might be better to keep this patch as a mechanical change and 
fix up the signedness issues separately.  (There are actually a few more 
that were previously hidden by casts but will now show up with something 
like -Wformat-signedness.)

- In src/fe_utils/print.c, there is also a format change in the second 
hunk, but if we're going to do that one, we should also make the same 
change in the first hunk.  Also, in the first hunk, the second format 
should be %zu not %zd.

- In src/test/modules/libpq_pipeline/libpq_pipeline.c, you're changing 
the shift base from 1LL (signed) to UINT64_C(1) (unsigned).  This 
appears to be a semantic change separate from this patch?  But if this 
change is desired, then the signedness of the format argument should 
also be adjusted.


About the subsequent pgbench patches:

v2-0002: ok

v2-0003: Again, I'm not sure insisting on int64 use is needed here, and 
I don't know that the existing code is incorrect.  If we don't like 
using "long", we could just switch to "long long" here.

v2-0004: ok


About the LSN format patch, I'm generally sympathetic about this, and I 
think I've sort of asked for a similar change some years ago, but it's 
probably not worth pursuing for this release (if ever).