Replace some %llu remnants in the tree
Hi all,
While hacking a different patch, I've noticed that a couple of %llu
did not get the PRIu64 call in the AIO code, and I don't see why we
could not switch them. These have been introduced in commits that got
into the tree after Peter's 15a79c73111f.
A couple remain even after the attached, which have been left off by
Peter for the same reasons as the ones I am guessing here:
- launch_backend.c for paramHandle, does not seem worth it.
- ecpglib/execute.c, when storing some input, which does not seem
worth bothering either.
- reconstruct.c, offset handling.
- pg_backup_tar.c, ftello() result and some consistency with the
surroundings.
- pg_verifybackup.c and astreamer_verify.c, for Sizes.
That's not necessarily mandatory for v18, for sure, but as this is new
code we could as well clean it up before forking the next stable
branch.
Comments or opinions?
--
Michael
Attachments:
0001-Replace-llu-by-PRIu64-in-AIO-io_uring-code.patchtext/x-diff; charset=us-asciiDownload
From 0c9ccd79203f4709638658cf918d5ebc3d584801 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Mon, 9 Jun 2025 11:41:57 +0900
Subject: [PATCH] Replace %llu by PRIu64 in AIO io_uring code
---
src/backend/storage/aio/method_io_uring.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/backend/storage/aio/method_io_uring.c b/src/backend/storage/aio/method_io_uring.c
index cc312b641ca6..b78048328e11 100644
--- a/src/backend/storage/aio/method_io_uring.c
+++ b/src/backend/storage/aio/method_io_uring.c
@@ -400,9 +400,9 @@ pgaio_uring_wait_one(PgAioHandle *ioh, uint64 ref_generation)
while (true)
{
pgaio_debug_io(DEBUG3, ioh,
- "wait_one io_gen: %llu, ref_gen: %llu, cycle %d",
- (long long unsigned) ioh->generation,
- (long long unsigned) ref_generation,
+ "wait_one io_gen: %" PRIu64 ", ref_gen: %" PRIu64 ", cycle %d",
+ ioh->generation,
+ ref_generation,
waited);
if (pgaio_io_was_recycled(ioh, ref_generation, &state) ||
--
2.49.0
On 09.06.25 05:59, Michael Paquier wrote:
While hacking a different patch, I've noticed that a couple of %llu
did not get the PRIu64 call in the AIO code, and I don't see why we
could not switch them. These have been introduced in commits that got
into the tree after Peter's 15a79c73111f.
Looks good.
That's not necessarily mandatory for v18, for sure, but as this is new
code we could as well clean it up before forking the next stable
branch.
Agree this should go into v18.
A couple remain even after the attached, which have been left off by
Peter for the same reasons as the ones I am guessing here:
- launch_backend.c for paramHandle, does not seem worth it.
According to Microsoft documentation, LONG_PTR is __int64, and so using
PRId64 would seem to be appropriate. However, I wonder whether it
wouldn't be simpler to print the original paramHandle either as %p or
cast to uintptr_t, to avoid having two separate code paths.
- ecpglib/execute.c, when storing some input, which does not seem
worth bothering either.
These are dealing with actual long long int values, to using %lld/%llu
seems appropriate.
- reconstruct.c, offset handling.
- pg_backup_tar.c, ftello() result and some consistency with the
surroundings.
- pg_verifybackup.c and astreamer_verify.c, for Sizes.
These are using off_t or pg_off_t, for which there is no format
placeholder, so you have to cast it to something.
On Wed, Jun 11, 2025 at 09:58:00AM +0200, Peter Eisentraut wrote:
On 09.06.25 05:59, Michael Paquier wrote:
That's not necessarily mandatory for v18, for sure, but as this is new
code we could as well clean it up before forking the next stable
branch.Agree this should go into v18.
Thanks for the review. Adding the RMT in CC for more comments. Would
you be OK with the patch added to v18? The answer is probably yes,
but let's ask anyway.
--
Michael
On Thu, Jun 12, 2025 at 07:16:37AM +0900, Michael Paquier wrote:
On Wed, Jun 11, 2025 at 09:58:00AM +0200, Peter Eisentraut wrote:
On 09.06.25 05:59, Michael Paquier wrote:
That's not necessarily mandatory for v18, for sure, but as this is new
code we could as well clean it up before forking the next stable
branch.Agree this should go into v18.
Thanks for the review. Adding the RMT in CC for more comments. Would
you be OK with the patch added to v18? The answer is probably yes,
but let's ask anyway.
Seems fine to me.
--
nathan
On Thu, Jun 12, 2025 at 09:56:28AM -0500, Nathan Bossart wrote:
On Thu, Jun 12, 2025 at 07:16:37AM +0900, Michael Paquier wrote:
Thanks for the review. Adding the RMT in CC for more comments. Would
you be OK with the patch added to v18? The answer is probably yes,
but let's ask anyway.Seems fine to me.
Thanks. Applied on HEAD, then.
--
Michael
Hi,
On 2025-06-09 12:59:20 +0900, Michael Paquier wrote:
While hacking a different patch, I've noticed that a couple of %llu
did not get the PRIu64 call in the AIO code, and I don't see why we
could not switch them. These have been introduced in commits that got
into the tree after Peter's 15a79c73111f.
FWIW, I find it utterly unsurpising that new users of %llu were introduced
after 15a79c73111f. For one, 15a79c73111f explicitly says "(minimal trial)" in
the subject line, it'd have hardly been sensible to introduce PRI* uses at
that point. Only a0ed19e0a9e, changed most of the uses (after e.g. the
io_uring uses were introduced). For another, how is everyone even supposed to
even have known about this new policy, it was just discussed in a long thread,
at a very busy time? Even now I suspect we'll grow more %ll[ud] users, it's
after all what we've been trained to do for quite a while now.
Greetings,
Andres Freund
On Thu, Jun 12, 2025 at 08:13:08PM -0400, Andres Freund wrote:
FWIW, I find it utterly unsurpising that new users of %llu were introduced
after 15a79c73111f. For one, 15a79c73111f explicitly says "(minimal trial)" in
the subject line, it'd have hardly been sensible to introduce PRI* uses at
that point. Only a0ed19e0a9e, changed most of the uses (after e.g. the
io_uring uses were introduced). For another, how is everyone even supposed to
even have known about this new policy, it was just discussed in a long thread,
at a very busy time? Even now I suspect we'll grow more %ll[ud] users, it's
after all what we've been trained to do for quite a while now.
I don't think it is possible for everybody to be able to track that,
so we can be really flex about that. So relying on individuals to
notice it something when it is noticed is OK, and this was new code.
In this case, the only reason why I bumped into this is pure luck: I
was working on some backend area and wrote code that did the %llu with
the cast dance, because I'm educated the same way as most people to
make the code portable (use %llu and %lld, not the new thing). Then
while grepping for similar code patterns and the git history, I've
bumped into Peter's commit and just noticed the remaining %llu. And
this was on new code, so fixing it seemed just the right thing to do
at this stage.
--
Michael