Re: [REVIEW] pg_last_xact_insert_timestamp

On Wed, Aug 13, 2014 at 12:41:24PM +0900, Fujii Masao wrote:
> On Mon, Aug 11, 2014 at 8:27 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> > On Mon, Aug 11, 2014 at 4:46 PM, Andres Freund <andres@2ndquadrant.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.

Is this going to be applied?

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +