Re: [REVIEW] pg_last_xact_insert_timestamp

Sorry for late to re-review.

 One question is remaind, 

> > 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?
> 
> CreateSharedBackendStatus() initializes that shmem entries by doing
> MemSet(BackendStatusArray, 0, size). You think this is not enough?

 Sorry for missing it. That's enough.

> > 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).
> 
> Which code are you concerned about?

xlog.c: 5889
> 	beentry = pgstat_fetch_all_beentry();
> 
> 	for (i = 0; i < MaxBackends; i++, beentry++)
> 	{
> 		xtime = beentry->st_xact_end_timestamp;

 I think the last line in quoted code above reads possibly
zero-initialized double (or int64) value, then the doubted will
be compared and copied to another double.

> 		if (result < xtime)
> 			result = xtime;
> 

  
> No, st_changecount is used to read st_xact_end_timestamp.
> st_xact_end_timestamp is read from the shmem to the local memory
> in pgstat_read_current_status(), and this function always checks
> st_changecount when reading the shmem value.

 Yes, I confirmed that pg_lats_xact_insert_timestamp looks local
copy of BackendStatus. I've found it not unnecessary. 

-- 
Kyotaro Horiguchi
NTT Open Source Software Center