Re: [REVIEW] pg_last_xact_insert_timestamp

On Mon, Aug 11, 2014 at 8:27 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> 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.

I attached the updated version of the patch. I changed pgstat_report_xx
functions like Andres suggested.

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

Done.

Regards,

--
Fujii Masao