On 2020-Aug-18, Ashutosh Bapat wrote:
> Right now pg_waldump just prints whether the message is transactional
> or not and its size. That doesn't help much to understand the message
> itself. If it prints the contents of a logical WAL message, it helps
> debugging logical replication related problems. Prefix is a
> null-terminated ASCII string, so no problem printing that. Even the
> contents can be printed as a series of hex bytes. Here's a patch to do
> that.
Looks like a good idea.
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.
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.
> + 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:
736F 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
6520 6D 65 73 73 61 67 65
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services