Horton

Thread

Commits

  1. Remove workarounds for avoiding [U]INT64_FORMAT in translatable strings.

  1. [PATCH] Remove workarounds to format [u]int64's

    Aleksander Alekseev <aleksander@timescale.com> — 2022-03-21T08:52:14Z

    Hi hackers,

I learned from Tom [1] that we can simplify the code like:

```
char buff[32];
snprintf(buf, sizeof(buf), INT64_FORMAT, ...)
ereport(WARNING, (errmsg("%s ...", buf)));
```

... and rely on %lld/%llu now as long as we explicitly cast the
argument to long long int / unsigned long long. This was previously
addressed in 6a1cd8b9 and d914eb34, but I see more places where we
still use an old approach.

Suggested patch fixes this. Tested locally - no warnings; passes all the tests.

[1] https://www.postgresql.org/message-id/771048.1647528068%40sss.pgh.pa.us

-- 
Best regards,
Aleksander Alekseev

  2. Re: [PATCH] Remove workarounds to format [u]int64's

    Pavel Borisov <pashkin.elfe@gmail.com> — 2022-03-21T09:12:33Z

    пн, 21 мар. 2022 г. в 12:52, Aleksander Alekseev <aleksander@timescale.com>:

> Hi hackers,
>
> I learned from Tom [1] that we can simplify the code like:
>
> ```
> char buff[32];
> snprintf(buf, sizeof(buf), INT64_FORMAT, ...)
> ereport(WARNING, (errmsg("%s ...", buf)));
> ```
>
> ... and rely on %lld/%llu now as long as we explicitly cast the
> argument to long long int / unsigned long long. This was previously
> addressed in 6a1cd8b9 and d914eb34, but I see more places where we
> still use an old approach.
>
> Suggested patch fixes this. Tested locally - no warnings; passes all the
> tests.
>
> [1]
> https://www.postgresql.org/message-id/771048.1647528068%40sss.pgh.pa.us
>
> Hi, Alexander!
Probably you can do (long long) instead of (long long int). It is shorter
and this is used elsewhere in the code.

-- 
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com <http://www.postgrespro.com>

  3. Re: [PATCH] Remove workarounds to format [u]int64's

    Aleksander Alekseev <aleksander@timescale.com> — 2022-03-21T09:23:33Z

    Hi Pavel,

> Probably you can do (long long) instead of (long long int). It is shorter and this is used elsewhere in the code.

Thanks! Here is the updated patch. I also added Reviewed-by: and
Discussion: to the commit message.

-- 
Best regards,
Aleksander Alekseev

  4. Re: [PATCH] Remove workarounds to format [u]int64's

    Pavel Borisov <pashkin.elfe@gmail.com> — 2022-03-21T09:30:59Z

    >
> > Probably you can do (long long) instead of (long long int). It is
> shorter and this is used elsewhere in the code.
>
> Thanks! Here is the updated patch. I also added Reviewed-by: and
> Discussion: to the commit message.
>
Thanks, Alexander!
I suggest the patch is in a good shape to be committed.
(
Maybe some strings that don't fit screen cloud be reflowed:
 (long long int)seqdataform->last_value, (long long int)seqform->seqmax)));
)

-- 
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com <http://www.postgrespro.com>

  5. Re: [PATCH] Remove workarounds to format [u]int64's

    Japin Li <japinli@hotmail.com> — 2022-03-21T10:13:46Z

    On Mon, 21 Mar 2022 at 17:23, Aleksander Alekseev <aleksander@timescale.com> wrote:
> Hi Pavel,
>
>> Probably you can do (long long) instead of (long long int). It is shorter and this is used elsewhere in the code.
>
> Thanks! Here is the updated patch. I also added Reviewed-by: and
> Discussion: to the commit message.

Hi,

After apply the patch, I found pg_checksums.c also has the similar code.

In progress_report(), I'm not sure we can do this replace for this code.

    snprintf(total_size_str, sizeof(total_size_str), INT64_FORMAT,
             total_size / (1024 * 1024));
    snprintf(current_size_str, sizeof(current_size_str), INT64_FORMAT,
             current_size / (1024 * 1024));

    fprintf(stderr, _("%*s/%s MB (%d%%) computed"),
            (int) strlen(current_size_str), current_size_str, total_size_str,
            percent);

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

  6. Re: [PATCH] Remove workarounds to format [u]int64's

    Aleksander Alekseev <aleksander@timescale.com> — 2022-03-21T10:55:44Z

    Hi Japin,

> After apply the patch, I found pg_checksums.c also has the similar code.

Thanks for noticing it.

> In progress_report(), I'm not sure we can do this replace for this code.

I added the corresponding change as a separate commit so it can be
easily reverted if necessary.

Here is a complete patchset with some additional changes by me.

