Re: pg_waldump erroneously outputs newline for FPWs, and another minor bug

Hi,

On 2019-10-29 16:33:41 -0700, Andres Freund wrote:
> Hi,
>
> When using -b, --bkp-details pg_waldump outputs an unnecessary newline
> for blocks that contain an FPW.
>
> In --bkp-details block references are output on their own lines, like:
>
> rmgr: SPGist len (rec/tot): 4348/ 4348, tx: 980, lsn: 0/01985818, prev 0/01983850, desc: PICKSPLIT ndel 92; nins 93
> blkref #0: rel 1663/16384/16967 fork main blk 3
> blkref #1: rel 1663/16384/16967 fork main blk 6
> blkref #2: rel 1663/16384/16967 fork main blk 5
> blkref #3: rel 1663/16384/16967 fork main blk 1
> rmgr: Heap len (rec/tot): 69/ 69, tx: 980, lsn: 0/01986930, prev 0/01985818, desc: INSERT off 2 flags 0x00
> blkref #0: rel 1663/16384/16961 fork main blk 1
>
> but unfortunately, when there's actually an FPW present, it looks like:
>
> rmgr: XLOG len (rec/tot): 75/ 11199, tx: 977, lsn: 0/019755E0, prev 0/0194EDD8, desc: FPI
> blkref #0: rel 1663/16384/16960 fork main blk 32 (FPW); hole: offset: 548, length: 4484
>
> blkref #1: rel 1663/16384/16960 fork main blk 33 (FPW); hole: offset: 548, length: 4484
>
> blkref #2: rel 1663/16384/16960 fork main blk 34 (FPW); hole: offset: 548, length: 4484
>
> rmgr: Heap len (rec/tot): 188/ 188, tx: 977, lsn: 0/019781D0, prev 0/019755E0, desc: INPLACE off 23
>
> which clearly seems unnecessary. Looking at the code it seems to me that
>
> static void
> XLogDumpDisplayRecord(XLogDumpConfig *config, XLogReaderState *record)
> {
> ...
> printf("\tblkref #%u: rel %u/%u/%u fork %s blk %u",
> block_id,
> rnode.spcNode, rnode.dbNode, rnode.relNode,
> forkNames[forknum],
> blk);
> if (XLogRecHasBlockImage(record, block_id))
> {
> if (record->blocks[block_id].bimg_info &
> BKPIMAGE_IS_COMPRESSED)
> {
> printf(" (FPW%s); hole: offset: %u, length: %u, "
> "compression saved: %u\n",
> XLogRecBlockImageApply(record, block_id) ?
> "" : " for WAL verification",
> record->blocks[block_id].hole_offset,
> record->blocks[block_id].hole_length,
> BLCKSZ -
> record->blocks[block_id].hole_length -
> record->blocks[block_id].bimg_len);
> }
> else
> {
> printf(" (FPW%s); hole: offset: %u, length: %u\n",
> XLogRecBlockImageApply(record, block_id) ?
> "" : " for WAL verification",
> record->blocks[block_id].hole_offset,
> record->blocks[block_id].hole_length);
> }
> }
> putchar('\n');
>
> was intended to not actually print a newline in the printfs in the if
> preceding the putchar.
>
> This is a fairly longstanding bug, introduced in:
>
> commit 2c03216d831160bedd72d45f712601b6f7d03f1c
> Author: Heikki Linnakangas <heikki(dot)linnakangas(at)iki(dot)fi>
> Date: 2014-11-20 17:56:26 +0200
>
> Revamp the WAL record format.
>
>
> Does anybody have an opinion about fixing it just in master or also
> backpatching it? I guess there could be people having written parsers
> for the waldump output? I'm inclined to backpatch.
>
>
> I also find a second minor bug:
>
> static void
> XLogDumpDisplayRecord(XLogDumpConfig *config, XLogReaderState *record)
> {
> ...
> const char *id;
> ...
> id = desc->rm_identify(info);
> if (id == NULL)
> id = psprintf("UNKNOWN (%x)", info & ~XLR_INFO_MASK);
> ...
> printf("desc: %s ", id);
>
> after that "id" is not referenced anymore. Which means we would leak
> memory if there were a lot of UNKNOWN records. This is from
> commit 604f7956b9460192222dd37bd3baea24cb669a47
> Author: Andres Freund <andres(at)anarazel(dot)de>
> Date: 2014-09-22 16:48:14 +0200
>
> Improve code around the recently added rm_identify rmgr callback.
>
> While not a lot of memory, it's not absurd to run pg_waldump against a
> large amount of WAL, so backpatching seems mildly advised.
>
> I'm inlined to think that the best fix is to just move the relevant code
> to the callsite, and not psprintf'ing into a temporary buffer. We'd need
> additional state to free the memory, as rm_identify returns a static
> buffer.
>
> So I'll make it
>
> id = desc->rm_identify(info);
> if (id == NULL)
> printf("desc: UNKNOWN (%x) ", info & ~XLR_INFO_MASK);
> else
> printf("desc: %s ", id);

Pushed fixes for these.

Greetings,

Andres Freund