From: | Li Japin <japinli(at)hotmail(dot)com> |
---|---|
To: | Alexey Kondratov <a(dot)kondratov(at)postgrespro(dot)ru> |
Cc: | Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>, Craig Ringer <craig(dot)ringer(at)enterprisedb(dot)com> |
Subject: | Re: Printing LSN made easy |
Date: | 2020-11-27 16:21:27 |
Message-ID: | 0269732D-C6DF-4D03-9436-32A0AF7578CC@hotmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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(dot)kondratov(at)postgrespro(dot)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>
From | Date | Subject | |
---|---|---|---|
Next Message | Stephen Frost | 2020-11-27 16:28:20 | Re: A few new options for CHECKPOINT |
Previous Message | Stephen Frost | 2020-11-27 16:15:27 | Re: Online verification of checksums |