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
From 698e481f5f55b967b5c60dba4bc577f8baa20ff4 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <ashutosh.ba...@2ndquadrant.com>
Date: Fri, 16 Oct 2020 17:09:29 +0530
Subject: [PATCH] Make it easy to print LSN
The commit introduces following macros and functions to make it easy to
use LSNs in printf variants, elog, ereport and appendStringInfo
variants.
LSN_FORMAT - macro representing the format in which LSN is printed
LSN_FORMAT_ARG - macro to pass LSN as an argument to the above format
pg_lsn_out_internal - a function which returns palloc'ed char array
containing string representation of given LSN.
pg_lsn_out_buffer - similar to above but accepts and returns a char
array of size (MAXPG_LSNLEN + 1)
The commit also has some example usages of these.
Ashutosh Bapat
---
contrib/pageinspect/rawpage.c | 3 +-
src/backend/access/rmgrdesc/replorigindesc.c | 5 +-
src/backend/access/rmgrdesc/xlogdesc.c | 3 +-
src/backend/access/transam/xlog.c | 8 ++--
src/backend/utils/adt/pg_lsn.c | 49 ++++++++++++++------
src/include/access/xlogdefs.h | 7 +++
src/include/utils/pg_lsn.h | 3 ++
7 files changed, 55 insertions(+), 23 deletions(-)
diff --git a/contrib/pageinspect/rawpage.c b/contrib/pageinspect/rawpage.c
index c0181506a5..2cd055a5f0 100644
--- a/contrib/pageinspect/rawpage.c
+++ b/contrib/pageinspect/rawpage.c
@@ -261,8 +261,7 @@ page_header(PG_FUNCTION_ARGS)
{
char lsnchar[64];
- snprintf(lsnchar, sizeof(lsnchar), "%X/%X",
- (uint32) (lsn >> 32), (uint32) lsn);
+ snprintf(lsnchar, sizeof(lsnchar), LSN_FORMAT, LSN_FORMAT_ARG(lsn));
values[0] = CStringGetTextDatum(lsnchar);
}
else
diff --git a/src/backend/access/rmgrdesc/replorigindesc.c b/src/backend/access/rmgrdesc/replorigindesc.c
index 19e14f910b..a3f49b5750 100644
--- a/src/backend/access/rmgrdesc/replorigindesc.c
+++ b/src/backend/access/rmgrdesc/replorigindesc.c
@@ -29,10 +29,9 @@ replorigin_desc(StringInfo buf, XLogReaderState *record)
xlrec = (xl_replorigin_set *) rec;
- appendStringInfo(buf, "set %u; lsn %X/%X; force: %d",
+ appendStringInfo(buf, "set %u; lsn " LSN_FORMAT "; force: %d",
xlrec->node_id,
- (uint32) (xlrec->remote_lsn >> 32),
- (uint32) xlrec->remote_lsn,
+ LSN_FORMAT_ARG(xlrec->remote_lsn),
xlrec->force);
break;
}
diff --git a/src/backend/access/rmgrdesc/xlogdesc.c b/src/backend/access/rmgrdesc/xlogdesc.c
index 3200f777f5..22bdae5d9a 100644
--- a/src/backend/access/rmgrdesc/xlogdesc.c
+++ b/src/backend/access/rmgrdesc/xlogdesc.c
@@ -89,8 +89,7 @@ xlog_desc(StringInfo buf, XLogReaderState *record)
XLogRecPtr startpoint;
memcpy(&startpoint, rec, sizeof(XLogRecPtr));
- appendStringInfo(buf, "%X/%X",
- (uint32) (startpoint >> 32), (uint32) startpoint);
+ appendStringInfo(buf, LSN_FORMAT, LSN_FORMAT_ARG(startpoint));
}
else if (info == XLOG_PARAMETER_CHANGE)
{
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 13f1d8c3dc..2436af5244 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -79,6 +79,7 @@
#include "utils/pg_rusage.h"
#include "utils/snapmgr.h"
#include "utils/timestamp.h"
+#include "utils/pg_lsn.h"
extern uint32 bootstrap_data_checksum_version;
@@ -5886,15 +5887,16 @@ recoveryStopsAfter(XLogReaderState *record)
recoveryTargetInclusive &&
record->ReadRecPtr >= recoveryTargetLSN)
{
+ char buf[MAXPG_LSNLEN + 1];
+
recoveryStopAfter = true;
recoveryStopXid = InvalidTransactionId;
recoveryStopLSN = record->ReadRecPtr;
recoveryStopTime = 0;
recoveryStopName[0] = '\0';
ereport(LOG,
- (errmsg("recovery stopping after WAL location (LSN) \"%X/%X\"",
- (uint32) (recoveryStopLSN >> 32),
- (uint32) recoveryStopLSN)));
+ (errmsg("recovery stopping after WAL location (LSN) \"%s\"",
+ pg_lsn_out_buffer(recoveryStopLSN, buf))));
return true;
}
diff --git a/src/backend/utils/adt/pg_lsn.c b/src/backend/utils/adt/pg_lsn.c
index ad0a7bd869..403a20c568 100644
--- a/src/backend/utils/adt/pg_lsn.c
+++ b/src/backend/utils/adt/pg_lsn.c
@@ -19,7 +19,6 @@
#include "utils/numeric.h"
#include "utils/pg_lsn.h"
-#define MAXPG_LSNLEN 17
#define MAXPG_LSNCOMPONENT 8
/*----------------------------------------------------------
@@ -81,18 +80,7 @@ Datum
pg_lsn_out(PG_FUNCTION_ARGS)
{
XLogRecPtr lsn = PG_GETARG_LSN(0);
- char buf[MAXPG_LSNLEN + 1];
- char *result;
- uint32 id,
- off;
-
- /* Decode ID and offset */
- id = (uint32) (lsn >> 32);
- off = (uint32) lsn;
-
- snprintf(buf, sizeof buf, "%X/%X", id, off);
- result = pstrdup(buf);
- PG_RETURN_CSTRING(result);
+ PG_RETURN_CSTRING(pg_lsn_out_internal(lsn));
}
Datum
@@ -317,3 +305,38 @@ pg_lsn_mii(PG_FUNCTION_ARGS)
/* Convert to pg_lsn */
return DirectFunctionCall1(numeric_pg_lsn, res);
}
+
+/*
+ * pg_lsn_out_internal
+ *
+ * Returns palloc'ed string representation of an LSN. Used by pg_lsn_out() but
+ * is useful to print LSN in non-performance critical paths. The caller may
+ * pfree the returned memory chunk.
+ */
+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);
+}
+
+/*
+ * pg_lsn_out_buffer
+ *
+ * Similar to the above function but saves the string representation of LSN in
+ * the given buffer which is expected to be at least MAXPG_LSNLEN long. It can
+ * be used in performance critical paths to avoid expensive memory allocation.
+ *
+ * The function returns pointer to the input buffer so that it can be used in
+ * printf variants.
+ */
+char *
+pg_lsn_out_buffer(XLogRecPtr lsn, char *buf)
+{
+ snprintf(buf, sizeof(buf), LSN_FORMAT, LSN_FORMAT_ARG(lsn));
+
+ return buf;
+}
diff --git a/src/include/access/xlogdefs.h b/src/include/access/xlogdefs.h
index e1f5812213..c704ca2454 100644
--- a/src/include/access/xlogdefs.h
+++ b/src/include/access/xlogdefs.h
@@ -35,6 +35,13 @@ typedef uint64 XLogRecPtr;
*/
#define FirstNormalUnloggedLSN ((XLogRecPtr) 1000)
+/* Handy macros to print LSN */
+#define LSN_FORMAT "%X/%X"
+#define LSN_FORMAT_ARG(lsn) (uint32) ((lsn) >> 32), (uint32) (lsn)
+
+/* Maximum length of string representation of an LSN */
+#define MAXPG_LSNLEN 17
+
/*
* XLogSegNo - physical log file sequence number.
*/
diff --git a/src/include/utils/pg_lsn.h b/src/include/utils/pg_lsn.h
index 25d6c5b38e..7a8637d2bd 100644
--- a/src/include/utils/pg_lsn.h
+++ b/src/include/utils/pg_lsn.h
@@ -26,4 +26,7 @@
extern XLogRecPtr pg_lsn_in_internal(const char *str, bool *have_error);
+extern char *pg_lsn_out_internal(XLogRecPtr lsn);
+extern char *pg_lsn_out_buffer(XLogRecPtr lsn, char *buf);
+
#endif /* PG_LSN_H */
--
2.17.1
