On Thu, Feb 10, 2022 at 10:18:15AM +0900, Kyotaro Horiguchi wrote: > Who repoerted that to you?
Let's bet on Coverity. > StartPrepare and EndPrepare are virtually a single function that > accepts some additional operations in the middle. StartPrepare leaves > the "records" incomplete then EndPrepare completes it. It is not > designed so that the fields are accessed from others before the > completion. There seems to be no critical reasons for EndPrepare to > do the pointed operations, but looking that another replorigin-related > operation is needed in the same function, it is sensible that the > author intended to consolidate all replorigin related changes in > EndPrepare. Well, that's the intention I recall from reading this code a couple of weeks ago. In the worst case, this could be considered as a bad practice as having clean data helps at least debugging if you look at this data between the calls of StartPrepare() and EndPrepare(), and we do things this way for all the other fields, like total_len. The proposed change is incomplete anyway once you consider this argument. First, there is no need to set up those fields in EndPrepare() anymore if there is no origin data, no? It would be good to comment that these are filled in EndPrepare(), I guess, once you do the initialization in StartPrepare(). I agree that those changes are purely cosmetic. -- Michael
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index 608c5149e5..874c8ed125 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -1074,6 +1074,9 @@ StartPrepare(GlobalTransaction gxact)
hdr.ninvalmsgs = xactGetCommittedInvalidationMessages(&invalmsgs,
&hdr.initfileinval);
hdr.gidlen = strlen(gxact->gid) + 1; /* Include '\0' */
+ /* EndPrepare will fill the origin data, if necessary */
+ hdr.origin_lsn = InvalidXLogRecPtr;
+ hdr.origin_timestamp = 0;
save_state_data(&hdr, sizeof(TwoPhaseFileHeader));
save_state_data(gxact->gid, hdr.gidlen);
@@ -1133,11 +1136,6 @@ EndPrepare(GlobalTransaction gxact)
hdr->origin_lsn = replorigin_session_origin_lsn;
hdr->origin_timestamp = replorigin_session_origin_timestamp;
}
- else
- {
- hdr->origin_lsn = InvalidXLogRecPtr;
- hdr->origin_timestamp = 0;
- }
/*
* If the data size exceeds MaxAllocSize, we won't be able to read it in
signature.asc
Description: PGP signature
