Printing LSN made easy

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
25 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Printing LSN made easy

Ashutosh Bapat-2
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

0001-Make-it-easy-to-print-LSN.patch (9K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Printing LSN made easy

Alexey Kondratov-2
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 (9K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Printing LSN made easy

japin
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 <[hidden email]> 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>



Reply | Threaded
Open this post in threaded view
|

Re: Printing LSN made easy

Michael Paquier-2
In reply to this post by Ashutosh Bapat-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Printing LSN made easy

Tom Lane-2
Michael Paquier <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Printing LSN made easy

Craig Ringer-5
On Mon, Nov 30, 2020 at 1:10 AM Tom Lane <[hidden email]> wrote:
Michael Paquier <[hidden email]> 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.
Reply | Threaded
Open this post in threaded view
|

Re: Printing LSN made easy

Ashutosh Bapat-2
In reply to this post by Alexey Kondratov-2
On Fri, Nov 27, 2020 at 7:54 PM Alexey Kondratov
<[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Printing LSN made easy

Ashutosh Bapat-2
In reply to this post by japin
On Fri, Nov 27, 2020 at 9:51 PM Li Japin <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Printing LSN made easy

Ashutosh Bapat
In reply to this post by Michael Paquier-2


On Sun, Nov 29, 2020 at 1:23 PM Michael Paquier <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: Printing LSN made easy

Ashutosh Bapat
In reply to this post by Tom Lane-2


On Sun, Nov 29, 2020 at 10:40 PM Tom Lane <[hidden email]> wrote:
Michael Paquier <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: Printing LSN made easy

japin
In reply to this post by Ashutosh Bapat-2
Hi,

On Nov 30, 2020, at 9:06 PM, Ashutosh Bapat <[hidden email]> wrote:

On Fri, Nov 27, 2020 at 9:51 PM Li Japin <[hidden email]> 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.





Reply | Threaded
Open this post in threaded view
|

Re: Printing LSN made easy

Álvaro Herrera
In reply to this post by 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.


Reply | Threaded
Open this post in threaded view
|

Re: Printing LSN made easy

Andres Freund
In reply to this post by Tom Lane-2
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


Reply | Threaded
Open this post in threaded view
|

Re: Printing LSN made easy

Ashutosh Bapat
In reply to this post by japin


On Mon, Nov 30, 2020 at 7:38 PM Li Japin <[hidden email]> wrote:
Hi,

On Nov 30, 2020, at 9:06 PM, Ashutosh Bapat <[hidden email]> wrote:

On Fri, Nov 27, 2020 at 9:51 PM Li Japin <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: Printing LSN made easy

Ashutosh Bapat
In reply to this post by Álvaro Herrera


On Mon, Nov 30, 2020 at 8:07 PM Alvaro Herrera <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: Printing LSN made easy

Peter Eisentraut-6
In reply to this post by Ashutosh Bapat-2
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/


Reply | Threaded
Open this post in threaded view
|

Re: Printing LSN made easy

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Printing LSN made easy

Ashutosh Bapat
In reply to this post by Peter Eisentraut-6


On Wed, Jan 20, 2021 at 11:55 AM Peter Eisentraut <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: Printing LSN made easy

Kyotaro Horiguchi-4
In reply to this post by Michael Paquier-2
At Wed, 20 Jan 2021 16:40:59 +0900, Michael Paquier <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Printing LSN made easy

Peter Eisentraut-6
In reply to this post by Ashutosh Bapat
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/


12