Thanks Alvaro for review.
On Wed, Aug 19, 2020 at 3:21 AM Alvaro Herrera <alvhe...@2ndquadrant.com> wrote:
>
> I didn't like that you're documenting the message format in the new
> function:
>
> > xl_logical_message *xlrec = (xl_logical_message *) rec;
> > + /*
> > + * Per LogLogicalMessage() actual logical message follows a
> > null-terminated prefix of length
> > + * prefix_size.
>
> I would prefer to remove this comment, and instead add a comment atop
> xl_logical_message's struct definition in message.h to say that the
> message has a valid C-string as prefix, whose length is prefix_size, and
> please see logicalmesg_desc() if you change this.
It's documented in the struct definition. Added a note about logicalmesg_desc().
> This way, you don't need to blame LogLogicalMessage for this
> restriction, but it's actually part of the definition of the WAL
> message.
>
> > + /*
> > + * Per LogLogicalMessage() actual logical message follows a
> > null-terminated prefix of length
> > + * prefix_size.
> > + */
> > + char *prefix = xlrec->message;
> > + char *message = xlrec->message + xlrec->prefix_size;
> > + int cnt;
> > + char *sep = "";
>
> This would cause a crash if the message actually fails to follow the
> rule. Let's test that prefix[xlrec->prefix_size] is a trailing zero,
> and if not, avoid printing it. Although, just Assert()'ing that it's a
> trailing zero would seem to suffice.
Added an Assert.
>
> > + appendStringInfo(buf, "%s message size %zu bytes, prefix %s;
> > mesage: ",
> > xlrec->transactional ?
> > "transactional" : "nontransactional",
> > - xlrec->message_size);
> > + xlrec->message_size, prefix);
>
> Misspelled "message", but also the line looks a bit repetitive -- the
> word "message" would appear three times:
>
> > lsn: 0/01570608, prev 0/015705D0, desc: MESSAGE nontransactional message
> > size 12 bytes, prefix some_prefix; mesage: 73 6F 6D 65 20 6D 65 73 73 61 67
> > 65
>
> I would reduce it to
>
> > lsn: 0/01570608, prev 0/015705D0, desc: MESSAGE nontransactional, prefix
> > "some_prefix"; payload (12 bytes): 73 6F 6D 65 20 6D 65 73 73 61 67 65
I like this format as well. Done.
PFA the patch attached with your comments addressed.
Thanks for your review.
--
Best Wishes,
Ashutosh Bapat
From d32859c8368ac25366b156c3247a5b77d9b72ce8 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <ashutosh.ba...@2ndquadrant.com>
Date: Tue, 18 Aug 2020 11:05:29 +0530
Subject: [PATCH] Print prefix and logical WAL message content in pg_waldump
Print logical WAL message prefix which is a null terminated ASCII
string. Print the actual message as a space separated hex byte string
since it may contain binary data. This is useful for debugging purposes.
Ashutosh Bapat, reviewed by Alvaro Herrera
---
src/backend/access/rmgrdesc/logicalmsgdesc.c | 16 ++++++++++++++--
src/include/replication/message.h | 4 +++-
2 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/src/backend/access/rmgrdesc/logicalmsgdesc.c b/src/backend/access/rmgrdesc/logicalmsgdesc.c
index bff298c928..5da2625a0b 100644
--- a/src/backend/access/rmgrdesc/logicalmsgdesc.c
+++ b/src/backend/access/rmgrdesc/logicalmsgdesc.c
@@ -24,10 +24,22 @@ logicalmsg_desc(StringInfo buf, XLogReaderState *record)
if (info == XLOG_LOGICAL_MESSAGE)
{
xl_logical_message *xlrec = (xl_logical_message *) rec;
+ char *prefix = xlrec->message;
+ char *message = xlrec->message + xlrec->prefix_size;
+ int cnt;
+ char *sep = "";
- appendStringInfo(buf, "%s message size %zu bytes",
+ Assert(prefix[xlrec->prefix_size] != '\0');
+
+ appendStringInfo(buf, "%s, prefix \"%s\"; payload (%zu bytes): ",
xlrec->transactional ? "transactional" : "nontransactional",
- xlrec->message_size);
+ prefix, xlrec->message_size);
+ /* Write actual message as a series of hex bytes. */
+ for (cnt = 0; cnt < xlrec->message_size; cnt++)
+ {
+ appendStringInfo(buf, "%s%02X", sep, (unsigned char)message[cnt]);
+ sep = " ";
+ }
}
}
diff --git a/src/include/replication/message.h b/src/include/replication/message.h
index 937addde48..7b882296ec 100644
--- a/src/include/replication/message.h
+++ b/src/include/replication/message.h
@@ -25,7 +25,9 @@ typedef struct xl_logical_message
Size message_size; /* size of the message */
char message[FLEXIBLE_ARRAY_MEMBER]; /* message including the null
* terminated prefix of length
- * prefix_size */
+ * prefix_size. Please see
+ * logicalmsg_desc(), if you
+ * change this. */
} xl_logical_message;
#define SizeOfLogicalMessage (offsetof(xl_logical_message, message))
--
2.17.1
