Horton

Thread

Commits

  1. Simplify printing of LSNs

  2. Unwind some workarounds for lack of portable int64 format specifier

  1. Printing LSN made easy

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2020-11-27T10:40:27Z

    Hi All,
An LSN is printed using format "%X/%X" and passing (uint32) (lsn >>
32), (uint32) lsn as arguments to that format specifier. This pattern
is repeated at 180 places according to Cscope. I find it hard to
remember this pattern every time I have to print LSN. Usually I end up
copying from some other place. Finding such a place takes a few
minutes and might be error prone esp when the lsn to be printed is an
expression. If we ever have to change this pattern in future, we will
need to change all those 180 places.

The solution seems to be simple though. In the attached patch, I have
added two macros
#define LSN_FORMAT "%X/%X"
#define LSN_FORMAT_ARG(lsn) (uint32) ((lsn) >> 32), (uint32) (lsn)

which can be used instead.

E.g. the following change in the patch
@@ -261,8 +261,7 @@ page_header(PG_FUNCTION_ARGS)
        {
                char            lsnchar[64];

-               snprintf(lsnchar, sizeof(lsnchar), "%X/%X",
-                                (uint32) (lsn >> 32), (uint32) lsn);
+               snprintf(lsnchar, sizeof(lsnchar), LSN_FORMAT,
LSN_FORMAT_ARG(lsn));
                values[0] = CStringGetTextDatum(lsnchar);

LSN_FORMAT_ARG expands to two comma separated arguments and is kinda
open at both ends  but it's handy that way.

Off list Peter Eisentraut pointed out that we can not use these macros
in elog/ereport since it creates problems for translations. He
suggested adding functions which return strings and use %s when doing
so.

The patch has two functions pg_lsn_out_internal() which takes an LSN
as input and returns a palloc'ed string containing the string
representation of LSN. This may not be suitable in performance
critical paths and also may leak memory if not freed. So there's
another function pg_lsn_out_buffer() which takes LSN and a char array
as input, fills the char array with the string representation and
returns the pointer to the char array. This allows the function to be
used as an argument in printf/elog etc. Macro MAXPG_LSNLEN has been
extern'elized for this purpose.

Off list Craig Ringer suggested introducing a new format specifier
similar to %m for LSN but I did not get time to take a look at the
relevant code. AFAIU it's available only to elog/ereport, so may not
be useful generally. But teaching printf variants about the new format
would be the best solution. However, I didn't find any way to do that.

In that patch I have changed some random code to use one of the above
methods appropriately, demonstrating their usage. Given that we have
grown 180 places already, I think that something like would have been
welcome earlier. But given that replication code is being actively
worked upon, it may not be too late. As a precedence lfirst_node() and
its variants arrived when the corresponding pattern had been repeated
at so many places.

I think we should move pg_lsn_out_internal() and pg_lsn_out_buffer()
somewhere else. Ideally xlogdefs.c would have been the best place but
there's no such file. May be we add those functions in pg_lsn.c and
add their declarations i xlogdefs.h.

-- 
Best Wishes,
Ashutosh Bapat

  2. Re: Printing LSN made easy

    Alexey Kondratov <a.kondratov@postgrespro.ru> — 2020-11-27T14:24:20Z

    Hi,

On 2020-11-27 13:40, Ashutosh Bapat wrote:
> 
> Off list Peter Eisentraut pointed out that we can not use these macros
> in elog/ereport since it creates problems for translations. He
> suggested adding functions which return strings and use %s when doing
> so.
> 
> The patch has two functions pg_lsn_out_internal() which takes an LSN
> as input and returns a palloc'ed string containing the string
> representation of LSN. This may not be suitable in performance
> critical paths and also may leak memory if not freed. So there's
> another function pg_lsn_out_buffer() which takes LSN and a char array
> as input, fills the char array with the string representation and
> returns the pointer to the char array. This allows the function to be
> used as an argument in printf/elog etc. Macro MAXPG_LSNLEN has been
> extern'elized for this purpose.
> 

If usage of macros in elog/ereport can cause problems for translation, 
then even with this patch life is not get simpler significantly. For 
example, instead of just doing like:

              elog(WARNING,
-                 "xlog min recovery request %X/%X is past current point 
%X/%X",
-                 (uint32) (lsn >> 32), (uint32) lsn,
-                 (uint32) (newMinRecoveryPoint >> 32),
-                 (uint32) newMinRecoveryPoint);
+                 "xlog min recovery request " LSN_FORMAT " is past 
current point " LSN_FORMAT,
+                 LSN_FORMAT_ARG(lsn),
+                 LSN_FORMAT_ARG(newMinRecoveryPoint));

