pg_waldump and PREPARE

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
33 messages Options
12
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
Reply | Threaded
Open this post in threaded view
|

Re: pg_waldump and PREPARE

Julien Rouhaud
On Fri, Apr 26, 2019 at 5:38 AM Michael Paquier <[hidden email]> wrote:

>
> 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 patch doesn't apply anymore, could you send a rebase?


Reply | Threaded
Open this post in threaded view
|

Re: pg_waldump and PREPARE

Fujii Masao-2
On Tue, Jul 2, 2019 at 7:16 PM Julien Rouhaud <[hidden email]> wrote:

>
> On Fri, Apr 26, 2019 at 5:38 AM Michael Paquier <[hidden email]> wrote:
> >
> > 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 patch doesn't apply anymore, could you send a rebase?
Yes, attached is the updated version of the patch.

Regards,

--
Fujii Masao

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

Re: pg_waldump and PREPARE

Julien Rouhaud
On Wed, Jul 3, 2019 at 5:21 PM Fujii Masao <[hidden email]> wrote:

>
> On Tue, Jul 2, 2019 at 7:16 PM Julien Rouhaud <[hidden email]> wrote:
> >
> > On Fri, Apr 26, 2019 at 5:38 AM Michael Paquier <[hidden email]> wrote:
> > >
> > > 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 patch doesn't apply anymore, could you send a rebase?
>
> Yes, attached is the updated version of the patch.

Thanks!

So the patch compiles and works as intended. I don't have much to say
about it, it all looks good to me, since the concerns about xactdesc.c
aren't worth the trouble.

I'm not sure that I understand Michael's objection though, as
xl_xact_prepare is not a new definition and AFAICS it couldn't contain
the records anyway.  So I'll let him say if he has further objections
or if it's ready for committer!


Reply | Threaded
Open this post in threaded view
|

Re: pg_waldump and PREPARE

Michael Paquier-2
On Wed, Jul 03, 2019 at 08:23:44PM +0200, Julien Rouhaud wrote:
> So the patch compiles and works as intended. I don't have much to say
> about it, it all looks good to me, since the concerns about xactdesc.c
> aren't worth the trouble.
>
> I'm not sure that I understand Michael's objection though, as
> xl_xact_prepare is not a new definition and AFAICS it couldn't contain
> the records anyway.  So I'll let him say if he has further objections
> or if it's ready for committer!

This patch provides parsing information only for the header of the 2PC
record.  Wouldn't it be interesting to get more information from the
various TwoPhaseRecordOnDisk's callbacks?  We could also print much
more information in xact_desc_prepare().  Like the subxacts, the XID,
the invalidation messages and the delete-on-abort/commit rels.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pg_waldump and PREPARE

Julien Rouhaud
On Thu, Jul 4, 2019 at 9:45 AM Michael Paquier <[hidden email]> wrote:

>
> On Wed, Jul 03, 2019 at 08:23:44PM +0200, Julien Rouhaud wrote:
> > So the patch compiles and works as intended. I don't have much to say
> > about it, it all looks good to me, since the concerns about xactdesc.c
> > aren't worth the trouble.
> >
> > I'm not sure that I understand Michael's objection though, as
> > xl_xact_prepare is not a new definition and AFAICS it couldn't contain
> > the records anyway.  So I'll let him say if he has further objections
> > or if it's ready for committer!
>
> This patch provides parsing information only for the header of the 2PC
> record.  Wouldn't it be interesting to get more information from the
> various TwoPhaseRecordOnDisk's callbacks?  We could also print much
> more information in xact_desc_prepare().  Like the subxacts, the XID,
> the invalidation messages and the delete-on-abort/commit rels.

Most of those are already described in the COMMIT PREPARE message,
wouldn't that be redundant?  abortrels aren't displayed anywhere
though, so +1 for adding them.

I also see that the dbid isn't displayed in any of the 2PC message,
that'd be useful to have it directly instead of looking for it in
other messages for the same transaction.


Reply | Threaded
Open this post in threaded view
|

Re: pg_waldump and PREPARE

