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