Re: 64 bit numbers vs format strings

On 02.03.25 22:08, Thomas Munro wrote:
> On Mon, Mar 3, 2025 at 6:21 AM Peter Eisentraut <peter(at)eisentraut(dot)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).