Re: Add information to rm_redo_error_callback()

On Tue, 11 Aug 2020 at 00:07, Drouvot, Bertrand <bdrouvot@amazon.com> wrote:
>
> 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.

Thank you for updating the patch!

The patch looks good to me. I've set this patch as Ready for Committer.

Regards,

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