we have to either declare two additional local buffers, which is 
verbose; or use pg_lsn_out_internal() and rely on memory contexts (or do 
pfree() manually, which is verbose again) to prevent memory leaks.

> 
> Off list Craig Ringer suggested introducing a new format specifier
> similar to %m for LSN but I did not get time to take a look at the
> relevant code. AFAIU it's available only to elog/ereport, so may not
> be useful generally. But teaching printf variants about the new format
> would be the best solution. However, I didn't find any way to do that.
> 

It seems that this topic has been extensively discussed off-list, but 
still strong +1 for the patch. I always wanted LSN printing to be more 
concise.

I have just tried new printing utilities in a couple of new places and 
it looks good to me.

+char *
+pg_lsn_out_internal(XLogRecPtr lsn)
+{
+	char		buf[MAXPG_LSNLEN + 1];
+
+	snprintf(buf, sizeof(buf), LSN_FORMAT, LSN_FORMAT_ARG(lsn));
+
+	return pstrdup(buf);
+}

Would it be a bit more straightforward if we palloc buf initially and 
just return a pointer instead of doing pstrdup()?


Regards
-- 
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company

  3. Re: Printing LSN made easy

    Japin Li <japinli@hotmail.com> — 2020-11-27T16:21:27Z

    Hi,

Here, we cannot use sizeof(but) to get the buf size, because it is a pointer, so it always
8 bytes on 64-bit or 4 bytes on 32-bit machine.

+char *
+pg_lsn_out_buffer(XLogRecPtr lsn, char *buf)
+{
+       snprintf(buf, sizeof(buf), LSN_FORMAT, LSN_FORMAT_ARG(lsn));
+
+       return buf;
+}

--
Best regards
Japin Li
ChengDu WenWu Information Technolog Co.,Ltd.


> On Nov 27, 2020, at 10:24 PM, Alexey Kondratov <a.kondratov@postgrespro.ru> wrote:
> 
> Hi,
> 
> On 2020-11-27 13:40, Ashutosh Bapat wrote:
>> Off list Peter Eisentraut pointed out that we can not use these macros
>> in elog/ereport since it creates problems for translations. He
>> suggested adding functions which return strings and use %s when doing
>> so.
>> The patch has two functions pg_lsn_out_internal() which takes an LSN
>> as input and returns a palloc'ed string containing the string
>> representation of LSN. This may not be suitable in performance
>> critical paths and also may leak memory if not freed. So there's
>> another function pg_lsn_out_buffer() which takes LSN and a char array
>> as input, fills the char array with the string representation and
>> returns the pointer to the char array. This allows the function to be
>> used as an argument in printf/elog etc. Macro MAXPG_LSNLEN has been
>> extern'elized for this purpose.
> 
> If usage of macros in elog/ereport can cause problems for translation, then even with this patch life is not get simpler significantly. For example, instead of just doing like:
> 
>             elog(WARNING,
> -                 "xlog min recovery request %X/%X is past current point %X/%X",
> -                 (uint32) (lsn >> 32), (uint32) lsn,
> -                 (uint32) (newMinRecoveryPoint >> 32),
> -                 (uint32) newMinRecoveryPoint);
> +                 "xlog min recovery request " LSN_FORMAT " is past current point " LSN_FORMAT,
> +                 LSN_FORMAT_ARG(lsn),
> +                 LSN_FORMAT_ARG(newMinRecoveryPoint));
> 
> we have to either declare two additional local buffers, which is verbose; or use pg_lsn_out_internal() and rely on memory contexts (or do pfree() manually, which is verbose again) to prevent memory leaks.
> 
>> Off list Craig Ringer suggested introducing a new format specifier
>> similar to %m for LSN but I did not get time to take a look at the
>> relevant code. AFAIU it's available only to elog/ereport, so may not
>> be useful generally. But teaching printf variants about the new format
>> would be the best solution. However, I didn't find any way to do that.
> 
> It seems that this topic has been extensively discussed off-list, but still strong +1 for the patch. I always wanted LSN printing to be more concise.
> 
> I have just tried new printing utilities in a couple of new places and it looks good to me.
> 
> +char *
> +pg_lsn_out_internal(XLogRecPtr lsn)
> +{
> +	char		buf[MAXPG_LSNLEN + 1];
> +
> +	snprintf(buf, sizeof(buf), LSN_FORMAT, LSN_FORMAT_ARG(lsn));
> +
> +	return pstrdup(buf);
> +}
> 
> Would it be a bit more straightforward if we palloc buf initially and just return a pointer instead of doing pstrdup()?
> 
> 
> Regards
> -- 
> Alexey Kondratov
> 
> Postgres Professional https://www.postgrespro.com
> Russian Postgres Company<0001-Make-it-easy-to-print-LSN.patch>

  4. Re: Printing LSN made easy

    Michael Paquier <michael@paquier.xyz> — 2020-11-29T07:52:59Z

    On Fri, Nov 27, 2020 at 04:10:27PM +0530, Ashutosh Bapat wrote:
