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
