On Wed, Sep 14, 2011 at 6:21 PM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@oss.ntt.co.jp> wrote:
> Hi, This is a review for pg_last_xact_insert_timestamp patch.
> (https://commitfest.postgresql.org/action/patch_view?id=634)
Thanks for the review!
> Q1: The shmem entry for timestamp is not initialized on
> allocating. Is this OK? (I don't know that for OSs other than
> Linux) And zeroing double field is OK for all OSs?
CreateSharedBackendStatus() initializes that shmem entries by doing
MemSet(BackendStatusArray, 0, size). You think this is not enough?
> Nevertheless this is ok for all OSs, I don't know whether
> initializing TimestampTz(double, int64 is ok) field with 8 bytes
> zeros is OK or not, for all platforms. (It is ok for
> IEEE754-binary64).
Which code are you concerned about?
> == Modification detection protocol in pgstat.c
>
> In pgstat_report_xact_end_timestamp, `beentry->st_changecount
> protocol' is used. It is for avoiding reading halfway-updated
> beentry as described in pgstat.h. Meanwhile,
> beentry->st_xact_end_timestamp is not read or (re-)initialized in
> pgstat.c and xlog.c reads only this field of whole beentry and
> st_changecount is not get cared here..
No, st_changecount is used to read st_xact_end_timestamp.
st_xact_end_timestamp is read from the shmem to the local memory
in pgstat_read_current_status(), and this function always checks
st_changecount when reading the shmem value.
> == Code duplication in xact.c
>
> in xact.c, same lines inserted into the end of both IF and ELSE
> blocks.
>
> xact.c:1047> pgstat_report_xact_end_timestamp(xlrec.xact_time);
> xact.c:1073> pgstat_report_xact_end_timestamp(xlrec.xact_time);
>
> These two lines refer to xlrec.xact_time, both of which comes
> from xactStopTimestamp freezed at xact.c:986
>
> xact.c:986> SetCurrentTransactionStopTimestamp();
> xact.c:1008> xlrec.xact_time = xactStopTimestamp;
> xact.c:1051> xlrec.xact_time = xactStopTimestamp;
>
> I think it is better to move this line to just after this ELSE
> block using xactStopTimestamp instead of xlrec.xact_time.
Okay, I've changed the patch in that way.
Regards,
--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center