Re: Add information to rm_redo_error_callback()

Hi,

On 8/10/20 7:10 AM, Masahiko Sawada wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you
canconfirm the sender and know the content is safe.
 
>
>
>
> 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:".

Thanks for looking at it!

I've attached a new version as per your comments.

Thanks,

Bertrand