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.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. > >> 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>