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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
