Обсуждение: Printing LSN made easy

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

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

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
simplersignificantly. 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
memorycontexts (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
LSNprinting 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>

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

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

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

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

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.

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

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/

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

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

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/

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/

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

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