Re: Add information to rm_redo_error_callback()

On Wed, 5 Aug 2020 at 00:37, Drouvot, Bertrand <bdrouvot@amazon.com> wrote:
>
> Hi hackers,
>
> I've attached a small patch to add information to rm_redo_error_callback().
>
> The changes attached in this patch came while working on the "Add
> information during standby recovery conflicts" patch (See [1]).
>
> The goal is to add more information during the callback (if doable), so
> that something like:
>
> 2020-08-04 14:42:57.545 UTC [15459] CONTEXT:  WAL redo at 0/4A3B0DE0 for
> Heap2/CLEAN: remxid 1168
>
> would get extra information that way:
>
> 2020-08-04 14:42:57.545 UTC [15459] CONTEXT:  WAL redo at 0/4A3B0DE0 for
> Heap2/CLEAN: remxid 1168, blkref #0: rel 1663/13586/16850 fork main blk 0
>
> As this could be useful outside of [1], a dedicated "sub" patch has been
> created (thanks Sawada for the suggestion).
>
> I will add this patch to the next commitfest. I look forward to your
> feedback about the idea and/or implementation.
>

Thank you for starting the new thread for this patch!

I think this patch is simple enough and improves information shown in
errcontext.

I have two comments on the patch:

diff --git a/src/backend/access/transam/xlog.c
b/src/backend/access/transam/xlog.c
index 756b838e6a..8b2024e9e9 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -11749,10 +11749,22 @@ rm_redo_error_callback(void *arg)
 {
    XLogReaderState *record = (XLogReaderState *) arg;
    StringInfoData buf;
+   int block_id;
+   RelFileNode rnode;
+   ForkNumber forknum;
+   BlockNumber blknum;

    initStringInfo(&buf);
    xlog_outdesc(&buf, record);

+   for (block_id = 0; block_id <= record->max_block_id; block_id++)
+   {
+       if (XLogRecGetBlockTag(record, block_id, &rnode, &forknum, &blknum))
+           appendStringInfo(&buf,", blkref #%d: rel %u/%u/%u fork %s blk %u",
+                       block_id, rnode.spcNode, rnode.dbNode,
+                       rnode.relNode, forkNames[forknum],
+                       blknum);
+   }
    /* translator: %s is a WAL record description */
    errcontext("WAL redo at %X/%X for %s",
               (uint32) (record->ReadRecPtr >> 32),

rnode, forknum and blknum can be declared within the for loop.

I think it's better to put a new line just before the comment starting
from "translator:".

Regards,

-- 
Masahiko Sawada            http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services