Re: Print logical WAL message content

Thanks Alvaro for review.

On Wed, Aug 19, 2020 at 3:21 AM Alvaro Herrera <alvherre(at)2ndquadrant(dot)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