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 can confirm the sender and know the content is safe.
>
>
>
> On Wed, 5 Aug 2020 at 00:37, Drouvot, Bertrand <bdrouvot(at)amazon(dot)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