Обсуждение: pg_waldump and PREPARE
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
Вложения
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
At Fri, 8 Nov 2019 09:53:07 +0900, Fujii Masao <masao.fujii@gmail.com> wrote in
> 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"?
On Fri, Apr 26, 2019 at 5:38 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> +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?
I agree to this in principle, but I'm afraid that we cannot do that
actually. Doing it straight way would result in something like this.
typedef struct xl_xact_prepare
{
uint32 magic;
...
TimestampTz origin_timestamp;
/* correct alignment here */
+ char gid[FLEXIBLE_ARRAY_MEMBER]; /* the GID of the prepred xact */
+ /* subxacts, xnodes, msgs and sentinel follow the gid[] array */
} xl_xact_prepare;
I don't think this is meaningful..
After all, the new xlog record struct is used only at one place.
xlog_redo is the correspondent of xact_desc, but it is not aware of
the stuct and PrepareRedoAdd decodes it using TwoPhaseFileHeader. In
that sense, the details of the record is a secret of twophase.c. What
is worse, apparently TwoPhaseFileHeader is a *subset* of
xl_xact_prepare but what we want to expose is the super set. Thus I
porpose to add a comment instead of exposing the full structure in
xl_xact_prepare definition.
typedef struct xl_xact_prepare
{
uint32 magic;
...
TimestampTz origin_timestamp;
+ /*
+ * This record has multiple trailing data sections with variable
+ * length. See twophase.c for the details.
+ */
} xl_xact_prepare;
Then, teach xlog_redo to resolve the record pointer to this type
before passing it to PrepareRedoAdd.
Does it make sense?
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Hello. At Fri, 8 Nov 2019 08:23:41 +0500, Andrey Lepikhov <a.lepikhov@postgrespro.ru> wrote in > > 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. Basically agrred, but it can be very large in certain cases, even if it is rare. By the way, in the patch xact_desc_prepare seems printing parseed.xact_time, which is not actually set by ParsePrepareRecord. # I missed the funtion. xl_xact_prepare is used in *two* places in # front end. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On 08/11/2019 09:26, Kyotaro Horiguchi wrote: > Hello. > > At Fri, 8 Nov 2019 08:23:41 +0500, Andrey Lepikhov <a.lepikhov@postgrespro.ru> wrote in >>> 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. > > Basically agrred, but it can be very large in certain cases, even if > it is rare. Maybe this is the reason for introducing a “verbose” option? -- Andrey Lepikhov Postgres Professional https://postgrespro.com The Russian Postgres Company
On Fri, Nov 8, 2019 at 1:33 PM Andrey Lepikhov <a.lepikhov@postgrespro.ru> wrote: > > > > On 08/11/2019 09:26, Kyotaro Horiguchi wrote: > > Hello. > > > > At Fri, 8 Nov 2019 08:23:41 +0500, Andrey Lepikhov <a.lepikhov@postgrespro.ru> wrote in > >>> 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. Thanks for the review! xid is already included in the pg_waldump output for PREPARE TRANSACTION record. Regarding subxacts, rels and invals, I agree that they might be useful when diagnosing 2PC-related trouble. I attached the updated version of the patch that also changes pg_waldump so that it outputs delete-on-commit rels, delete-on-aborts, subxacts and invals. Here is the example of output for PREPARE TRASACTION record, with the pach. rmgr: Transaction len (rec/tot): 837/ 837, tx: 503, lsn: 0/030055C8, prev 0/03005588, desc: PREPARE gid xxx: 2019-11-11 13:00:18.616056 JST; rels(commit): base/12923/16408 base/12923/16407 base/12923/16406 base/12923/16405; rels(abort): base/12923/16412 base/12923/16411 base/12923/16408 base/12923/16407; subxacts: 505; inval msgs: catcache 50 catcache 49 catcache 50 catcache 49 catcache 50 catcache 49 catcache 50 catcache 49 catcache 50 catcache 49 catcache 50 catcache 49 relcache 16386 relcache 16390 relcache 16390 relcache 16386 relcache 16386 relcache 16390 relcache 16390 relcache 16386 > By the way, in the patch xact_desc_prepare seems printing > parseed.xact_time, which is not actually set by ParsePrepareRecord. Thanks for the review! You are right. I fixed this issue in the attached patch. Regards, -- Fujii Masao
Вложения
On Mon, Nov 11, 2019 at 01:21:28PM +0900, Fujii Masao wrote: > Thanks for the review! You are right. > I fixed this issue in the attached patch. The proposed format looks fine to me. I have just one comment. All three callers of standby_desc_invalidations() don't actually need to print any data if there are zero messages, so you can simplify a bit xact_desc_commit() and xact_desc_prepare() regarding the check on parsed.nmsgs, no? -- Michael
Вложения
On Mon, Nov 11, 2019 at 4:16 PM Michael Paquier <michael@paquier.xyz> wrote: > > On Mon, Nov 11, 2019 at 01:21:28PM +0900, Fujii Masao wrote: > > Thanks for the review! You are right. > > I fixed this issue in the attached patch. > > The proposed format looks fine to me. I have just one comment. All > three callers of standby_desc_invalidations() don't actually need to > print any data if there are zero messages, so you can simplify a bit > xact_desc_commit() and xact_desc_prepare() regarding the check on > parsed.nmsgs, no? Thanks for the review! But probably I failed to understand your point... Could you clarify what actual change is necessary? You are thinking that the check of "parsed.nmsgs >= 0" should be moved to the inside of standby_desc_invalidations()? Regards, -- Fujii Masao
On Tue, Nov 12, 2019 at 05:53:02PM +0900, Fujii Masao wrote: > Thanks for the review! But probably I failed to understand your point... > Could you clarify what actual change is necessary? You are thinking that > the check of "parsed.nmsgs >= 0" should be moved to the inside of > standby_desc_invalidations()? Yes that's what I meant here. -- Michael
Вложения
On Tue, Nov 12, 2019 at 6:03 PM Michael Paquier <michael@paquier.xyz> wrote: > > On Tue, Nov 12, 2019 at 05:53:02PM +0900, Fujii Masao wrote: > > Thanks for the review! But probably I failed to understand your point... > > Could you clarify what actual change is necessary? You are thinking that > > the check of "parsed.nmsgs >= 0" should be moved to the inside of > > standby_desc_invalidations()? > > Yes that's what I meant here. Ok, I changed the patch that way. Attached is the latest version of the patch. Regards, -- Fujii Masao
Вложения
On Tue, Nov 12, 2019 at 06:41:12PM +0900, Fujii Masao wrote: > Ok, I changed the patch that way. > Attached is the latest version of the patch. Thanks for the new patch. Looks fine to me. -- Michael
Вложения
12.11.2019 12:41, Fujii Masao пишет: > Ok, I changed the patch that way. > Attached is the latest version of the patch. > > Regards, I did not see any problems in this version of the patch. The information displayed by pg_waldump for the PREPARE record is sufficient for use. -- Andrey Lepikhov Postgres Professional https://postgrespro.com The Russian Postgres Company
On Wed, Nov 13, 2019 at 3:53 PM Andrey Lepikhov <a.lepikhov@postgrespro.ru> wrote: > > > > 12.11.2019 12:41, Fujii Masao пишет: > > Ok, I changed the patch that way. > > Attached is the latest version of the patch. > > > > Regards, > > I did not see any problems in this version of the patch. The information > displayed by pg_waldump for the PREPARE record is sufficient for use. Thanks Andrey and Michael for the review! I committed the patch. Regards, -- Fujii Masao
>> I did not see any problems in this version of the patch. The >> information >> displayed by pg_waldump for the PREPARE record is sufficient for use. > > Thanks Andrey and Michael for the review! I committed the patch. > > Regards, Hi, There is a mistake in the comment in the definition of xl_xact_relfilenodes. This is a small patch to correct it. Regards, Yu Kimura
Вложения
On Wed, Nov 20, 2019 at 04:16:45PM +0900, btkimurayuzk wrote:
> typedef struct xl_xact_relfilenodes
> {
> - int nrels; /* number of subtransaction XIDs */
> + int nrels; /* number of relations */
> RelFileNode xnodes[FLEXIBLE_ARRAY_MEMBER];
> } xl_xact_relfilenodes;
> #define MinSizeOfXactRelfilenodes offsetof(xl_xact_relfilenodes, xnodes)
These are relations, and it smells like a copy-pasto coming from
xl_xact_subxacts. Thanks Kimura-san, committed.
--
Michael