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: 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 -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
