pg_waldump and PREPARE

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

pg_waldump and PREPARE

Fujii Masao-2
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 will add this to next CommitFest page.

Regards,

--
Fujii Masao

prepare-xact-desc.patch (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pg_waldump and PREPARE

Alvaro Herrera-9
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.

Strangely, before your patch, ParsePrepareRecord seems completely
unused.

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 also wonder if
defining the structs in the way you do is the most sensible arrangement.

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: pg_waldump and PREPARE

Fujii Masao-2
On Fri, Apr 26, 2019 at 1:04 AM Alvaro Herrera <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: pg_waldump and PREPARE

Alvaro Herrera-9
On 2019-Apr-26, Fujii Masao wrote:

> On Fri, Apr 26, 2019 at 1:04 AM Alvaro Herrera <[hidden email]> wrote:

> >  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!

I don't know.  I spent some time staring at the code involved, and it
seems it'd be possible to improve just a little bit on cleanliness
grounds, with a lot of effort, but not enough practical value.

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: pg_waldump and PREPARE

Michael Paquier-2
On Thu, Apr 25, 2019 at 03:08:36PM -0400, Alvaro Herrera wrote:
> On 2019-Apr-26, Fujii Masao wrote:
>> 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!
>
> I don't know.  I spent some time staring at the code involved, and it
> seems it'd be possible to improve just a little bit on cleanliness
> grounds, with a lot of effort, but not enough practical value.

Describing those records is something we should do.  There are other
parsing routines in xactdesc.c for commit and abort records, so having
that extra routine for prepare at the same place does not sound
strange to me.

+typedef xl_xact_prepare TwoPhaseFileHeader;
I find this mapping implementation a bit lazy, and your
newly-introduced xl_xact_prepare does not count for all the contents
of the actual WAL record for PREPARE TRANSACTION.  Wouldn't it be
better to put all the contents of the record in the same structure,
and not only the 2PC header information?

This is not v12 material of course.
--
Michael

signature.asc (849 bytes) Download Attachment