-- 
Best regards,
Aleksander Alekseev

  7. Re: [PATCH] Remove workarounds to format [u]int64's

    Aleksander Alekseev <aleksander@timescale.com> — 2022-03-21T11:25:33Z

    Hi Japin,

> As Tom said in [1], we don't need to touch the *.po files, since those
files
> are managed by the translation team.
>
> [1]
https://www.postgresql.org/message-id/1110708.1647623560%40sss.pgh.pa.us

True, but I figured that simplifying the work of the translation team would
not harm either. In any case, the committer can easily exclude these
changes from the patch, if necessary.

-- 
Best regards,
Aleksander Alekseev

  8. Re: [PATCH] Remove workarounds to format [u]int64's

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-21T13:41:13Z

    Aleksander Alekseev <aleksander@timescale.com> writes:
>> As Tom said in [1], we don't need to touch the *.po files, since those
>> files are managed by the translation team.

> True, but I figured that simplifying the work of the translation team would
> not harm either.

It would not simplify things for them at all, just mess it up.
The master copies of the .po files are kept in a different repo.
Also, I believe that extraction of new message strings is automated
already.

https://www.postgresql.org/docs/devel/nls.html

https://wiki.postgresql.org/wiki/NLS

			regards, tom lane

  9. Re: [PATCH] Remove workarounds to format [u]int64's

    Aleksander Alekseev <aleksander@timescale.com> — 2022-03-21T14:37:44Z

    Hi Tom,

> It would not simplify things for them at all, just mess it up.
> The master copies of the .po files are kept in a different repo.
> Also, I believe that extraction of new message strings is automated
> already.

Got it, thanks. Here is the corrected patch. It includes all the
changes by me and Japin, and doesn't touch PO files.

-- 
Best regards,
Aleksander Alekseev

  10. Re: [PATCH] Remove workarounds to format [u]int64's

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-21T15:13:14Z

    Aleksander Alekseev <aleksander@timescale.com> writes:
> Got it, thanks. Here is the corrected patch. It includes all the
> changes by me and Japin, and doesn't touch PO files.

Pushed.  I removed now-unnecessary braces, reflowed some lines
as suggested by Pavel, and pgindent'ed (which insisted on adding
spaces after the casts, as is project style).

			regards, tom lane

  11. Re: [PATCH] Remove workarounds to format [u]int64's

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-03-23T20:48:48Z

    On 21.03.22 15:37, Aleksander Alekseev wrote:
>> It would not simplify things for them at all, just mess it up.
>> The master copies of the .po files are kept in a different repo.
>> Also, I believe that extraction of new message strings is automated
>> already.
> 
> Got it, thanks. Here is the corrected patch. It includes all the
> changes by me and Japin, and doesn't touch PO files.

I think in some cases we can make this even simpler (and cast-free) by 
changing the underlying variable to be long long instead of int64. 
Especially in cases where the whole point of the variable is to be some 
counter that ends up being printed, there isn't a need to use int64 in 
the first place.  See attached patch for examples.

  12. Re: [PATCH] Remove workarounds to format [u]int64's

    Pavel Borisov <pashkin.elfe@gmail.com> — 2022-03-23T21:12:27Z

    >
> On 21.03.22 15:37, Aleksander Alekseev wrote:
> >> It would not simplify things for them at all, just mess it up.
> >> The master copies of the .po files are kept in a different repo.
> >> Also, I believe that extraction of new message strings is automated
> >> already.
> >
> > Got it, thanks. Here is the corrected patch. It includes all the
> > changes by me and Japin, and doesn't touch PO files.
>
> I think in some cases we can make this even simpler (and cast-free) by
> changing the underlying variable to be long long instead of int64.
> Especially in cases where the whole point of the variable is to be some
> counter that ends up being printed, there isn't a need to use int64 in
> the first place.  See attached patch for examples.


Yes, this will work, when we can define a variable itself as *long long*.
But for some applications: [1], [2], I suppose we'll need exactly uint64 to
represent TransactionId. uint64 is warrantied to fit into *unsigned long
long*, but on most of archs it is just *unsigned long*. Defining what we
need to be uint64 as *unsigned long long* on these archs will mean it
become uint128, which we may not like.

In my opinion, in many places, it's better to have casts when it's for
printing fixed-width int/uint variables than the alternative.

[1]
https://www.postgresql.org/message-id/flat/CACG%3DezYV3FM5i9ws2QLyF%2Brz5WHTqheL59VRsHGsgAwfx8gh4g%40mail.gmail.com#d7068b9d25a2f8a1064d2ea4815df23d
[2]
https://www.postgresql.org/message-id/flat/CACG%3DezZe1NQSCnfHOr78AtAZxJZeCvxrts0ygrxYwe%3DpyyjVWA%40mail.gmail.com

--
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com <http://www.postgrespro.com>