Re: pg_waldump and PREPARE

On Fri, Apr 26, 2019 at 1:04 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>
> On 2019-Apr-26, Fujii Masao wrote:
>
> > Hi,
> >
> > pg_waldump report no detail information about PREPARE TRANSACTION record,
> > as follows.
> >
> >     rmgr: Transaction len (rec/tot):    250/   250, tx:        485,
> > lsn: 0/020000A8, prev 0/02000060, desc: PREPARE
> >
> > I'd like to modify pg_waldump, i.e., xact_desc(), so that it reports
> > detail information like GID, as follows. Attached patch does that.
> > This would be helpful, for example, when diagnosing 2PC-related
> > trouble by checking the status of 2PC transaction with the specified GID.
> > Thought?
> >
> >     rmgr: Transaction len (rec/tot):    250/   250, tx:        485,
> > lsn: 0/020000A8, prev 0/02000060, desc: PREPARE gid abc: 2004-06-17
> > 05:26:27.500240 JST
>
> I think this is a great change to make.

Thanks!

> Strangely, before your patch, ParsePrepareRecord seems completely
> unused.

Yes, this seems to be the leftover of commit 1eb6d6527a.

> I'm not sure I like the moving of that routine to xactdesc.c ...
> on one hand, it would be side-by-side with ParseCommitRecord, but OTOH
> it seems weird to have twophase.c call xactdesc.c.

I moved ParsePrepareRecord() to xactdesc.c because it should be
accessed in backend (when replaying WAL) and frontend (pg_waldump) code
and xactdesc.c looked like proper place for that purpose
ParseCommitRecord() is also in xactdesc.c because of the same reason..

>  I also wonder if
> defining the structs in the way you do is the most sensible arrangement.

I did that arrangement because the format of PREPARE TRANSACTION record,
i.e., that struct, also needs to be accessed in backend and frontend.
But, of course, if there is smarter way, I'm happy to adopt that!

Regards,

-- 
Fujii Masao