Thomas Munro-5
On Thu, Jul 4, 2019 at 8:25 PM Julien Rouhaud <[hidden email]> wrote:

> On Thu, Jul 4, 2019 at 9:45 AM Michael Paquier <[hidden email]> wrote:
> > On Wed, Jul 03, 2019 at 08:23:44PM +0200, Julien Rouhaud wrote:
> > > So the patch compiles and works as intended. I don't have much to say
> > > about it, it all looks good to me, since the concerns about xactdesc.c
> > > aren't worth the trouble.
> > >
> > > I'm not sure that I understand Michael's objection though, as
> > > xl_xact_prepare is not a new definition and AFAICS it couldn't contain
> > > the records anyway.  So I'll let him say if he has further objections
> > > or if it's ready for committer!
> >
> > This patch provides parsing information only for the header of the 2PC
> > record.  Wouldn't it be interesting to get more information from the
> > various TwoPhaseRecordOnDisk's callbacks?  We could also print much
> > more information in xact_desc_prepare().  Like the subxacts, the XID,
> > the invalidation messages and the delete-on-abort/commit rels.
>
> Most of those are already described in the COMMIT PREPARE message,
> wouldn't that be redundant?  abortrels aren't displayed anywhere
> though, so +1 for adding them.
>
> I also see that the dbid isn't displayed in any of the 2PC message,
> that'd be useful to have it directly instead of looking for it in
> other messages for the same transaction.

Hello all,

I've moved this to the next CF, and set it to "Needs review" since a
rebase was provided.

--
Thomas Munro
https://enterprisedb.com


Reply | Threaded
Open this post in threaded view
|

Re: pg_waldump and PREPARE

Michael Paquier-2
On Thu, Aug 01, 2019 at 11:05:34PM +1200, Thomas Munro wrote:
> I've moved this to the next CF, and set it to "Needs review" since a
> rebase was provided.

I may be missing something of course, but in this case we argued about
adding a couple of more fields.  In consequence, the patch should be
waiting on its author, no?
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pg_waldump and PREPARE

Julien Rouhaud
Hi, 

Le jeu. 1 août 2019 à 13:51, Michael Paquier <[hidden email]> a écrit :
On Thu, Aug 01, 2019 at 11:05:34PM +1200, Thomas Munro wrote:
> I've moved this to the next CF, and set it to "Needs review" since a
> rebase was provided.

I may be missing something of course, but in this case we argued about
adding a couple of more fields.  In consequence, the patch should be
waiting on its author, no?

That's also my understanding. 
Reply | Threaded
Open this post in threaded view
|

Re: pg_waldump and PREPARE

Thomas Munro-5
In reply to this post by Michael Paquier-2
On Thu, Aug 1, 2019 at 11:51 PM Michael Paquier <[hidden email]> wrote:
> On Thu, Aug 01, 2019 at 11:05:34PM +1200, Thomas Munro wrote:
> > I've moved this to the next CF, and set it to "Needs review" since a
> > rebase was provided.
>
> I may be missing something of course, but in this case we argued about
> adding a couple of more fields.  In consequence, the patch should be
> waiting on its author, no?

Oh, OK.  Changed.  So we're waiting for Fujii-san to respond to the
suggestions about new fields.

--
Thomas Munro
https://enterprisedb.com


Reply | Threaded
Open this post in threaded view
|

Re: pg_waldump and PREPARE

Alvaro Herrera-9
In reply to this post by Michael Paquier-2
On 2019-Aug-01, Michael Paquier wrote:

> I may be missing something of course, but in this case we argued about
> adding a couple of more fields.  In consequence, the patch should be
> waiting on its author, no?

Fujii,

Are you in a position to submit an updated version of this patch?

Maybe Vignesh is in a position to help to complete this, since he has
been eyeing this code lately.  Vignesh?

Thanks,

--
Á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
In reply to this post by Julien Rouhaud
Sorry for the long delay...

On Thu, Jul 4, 2019 at 5:25 PM Julien Rouhaud <[hidden email]> wrote:

