Re: [REVIEW] pg_last_xact_insert_timestamp

On Mon, Aug 11, 2014 at 4:46 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> Hi,
>
> On 2011-10-04 20:52:59 +0900, Fujii Masao wrote:
>> *** a/src/backend/access/transam/xact.c
>> --- b/src/backend/access/transam/xact.c
>> ***************
>> *** 1066,1071 **** RecordTransactionCommit(void)
>> --- 1066,1074 ----
>>
>> (void) XLogInsert(RM_XACT_ID, XLOG_XACT_COMMIT_COMPACT, rdata);
>> }
>> +
>> + /* Save timestamp of latest transaction commit record */
>> + pgstat_report_xact_end_timestamp(xactStopTimestamp);
>> }
>>
>
> Perhaps that pgstat_report() should instead be combined with the
> pgstat_report_xact_timestamp(0) in CommitTransaction()? Then the number
> of changecount increases and cacheline references would stay the
> same. The only thing that'd change would be a single additional
> assignment.

Sounds good suggestion.

While reading the patch again, I found it didn't handle the COMMIT/ABORT
PREPARED case properly. According to the commit e74e090, now
pg_last_xact_replay_timestamp() returns the timestamp of COMMIT/ABORT PREPARED.
pg_last_xact_insert_timestamp() is mainly expected to be used to calculate
the replication delay, so it also needs to return that timestam. But the patch
didn't change 2PC code at all. We need to add pgstat_report_xact_end_timestamp()
into FinishPreparedTransaction(), RecordTransactionCommitPrepared() or
RecordTransactionAbortPrepared().

Regards,

--
Fujii Masao