Re: Printing LSN made easy

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>