On Fri, Nov 27, 2020 at 7:54 PM Alexey Kondratov
<a.kondra...@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
