pg_waldump and PREPARE

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

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

On 2019-Apr-26, Fujii Masao wrote:

On Fri, Apr 26, 2019 at 1:04 AM Alvaro Herrera <alvherre@2ndquadrant.com> 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

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

On Fri, Apr 26, 2019 at 5:38 AM Michael Paquier <michael@paquier.xyz> 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?

On Tue, Jul 2, 2019 at 7:16 PM Julien Rouhaud <rjuju123@gmail.com> wrote:

On Fri, Apr 26, 2019 at 5:38 AM Michael Paquier <michael@paquier.xyz> 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

On Wed, Jul 3, 2019 at 5:21 PM Fujii Masao <masao.fujii@gmail.com> wrote:

On Tue, Jul 2, 2019 at 7:16 PM Julien Rouhaud <rjuju123@gmail.com> wrote:

On Fri, Apr 26, 2019 at 5:38 AM Michael Paquier <michael@paquier.xyz> 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!

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

On Thu, Jul 4, 2019 at 9:45 AM Michael Paquier <michael@paquier.xyz> 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.

On Thu, Jul 4, 2019 at 8:25 PM Julien Rouhaud <rjuju123@gmail.com> wrote:

On Thu, Jul 4, 2019 at 9:45 AM Michael Paquier <michael@paquier.xyz> 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

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

Hi,

Le jeu. 1 août 2019 à 13:51, Michael Paquier <michael@paquier.xyz> 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.

On Thu, Aug 1, 2019 at 11:51 PM Michael Paquier <michael@paquier.xyz> 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

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

Sorry for the long delay...

On Thu, Jul 4, 2019 at 5:25 PM Julien Rouhaud <rjuju123@gmail.com> wrote:

On Thu, Jul 4, 2019 at 9:45 AM Michael Paquier <michael@paquier.xyz> 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

On Tue, Sep 3, 2019 at 3:04 AM Alvaro Herrera <alvherre@2ndquadrant.com> 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

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

On Fri, Nov 8, 2019 at 9:41 AM Michael Paquier <michael@paquier.xyz> 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

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

On Fri, Nov 8, 2019 at 9:41 AM Michael Paquier <michael@paquier.xyz> 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