> LSN_FORMAT_ARG expands to two comma separated arguments and is kinda
> open at both ends  but it's handy that way.

Agreed that's useful.

> Off list Peter Eisentraut pointed out that we can not use these macros
> in elog/ereport since it creates problems for translations. He
> suggested adding functions which return strings and use %s when doing
> so.

What's the problem with translations?  We already have equivalent
stuff with INT64_FORMAT that may map to %ld or %lld.  But here we have
a format that's set in stone.

> The patch has two functions pg_lsn_out_internal() which takes an LSN
> as input and returns a palloc'ed string containing the string
> representation of LSN. This may not be suitable in performance
> critical paths and also may leak memory if not freed.

I'd rather never use this flavor.  An OOM could mask the actual error
behind.

> Off list Craig Ringer suggested introducing a new format specifier
> similar to %m for LSN but I did not get time to take a look at the
> relevant code. AFAIU it's available only to elog/ereport, so may not
> be useful generally. But teaching printf variants about the new format
> would be the best solution. However, I didn't find any way to do that.

-1.  %m maps to errno, that is much more generic.  A set of macros
that maps to our internal format would be fine enough IMO.
--
Michael

  5. Re: Printing LSN made easy

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-11-29T17:10:21Z

    Michael Paquier <michael@paquier.xyz> writes:
> On Fri, Nov 27, 2020 at 04:10:27PM +0530, Ashutosh Bapat wrote:
>> Off list Craig Ringer suggested introducing a new format specifier
>> similar to %m for LSN but I did not get time to take a look at the
>> relevant code. AFAIU it's available only to elog/ereport, so may not
>> be useful generally. But teaching printf variants about the new format
>> would be the best solution. However, I didn't find any way to do that.

> -1.  %m maps to errno, that is much more generic.  A set of macros
> that maps to our internal format would be fine enough IMO.

