Обсуждение: 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
Вложения
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 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
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