Horton

Thread

Commits

  1. Replace %llu by PRIu64 in AIO io_uring code

  1. Replace some %llu remnants in the tree

    Michael Paquier <michael@paquier.xyz> — 2025-06-09T03:59:20Z

    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

  2. Re: Replace some %llu remnants in the tree

    Peter Eisentraut <peter@eisentraut.org> — 2025-06-11T07:58:00Z

    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.

  3. Re: Replace some %llu remnants in the tree

    Michael Paquier <michael@paquier.xyz> — 2025-06-11T22:16:37Z

    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

  4. Re: Replace some %llu remnants in the tree

    Nathan Bossart <nathandbossart@gmail.com> — 2025-06-12T14:56:28Z

    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

  5. Re: Replace some %llu remnants in the tree

    Michael Paquier <michael@paquier.xyz> — 2025-06-13T00:01:43Z

    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

  6. Re: Replace some %llu remnants in the tree

    Andres Freund <andres@anarazel.de> — 2025-06-13T00:13:08Z

    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

  7. Re: Replace some %llu remnants in the tree

    Michael Paquier <michael@paquier.xyz> — 2025-06-14T23:25:33Z

    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