Agreed.  snprintf.c is meant to implement a recognized standard
(ok, %m is a GNU extension, but it's still pretty standard).
I'm not on board with putting PG-only extensions in there.

			regards, tom lane

  6. Re: Printing LSN made easy

    Craig Ringer <craig.ringer@enterprisedb.com> — 2020-11-30T02:12:35Z

    On Mon, Nov 30, 2020 at 1:10 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Michael Paquier <michael@paquier.xyz> writes:
> > On Fri, Nov 27, 2020 at 04:10:27PM +0530, Ashutosh Bapat wrote:
> >> Off list Craig Ringer suggested introducing a new format specifier
> >> similar to %m for LSN but I did not get time to take a look at the
> >> relevant code. AFAIU it's available only to elog/ereport, so may not
> >> be useful generally. But teaching printf variants about the new format
> >> would be the best solution. However, I didn't find any way to do that.
>
> > -1.  %m maps to errno, that is much more generic.  A set of macros
> > that maps to our internal format would be fine enough IMO.
>
> Agreed.  snprintf.c is meant to implement a recognized standard
> (ok, %m is a GNU extension, but it's still pretty standard).
> I'm not on board with putting PG-only extensions in there.
>

Fair enough. I did not realise that %m was a GNU extension (never looked
closely) so I thought we had precedent for Pg specific extensions there.

Agreed in that case, no argument from me.

  7. Re: Printing LSN made easy

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2020-11-30T13:02:46Z

    On Fri, Nov 27, 2020 at 7:54 PM Alexey Kondratov
<a.kondratov@postgrespro.ru> wrote:
>
> Hi,
>
> On 2020-11-27 13:40, Ashutosh Bapat wrote:
> >
> > Off list Peter Eisentraut pointed out that we can not use these macros
> > in elog/ereport since it creates problems for translations. He
> > suggested adding functions which return strings and use %s when doing
> > so.
> >
> > The patch has two functions pg_lsn_out_internal() which takes an LSN
> > as input and returns a palloc'ed string containing the string
> > representation of LSN. This may not be suitable in performance
> > critical paths and also may leak memory if not freed. So there's
> > another function pg_lsn_out_buffer() which takes LSN and a char array
> > as input, fills the char array with the string representation and
> > returns the pointer to the char array. This allows the function to be
> > used as an argument in printf/elog etc. Macro MAXPG_LSNLEN has been
> > extern'elized for this purpose.
> >
>
> If usage of macros in elog/ereport can cause problems for translation,
> then even with this patch life is not get simpler significantly. For
> example, instead of just doing like:
>
>               elog(WARNING,
> -                 "xlog min recovery request %X/%X is past current point
> %X/%X",
> -                 (uint32) (lsn >> 32), (uint32) lsn,
> -                 (uint32) (newMinRecoveryPoint >> 32),
> -                 (uint32) newMinRecoveryPoint);
> +                 "xlog min recovery request " LSN_FORMAT " is past
> current point " LSN_FORMAT,
> +                 LSN_FORMAT_ARG(lsn),
> +                 LSN_FORMAT_ARG(newMinRecoveryPoint));
>
> we have to either declare two additional local buffers, which is
> verbose; or use pg_lsn_out_internal() and rely on memory contexts (or do
> pfree() manually, which is verbose again) to prevent memory leaks.

I agree, that using LSN_FORMAT is best, but if that's not allowed, at
least the pg_lsn_out_internal() and variants encapsulate the
LSN_FORMAT so that the callers don't need to remember those and we
have to change only a couple of places when the LSN_FORMAT itself
changes.

>
> >
> > Off list Craig Ringer suggested introducing a new format specifier
> > similar to %m for LSN but I did not get time to take a look at the
> > relevant code. AFAIU it's available only to elog/ereport, so may not
> > be useful generally. But teaching printf variants about the new format
> > would be the best solution. However, I didn't find any way to do that.
> >
>
> It seems that this topic has been extensively discussed off-list, but
> still strong +1 for the patch. I always wanted LSN printing to be more
> concise.
>
> I have just tried new printing utilities in a couple of new places and
> it looks good to me.

Thanks.

>
> +char *
> +pg_lsn_out_internal(XLogRecPtr lsn)
> +{
> +       char            buf[MAXPG_LSNLEN + 1];
> +
> +       snprintf(buf, sizeof(buf), LSN_FORMAT, LSN_FORMAT_ARG(lsn));
> +
> +       return pstrdup(buf);
> +}
>
> Would it be a bit more straightforward if we palloc buf initially and
> just return a pointer instead of doing pstrdup()?

Possibly. I just followed the code in pg_lsn_out(), which snprintf()
in a char array and then does pstrdup(). I don't quite understand the
purpose of that.

-- 
Best Wishes,
Ashutosh Bapat

  8. Re: Printing LSN made easy

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2020-11-30T13:06:42Z

    On Fri, Nov 27, 2020 at 9:51 PM Li Japin <japinli@hotmail.com> wrote:
>
> Hi,
>
> Here, we cannot use sizeof(but) to get the buf size, because it is a pointer, so it always
> 8 bytes on 64-bit or 4 bytes on 32-bit machine.

For an array, the sizeof() returns the size of memory consumed by the
array. See section "Application to arrays" at
https://en.wikipedia.org/wiki/Sizeof.
-- 
Best Wishes,
Ashutosh Bapat

  9. Re: Printing LSN made easy

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2020-11-30T13:18:32Z

    On Sun, Nov 29, 2020 at 1:23 PM Michael Paquier <michael@paquier.xyz> wrote:

> On Fri, Nov 27, 2020 at 04:10:27PM +0530, Ashutosh Bapat wrote:
> > LSN_FORMAT_ARG expands to two comma separated arguments and is kinda
> > open at both ends  but it's handy that way.
>
> Agreed that's useful.
>
> > Off list Peter Eisentraut pointed out that we can not use these macros
> > in elog/ereport since it creates problems for translations. He
> > suggested adding functions which return strings and use %s when doing
> > so.
>
> What's the problem with translations?  We already have equivalent
> stuff with INT64_FORMAT that may map to %ld or %lld.  But here we have
> a format that's set in stone.
>

Peter Eisentraut explained that the translation system can not handle
strings broken by macros, e.g. errmsg("foo" MACRO "bar"), since it doesn't
know statically what the macro resolves to. But I am not familiar with our
translation system myself. But if we allow INT64_FORMAT, LSN_FORMAT should
be allowed. That makes life much easier. We do not need other functions at
all.

Peter E or somebody familiar with translations can provide more
information.

>
> > The patch has two functions pg_lsn_out_internal() which takes an LSN
> > as input and returns a palloc'ed string containing the string
> > representation of LSN. This may not be suitable in performance
> > critical paths and also may leak memory if not freed.
>
> I'd rather never use this flavor.  An OOM could mask the actual error
> behind.
>

Hmm that's a good point. It might be useful in non-ERROR cases, merely
because it avoids declaring a char array :).

No one seems to reject this idea so I will add it to the next commitfest to
get more reviews esp. clarification around whether macros are enough or not.

--
Best Wishes,
Ashutosh

  10. Re: Printing LSN made easy

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2020-11-30T13:19:03Z

    On Sun, Nov 29, 2020 at 10:40 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Michael Paquier <michael@paquier.xyz> writes:
> > On Fri, Nov 27, 2020 at 04:10:27PM +0530, Ashutosh Bapat wrote:
> >> Off list Craig Ringer suggested introducing a new format specifier
> >> similar to %m for LSN but I did not get time to take a look at the
> >> relevant code. AFAIU it's available only to elog/ereport, so may not
> >> be useful generally. But teaching printf variants about the new format
> >> would be the best solution. However, I didn't find any way to do that.
>
> > -1.  %m maps to errno, that is much more generic.  A set of macros
> > that maps to our internal format would be fine enough IMO.
>
> Agreed.  snprintf.c is meant to implement a recognized standard
> (ok, %m is a GNU extension, but it's still pretty standard).
> I'm not on board with putting PG-only extensions in there.
>

Thanks for the clarification.

--
Best Wishes,
Ashutosh

  11. Re: Printing LSN made easy

    Japin Li <japinli@hotmail.com> — 2020-11-30T14:08:44Z

    Hi,

On Nov 30, 2020, at 9:06 PM, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com<mailto:ashutosh.bapat.oss@gmail.com>> wrote:

On Fri, Nov 27, 2020 at 9:51 PM Li Japin <japinli@hotmail.com<mailto:japinli@hotmail.com>> wrote:

Hi,

Here, we cannot use sizeof(but) to get the buf size, because it is a pointer, so it always
8 bytes on 64-bit or 4 bytes on 32-bit machine.

For an array, the sizeof() returns the size of memory consumed by the
array. See section "Application to arrays" at
https://en.wikipedia.org/wiki/Sizeof.

That’s true! However, in pg_lsn_out_buffer(), it converts to a pointer, not an array. See the following test:

```c
#include <stdio.h>
#include <stdint.h>

typedef uint64_t    XLogRecPtr;
typedef uint32_t    uint32;

#define MAXPG_LSNLEN                    17
#define LSN_FORMAT "%X/%X"
#define LSN_FORMAT_ARG(lsn) (uint32) ((lsn) >> 32), (uint32) (lsn)

char *
pg_lsn_out_buffer(XLogRecPtr lsn, char *buf)
{
  printf("pg_lsn_out_buffer: sizeof(buf) = %lu\n", sizeof(buf));
  snprintf(buf, sizeof(buf), LSN_FORMAT, LSN_FORMAT_ARG(lsn));
  return buf;
}

char *
pg_lsn_out_buffer1(XLogRecPtr lsn, char *buf, size_t len)
{
  printf("pg_lsn_out_buffer1: sizeof(buf) = %lu, len = %lu\n", sizeof(buf), len);
  snprintf(buf, len, LSN_FORMAT, LSN_FORMAT_ARG(lsn));
  return buf;
}

int
main(void)
{
  char buf[MAXPG_LSNLEN + 1];
  XLogRecPtr    lsn = 1234567UL;

  printf("main: sizeof(buf) = %lu\n", sizeof(buf));
  pg_lsn_out_buffer(lsn, buf);
  printf("buffer's content from pg_lsn_out_buffer: %s\n", buf);

  pg_lsn_out_buffer1(lsn, buf, sizeof(buf));
  printf("buffer's content from pg_lsn_out_buffer1: %s\n", buf);
  return 0;
}
```

The above output is:

```
main: sizeof(buf) = 18
pg_lsn_out_buffer: sizeof(buf) = 8
buffer's content from pg_lsn_out_buffer: 0/12D68
pg_lsn_out_buffer1: sizeof(buf) = 8, len = 18
buffer's content from pg_lsn_out_buffer1: 0/12D687
```

--
Best regards
Japin Li
ChengDu WenWu Information Technolog Co.,Ltd.

  12. Re: Printing LSN made easy

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2020-11-30T14:37:38Z

    On 2020-Nov-30, Ashutosh Bapat wrote:

> Peter Eisentraut explained that the translation system can not handle
> strings broken by macros, e.g. errmsg("foo" MACRO "bar"), since it doesn't
> know statically what the macro resolves to. But I am not familiar with our
> translation system myself. But if we allow INT64_FORMAT, LSN_FORMAT should
> be allowed. That makes life much easier. We do not need other functions at
> all.
> 
> Peter E or somebody familiar with translations can provide more
> information.

We don't allow INT64_FORMAT in translatable strings, period.  (See
commit 6a1cd8b9236d which undid some hacks we had to use to work around
the lack of it, by allowing %llu to take its place.)  For the same
reason, we cannot allow LSN_FORMAT or similar constructions in
translatable strings either.

If the solution to ugliness of LSN printing is going to require that we
add a "%s" which prints a previously formatted string with LSN_FORMAT,
it won't win us anything.

As annoyed as I am about our LSN printing, I don't think this patch
has the solutions we need.

  13. Re: Printing LSN made easy

    Andres Freund <andres@anarazel.de> — 2020-11-30T21:03:26Z

    Hi,

On 2020-11-29 12:10:21 -0500, Tom Lane wrote:
> Agreed.  snprintf.c is meant to implement a recognized standard
> (ok, %m is a GNU extension, but it's still pretty standard).
> I'm not on board with putting PG-only extensions in there.

I wonder if there's something we could layer on the elog.c level
instead. But I also don't like the idea of increasing the use of wrapper
functions that need to allocate string buffers than then need to get
copied in turn.

We could do something like
   errmsg("plain string arg: %s, conv string arg: %s", somestr, estr_lsn(lsn))

where estr_lsn() wouldn't do any conversion directly, instead setting up
a callback in ErrorData that knows how to do the type specific
conversion. Then during EVALUATE_MESSAGE() we'd evaluate the message
piecemeal and call the output callbacks for each arg, using the
StringInfo.

There's two main issues with something roughly like this:
1) We'd need to do format string parsing somewhere above snprintf.c,
   which isn't free.
2) Without relying on C11 / _Generic() some ugly macro hackery would be
   needed to have a argument-number indexed state. If we did rely on
   _Generic() we probably could do better, even getting rid of the need
   for something like estr_lsn().

Greetings,

Andres Freund

  14. Re: Printing LSN made easy

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2020-12-03T06:13:41Z

    On Mon, Nov 30, 2020 at 7:38 PM Li Japin <japinli@hotmail.com> wrote:

> Hi,
>
> On Nov 30, 2020, at 9:06 PM, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
> wrote:
>
> On Fri, Nov 27, 2020 at 9:51 PM Li Japin <japinli@hotmail.com> wrote:
>
>
> Hi,
>
> Here, we cannot use sizeof(but) to get the buf size, because it is a
> pointer, so it always
> 8 bytes on 64-bit or 4 bytes on 32-bit machine.
>
>
> For an array, the sizeof() returns the size of memory consumed by the
> array. See section "Application to arrays" at
> https://en.wikipedia.org/wiki/Sizeof.
>
>
> That’s true! However, in pg_lsn_out_buffer(), it converts to a pointer,
> not an array. See the following test:
>
>
Ah! Thanks for pointing that out. I have fixed this in my repository.
However, from Alvaro's reply it looks like the approach is not acceptable,
so I am not posting the fixed version here.

--
Best Wishes,
Ashutosh

  15. Re: Printing LSN made easy

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2020-12-03T06:38:39Z

    On Mon, Nov 30, 2020 at 8:07 PM Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:

> On 2020-Nov-30, Ashutosh Bapat wrote:
>
> > Peter Eisentraut explained that the translation system can not handle
> > strings broken by macros, e.g. errmsg("foo" MACRO "bar"), since it
> doesn't
> > know statically what the macro resolves to. But I am not familiar with
> our
> > translation system myself. But if we allow INT64_FORMAT, LSN_FORMAT
> should
> > be allowed. That makes life much easier. We do not need other functions
> at
> > all.
> >
> > Peter E or somebody familiar with translations can provide more
> > information.
>
> We don't allow INT64_FORMAT in translatable strings, period.  (See
> commit 6a1cd8b9236d which undid some hacks we had to use to work around
> the lack of it, by allowing %llu to take its place.)  For the same
> reason, we cannot allow LSN_FORMAT or similar constructions in
> translatable strings either.


> If the solution to ugliness of LSN printing is going to require that we
> add a "%s" which prints a previously formatted string with LSN_FORMAT,
> it won't win us anything.
>

Thanks for the commit. The commit reverted code which uses a char array to
print int64. On the same lines, using a character array to print an LSN
won't be acceptable. I agree.

Maybe we should update the section "Message-Writing Guidelines" in
doc/src/sgml/nls.sgml. I think it's implied by "Do not construct sentences
at run-time, like ... " but using a macro is not really constructing
sentences run time. Hopefully, some day, we will fix the translation system
to handle strings with macros and make it easier to print PG specific
objects in strings easily.


>
> As annoyed as I am about our LSN printing, I don't think this patch
> has the solutions we need.
>

I think we could at least accept the macros so that they can be used in
non-translatable strings i.e. use it with sprints, appendStringInfo
variants etc. The macros will also serve to document the string format of
LSN. Developers can then copy from the macros rather than copying from
"some other" (erroneous) usage in the code.

--
Best Wishes,
Ashutosh

  16. Re: Printing LSN made easy

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2021-01-20T06:25:37Z

    On 2020-11-27 11:40, Ashutosh Bapat wrote:
> The solution seems to be simple though. In the attached patch, I have
> added two macros
> #define LSN_FORMAT "%X/%X"
> #define LSN_FORMAT_ARG(lsn) (uint32) ((lsn) >> 32), (uint32) (lsn)
> 
> which can be used instead.

It looks like we are not getting any consensus on this approach.  One 
reduced version I would consider is just the second part, so you'd write 
something like

     snprintf(lsnchar, sizeof(lsnchar), "%X/%X",
              LSN_FORMAT_ARGS(lsn));

This would still reduce notational complexity quite a bit but avoid any 
funny business with the format strings.

-- 
Peter Eisentraut
2ndQuadrant, an EDB company
https://www.2ndquadrant.com/

  17. Re: Printing LSN made easy

    Michael Paquier <michael@paquier.xyz> — 2021-01-20T07:40:59Z

    On Wed, Jan 20, 2021 at 07:25:37AM +0100, Peter Eisentraut wrote:
> It looks like we are not getting any consensus on this approach.  One
> reduced version I would consider is just the second part, so you'd write
> something like
> 
>     snprintf(lsnchar, sizeof(lsnchar), "%X/%X",
>              LSN_FORMAT_ARGS(lsn));
> 
> This would still reduce notational complexity quite a bit but avoid any
> funny business with the format strings.

That seems reasonable to me.  So +1.
--
Michael

  18. Re: Printing LSN made easy

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2021-01-20T07:50:11Z

    On Wed, Jan 20, 2021 at 11:55 AM Peter Eisentraut <
peter.eisentraut@2ndquadrant.com> wrote:

> On 2020-11-27 11:40, Ashutosh Bapat wrote:
> > The solution seems to be simple though. In the attached patch, I have
> > added two macros
> > #define LSN_FORMAT "%X/%X"
> > #define LSN_FORMAT_ARG(lsn) (uint32) ((lsn) >> 32), (uint32) (lsn)
> >
> > which can be used instead.
>
> It looks like we are not getting any consensus on this approach.  One
> reduced version I would consider is just the second part, so you'd write
> something like
>
>      snprintf(lsnchar, sizeof(lsnchar), "%X/%X",
>               LSN_FORMAT_ARGS(lsn));
>
> This would still reduce notational complexity quite a bit but avoid any
> funny business with the format strings.
>

Thanks for looking into this. I would like to keep both the LSN_FORMAT and
LSN_FORMAT_ARGS but with a note that the first can not be used in elog() or
in messages which require localization. We have many other places doing
snprintf() and such stuff, which can use LSN_FORMAT. If we do so, the
functions to output string representation will not be needed so they can be
removed.

--
Best Wishes,
Ashutosh

  19. Re: Printing LSN made easy

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-01-21T00:29:59Z

    At Wed, 20 Jan 2021 16:40:59 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> On Wed, Jan 20, 2021 at 07:25:37AM +0100, Peter Eisentraut wrote:
> > It looks like we are not getting any consensus on this approach.  One
> > reduced version I would consider is just the second part, so you'd write
> > something like
> > 
> >     snprintf(lsnchar, sizeof(lsnchar), "%X/%X",
> >              LSN_FORMAT_ARGS(lsn));
> > 
> > This would still reduce notational complexity quite a bit but avoid any
> > funny business with the format strings.
> 
> That seems reasonable to me.  So +1.

