[REVIEW] pg_last_xact_insert_timestamp

Hi, This is a review for pg_last_xact_insert_timestamp patch.
(https://commitfest.postgresql.org/action/patch_view?id=634)

Summary
====================
There's one question and two comments.

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?

And you can find the two more comment in "Code details" section.

I have no objection for commiter review with the Q1 above is
cleared.

Purpose and function of this patch
====================

This patch allows users to grip the amount of replay lag on the
standby by giving the timestamp of the WAL latestly inserted on
the primary.

Each backend writes the timestamp of the just inserted log record
of commit/abort freely on PgBackendStatus, and the function
pg_last_xact_insert_timestamp() gathers them for all backends
including inactive ones and returns the latest one, or NULL when
no timestamp is saved.

Implemented in lockless way.

Patch application, regression test
====================
This patch applies cleanly onto HEAD.
make world completed successfully. make check passed.
Style of the code and patch seems correct.

Documentation
====================
It looks properly documented for new functions.

Performance
====================

This patch writes one TimestampTz value on shmem per one WAL
insertion for commit/abort with no lock, and collect the values
for all backends on reading without any locks.

I think the former gives virtually no penalty for performance of
transactions, and the latter has no influence on other
transactions thanks to the lockless implement.

Code details
====================

== Shared memory initialization

beentry->st_xact_end_timestamp is not initialized on backend
startup. This is because the life time of the field is beyond
that of backends. On the other hand, Linux man page says that
newly created shared memory segment is zeroed, but I don't have
information about other OSs.

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).

== 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..

So I think at present it is needless to touch st_changecount
here.

Of cource the penalty is very close to nothing so it may be there
for future use...

== 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. Please
leave it as it is if you put more importance on the similarity
with the code inserted into RecordTransactionAbort().

Conclusion
====================

With the issue of shmem zeroing on other OSs is cleard, I have no
objection to commit this patch.

Sincerely,

--
Kyotaro Horiguchi
NTT Open Source Software Center