Re: Printing LSN made easy

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