That seems in the good balance. +1, too.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  20. Re: Printing LSN made easy

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2021-01-21T10:23:16Z

    On 2021-01-20 08:50, Ashutosh Bapat wrote:
> Thanks for looking into this. I would like to keep both the LSN_FORMAT 
> and LSN_FORMAT_ARGS but with a note that the first can not be used in 
> elog() or in messages which require localization. We have many other 
> places doing snprintf() and such stuff, which can use LSN_FORMAT. If we 
> do so, the functions to output string representation will not be needed 
> so they can be removed.

Then you'd end up with half the code doing this and half the code doing 
that.  That doesn't sound very attractive.  It's not like "%X/%X" is 
hard to type.

-- 
Peter Eisentraut
2ndQuadrant, an EDB company
https://www.2ndquadrant.com/

  21. Re: Printing LSN made easy

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2021-01-21T10:30:14Z

    On Thu, Jan 21, 2021 at 3:53 PM Peter Eisentraut <
peter.eisentraut@2ndquadrant.com> wrote:

> On 2021-01-20 08:50, Ashutosh Bapat wrote:
> > Thanks for looking into this. I would like to keep both the LSN_FORMAT
> > and LSN_FORMAT_ARGS but with a note that the first can not be used in
> > elog() or in messages which require localization. We have many other
> > places doing snprintf() and such stuff, which can use LSN_FORMAT. If we
> > do so, the functions to output string representation will not be needed
> > so they can be removed.
>
> Then you'd end up with half the code doing this and half the code doing
> that.  That doesn't sound very attractive.  It's not like "%X/%X" is
> hard to type.
>

:). That's true. I thought LSN_FORMAT would document the string
representation of an LSN. But anyway I am fine with this as well.


>
> --
> Peter Eisentraut
> 2ndQuadrant, an EDB company
> https://www.2ndquadrant.com/
>


-- 
--
Best Wishes,
Ashutosh

  22. Re: Printing LSN made easy

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2021-02-18T12:49:02Z

    Here is an updated patch that just introduces LSN_FORMAT_ARGS().  I 
think the result is quite pleasant.

-- 
Peter Eisentraut
2ndQuadrant, an EDB company
https://www.2ndquadrant.com/

  23. Re: Printing LSN made easy

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2021-02-18T13:21:37Z

    On Thu, Feb 18, 2021 at 6:19 PM Peter Eisentraut <
peter.eisentraut@2ndquadrant.com> wrote:

> Here is an updated patch that just introduces LSN_FORMAT_ARGS().  I
> think the result is quite pleasant.
>

Thanks a lot Peter for producing this patch. I am fine with it. The way
this is defined someone could write xyz = LSN_FORMAT_ARGS(lsn). But then
they are misusing it so I won't care. Even my proposal had that problem.

--
Best Wishes,
Ashutosh

  24. Re: Printing LSN made easy

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-02-19T01:54:05Z

    At Thu, 18 Feb 2021 18:51:37 +0530, Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> wrote in 
> On Thu, Feb 18, 2021 at 6:19 PM Peter Eisentraut <
> peter.eisentraut@2ndquadrant.com> wrote:
> 
> > Here is an updated patch that just introduces LSN_FORMAT_ARGS().  I
> > think the result is quite pleasant.
> >
> 
> Thanks a lot Peter for producing this patch. I am fine with it. The way
> this is defined someone could write xyz = LSN_FORMAT_ARGS(lsn). But then
> they are misusing it so I won't care. Even my proposal had that problem.

As for the side effect by expressions as the parameter, unary
operators are seldom (or never) applied to LSN. I think there's no
need to fear about other (modifying) expressions, too.

As a double-checking, I checked that the patch covers all output by
'%X/%X' and " ?>> ?32)" that are handling LSNs, and there's no
mis-replacing of the source variables.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  25. Re: Printing LSN made easy

    Michael Paquier <michael@paquier.xyz> — 2021-02-19T06:10:53Z

    On Fri, Feb 19, 2021 at 10:54:05AM +0900, Kyotaro Horiguchi wrote:
> At Thu, 18 Feb 2021 18:51:37 +0530, Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> wrote in 
>> On Thu, Feb 18, 2021 at 6:19 PM Peter Eisentraut <
>> peter.eisentraut@2ndquadrant.com> wrote:
>>> Here is an updated patch that just introduces LSN_FORMAT_ARGS().  I
>>> think the result is quite pleasant.

This result is nice for the eye.  Thanks Peter.
--
Michael