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

Is this going to be applied?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +