Re: [REVIEW] pg_last_xact_insert_timestamp

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