>
> On Thu, Jul 4, 2019 at 9:45 AM Michael Paquier <[hidden email]> wrote:
> >
> > On Wed, Jul 03, 2019 at 08:23:44PM +0200, Julien Rouhaud wrote:
> > > So the patch compiles and works as intended. I don't have much to say
> > > about it, it all looks good to me, since the concerns about xactdesc.c
> > > aren't worth the trouble.
> > >
> > > I'm not sure that I understand Michael's objection though, as
> > > xl_xact_prepare is not a new definition and AFAICS it couldn't contain
> > > the records anyway.  So I'll let him say if he has further objections
> > > or if it's ready for committer!
> >
> > This patch provides parsing information only for the header of the 2PC
> > record.  Wouldn't it be interesting to get more information from the
> > various TwoPhaseRecordOnDisk's callbacks?  We could also print much
> > more information in xact_desc_prepare().  Like the subxacts, the XID,
> > the invalidation messages and the delete-on-abort/commit rels.
>
> Most of those are already described in the COMMIT PREPARE message,
> wouldn't that be redundant?  abortrels aren't displayed anywhere
> though, so +1 for adding them.

xact_desc_abort() for ROLLBACK PREPARED describes abortrels. No?

> I also see that the dbid isn't displayed in any of the 2PC message,
> that'd be useful to have it directly instead of looking for it in
> other messages for the same transaction.

dbid is not reported even in COMMIT message. So I don't like adding
dbid into only the PREPARE message.

Regards,

--
Fujii Masao


Reply | Threaded
Open this post in threaded view
|

Re: pg_waldump and PREPARE

Fujii Masao-2
In reply to this post by Alvaro Herrera-9
On Tue, Sep 3, 2019 at 3:04 AM Alvaro Herrera <[hidden email]> wrote:

>
> On 2019-Aug-01, Michael Paquier wrote:
>
> > I may be missing something of course, but in this case we argued about
> > adding a couple of more fields.  In consequence, the patch should be
> > waiting on its author, no?
>
> Fujii,
>
> Are you in a position to submit an updated version of this patch?

Sorry for the long delay... Yes, I will update the patch if necessary.

Regards,

--
Fujii Masao


Reply | Threaded
Open this post in threaded view
|

Re: pg_waldump and PREPARE

Michael Paquier-2
On Tue, Sep 03, 2019 at 10:00:08AM +0900, Fujii Masao wrote:
> Sorry for the long delay... Yes, I will update the patch if necessary.

Fujii-san, are you planning to update this patch then?  I have
switched it as waiting on author.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pg_waldump and PREPARE

Fujii Masao-2
On Fri, Nov 8, 2019 at 9:41 AM Michael Paquier <[hidden email]> wrote:
>
> On Tue, Sep 03, 2019 at 10:00:08AM +0900, Fujii Masao wrote:
> > Sorry for the long delay... Yes, I will update the patch if necessary.
>
> Fujii-san, are you planning to update this patch then?  I have
> switched it as waiting on author.

No because there has been nothing to update in the latest patch for now
unless I'm missing something. So I'm just waiting for some new review
comments against the latest patch to come :)
Can I switch the status back to "Needs review"?

Regards,

--
Fujii Masao


Reply | Threaded
Open this post in threaded view
|

Re: pg_waldump and PREPARE

Andrey Lepikhov


On 08/11/2019 05:53, Fujii Masao wrote:

> On Fri, Nov 8, 2019 at 9:41 AM Michael Paquier <[hidden email]> wrote:
>>
>> On Tue, Sep 03, 2019 at 10:00:08AM +0900, Fujii Masao wrote:
>>> Sorry for the long delay... Yes, I will update the patch if necessary.
>>
>> Fujii-san, are you planning to update this patch then?  I have
>> switched it as waiting on author.
>
> No because there has been nothing to update in the latest patch for now
> unless I'm missing something. So I'm just waiting for some new review
> comments against the latest patch to come :)
> Can I switch the status back to "Needs review"?
>
> Regards,
>

One issue is that your patch provides small information. WAL errors
Investigation often requires information on xid, subxacts,
delete-on-abort/commit rels; rarely - invalidation messages etc.

--
Andrey Lepikhov
Postgres Professional
https://postgrespro.com
The Russian Postgres Company


12